Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG // Propaganda asks to pay again infinitly #10372

Open
randomsuspect opened this issue May 14, 2023 · 15 comments
Open

BUG // Propaganda asks to pay again infinitly #10372

randomsuspect opened this issue May 14, 2023 · 15 comments
Labels
bug Bugs and errors

Comments

@randomsuspect
Copy link

I tried to pinpoint the bug and came to conclusion it must be Propaganda itself. I'll just explain the situation:

My Opponent has 2 Propaganda's. One Original and one Mirrormade Copy. I cast obliterate. Other 2 Players scoop. 1 opponent left. The 2 Propagandas are the only Permaments left on his side. Next Turn i flip my The Kami War into O-Kagachi Made Manifest. The next turn I have Sol ring and mana crypt on the bf, I try to attack with O-Kagachi but after i paid 2 for each Propaganda it asks me again. I also tried to play another creature, it still asks more then once for each Propaganda. I destroyed one of his Propaganda (The original) and with 6 Mana open i tried to attack with a third creature (only with that single creature). Still the same: After I click yes and pay the (2) the Yes/no buttons turn up immediatly again and ask me if i want to Pay to attack.

First i thought it my have something to do with O-Kagachi being a Saga and a relativly new Card, but after i tried the other creatures i m sure it has to do with either Propaganda Or Mirrormade. Maybe it started bugging after the last update; i m not sure. I will edit if i run into Propaganda again.

@xenohedron xenohedron added the bug Bugs and errors label Jan 8, 2024
@JayDi85
Copy link
Member

JayDi85 commented Feb 25, 2024

[[Propaganda]]

Copy link

Propaganda - (Gatherer) (Scryfall) (EDHREC)

{2}{U}
Enchantment
Creatures can't attack you unless their controller pays {2} for each creature they control that's attacking you.

@Susucre
Copy link
Contributor

Susucre commented Apr 1, 2024

Has been reported again on discord.
I failed to replicate it, so must need special circonstances to happen?

@JayDi85
Copy link
Member

JayDi85 commented Apr 1, 2024

Discord story has copies of propaganda. Topic starter also used a copy by [[Mirrormade]].

Copy link

github-actions bot commented Apr 1, 2024

Mirrormade - (Gatherer) (Scryfall) (EDHREC)

{1}{U}{U}
Enchantment
You may have Mirrormade enter the battlefield as a copy of any artifact or enchantment on the battlefield.

@Susucre
Copy link
Contributor

Susucre commented Apr 1, 2024

Oh I tried with tokens, not actual copy. Could be ability ids being lost somewhere maybe?

@JayDi85
Copy link
Member

JayDi85 commented Apr 1, 2024

Usage of ability ids or ability refs can be bad, cause it changes all around by game engine. Copy process also change it:

IMG_0111

IMG_0112

@jimga150
Copy link
Contributor

Trying to wrap my head around this.

It looks like, from my experimentation, that the issue is how the screenshotted code above (PermanentImpl.addAbility(Ability ability, UUID sourceId, Game game, boolean fromExistingObject)) generates a new UUID for the ability its copying onto the current permanent. This is normally necessary to distiguish between otherwise identical and identically sourced abilities copied using different effects, but results in an incredible loop in this specific instance:

Propaganda gets copied as a permanent with Mirrormade, causing Mirrormade's ability ID (ID 2) to be different from that of Propaganda's ability ID (ID 1). This is fine, up until the ability is applied--the attacker tapping lands for mana causes all continuous effects to be re-checked and reapplied, since something might have changed, which causes the ability generating the copied "pay to attack" effect (PayCostToAttackBlockEffectImpl) to be copied again, since its existence is the result of a CopyEffect.

This gives it a third ID (ID 3), different from IDs 1 and 2. Since a replacement effect with a brand new ID is detected, and hasn't been consumed (it WAS consumed, really, but under ID 2, so the do./while loop in `ContinuousEffects.replaceEvent' can't tell), it gets applied again. This loop of paying mana to accidentally make a "brand new" identical replacement effect in the game state repeats infinitely.

This theory is confirmed when you are able to pay the 2 mana to attack the Mirrormade-controlling player /without/ tapping any lands--With [[Omnath, Locus of Mana]] on the battlefield, save some green mana for the declare attackers step, and use that. The loop suddenly stops.

The question of how to solve this seems rough to me. We can't just not generate a new ID for this ability.

Could we replace this UUID randomization with a deterministic UUID generation, for example by simply adding together the UUIDs of the ability to copy and the permanent receiving the copied ability? That way. every time Propaganda's ability is copied with PermanentImpl.addAbility(Ability ability, UUID sourceId, Game game, boolean fromExistingObject), the ID generated from its ability is the same.

Another idea is to add a property to abilities with which to retrieve the information about a copied ability, enough to distinguish between otherwise identical copies that are from different effects: Probably the UUID of the ability it copied from and the UUID of the effect causing the copy. Then, `ContinuousEffects.replaceEvent' would need to look for that info, further deepening an already convoluted loop.

@xenohedron
Copy link
Contributor

It sounds similar to the bug #8363 with Whipgrass Entangler, for which a card-specific solution was implemented in #10802. But this manifestation is broader and I don't think can be solved in the same way.

Deterministic UUID generation for copy effects is an interesting idea. It seems like it has the potential to be the cleanest solution, haven't thought about it enough yet though.

See #11077 for potential related scope as well

@ssk97
Copy link
Contributor

ssk97 commented Sep 19, 2024

Deterministic UUID generation could also potentially be a solution to #12752, as the linked subability could find the new ID of its linked ability by running the same algorithm whenever its own new ID function is called.

@JayDi85
Copy link
Member

JayDi85 commented Sep 19, 2024

Omg, looks like I miss #10802. That hacks smells very bad.

Try to look at custom cost implementation — it has full control over payed status and all related data (e.g. you can have already payed cost without real pay). And it used all around xmage. Ability’s original id also can help (it’s same after copy). Like that: original id + source id + attacking id. Just a theory.

@jimga150
Copy link
Contributor

See #11077 for potential related scope as well

Tons of places where copies just get new UUIDs with no way to corroborate between identical copies made for the same effect instance. It seems like something akin to what @Susucre did for LinkedEffectIdStaticAbility should be rolled into AbilityImpl.

@jimga150
Copy link
Contributor

Omg, looks like I miss #10802. That hacks smells very bad.

@JayDi85 it smells bad that the workaround is for just one card, or the idea of an ability linking to its copy source at all?

@jimga150
Copy link
Contributor

Try to look at custom cost implementation

I'm not sure this would address the root cause. needing to tap lands and that causing the loop is incidental to this issue, no?

@JayDi85
Copy link
Member

JayDi85 commented Sep 19, 2024

I'm not sure this would address the root cause. needing to tap lands and that causing the loop is incidental to this issue

Due bug description — copy/gain ability generates unique instances on each game cycle. And that instances must keep payed status, but it lost it. Manual control of payed status can help with it.

it smells bad that the workaround is for just one card, or the idea of an ability linking to its copy source at all

For PR. It’s force data change in reverse order (effect trying to change ability). E.g. List’s item trying to change that List from itself. It’s replace root methods logic (id generation), but for special use cases only. It’s require to replace pack of standard commands by alternatives (instead fully replace/fix it - see copy ability code). Also it’s not finished, e.g. added to 1 of 10 places, see #11077.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugs and errors
Projects
None yet
Development

No branches or pull requests

6 participants