magefree / mage

Magic Another Game Engine
http://xmage.today
MIT License
1.91k stars 778 forks source link

Multiple copies of Linked Abilities incorrectly see each other #12752

Open ssk97 opened 2 months ago

ssk97 commented 2 months ago

Right now, Linked Abilities are implemented by storing information on the card/permanent in a manner accessible to all other abilities. This mostly works, but has problems when linked abilities are granted to other cards, which can cause separate abilities to look up their information using the same string. (This information is stored in many different locations: costs tags, exile zone names, game.getState().setValue, etc.)

For Costs Tags, one example of this is [[Zinnia, Valley's Voice]], where if casting a spell that already has Offspring should make separate triggers and offspring for each cost paid. This also applies to Kicker and Squad (though those currently have no cards where that matters except for the un-set [[Wicker Picker]]) and potentially others.

I think we need some sort of more general way to attach linked abilities to each other, not sure exactly how. This linkage must remain across deep copy effects, but must also create a new link when copying the card it's on (such as by making a non-legendary copy of Zinnia). I don't currently have any solution here, I just wanted to bring the issue up and discuss. For more information on the correct rules, see rule 607 Linked Abilities.

github-actions[bot] commented 2 months ago

Zinnia, Valley's Voice - (Gatherer) (Scryfall) (EDHREC)

{U}{R}{W} Legendary Creature — Bird Bard 1/3 Flying Zinnia, Valley's Voice gets +X/+0, where X is the number of other creatures you control with base power 1. Creature spells you cast have offspring {2}. (You may pay an additional {2} as you cast a creature spell. If you do, when that creature enters, create a 1/1 token copy of it.)

Wicker Picker - (Gatherer) (Scryfall) (EDHREC)

{3} Artifact Creature — Scarecrow Guest 2/3 Creature spells you cast have sticker kicker {1}. (You may pay an additional {1} as you cast a creature spell. If you do, you get {TK}, then you may put a sticker on it.)

JayDi85 commented 2 months ago

Game state and watchers with stored data already has all tools. No needs to link java objects -- it's not compatible with game engine (object copied all around, abilities get's new ids on trigger, activate, etc).

Card id + zcc + searching key are good. There are also originalId logic (if you need really link two abilities and support multiple instances), but it's a rare usage and I don't recommend to use it without real needs.

ssk97 commented 2 months ago

Card id + zcc + searching key are good. There are also originalId logic (if you need really link two abilities and support multiple instances), but it's a rare usage and I don't recommend to use it without real needs.

"it's a rare usage" is true in that it is rare for the game state to be such that it matters, but there are several abilities where it can unexpectedly show up and the current behavior is wrong. As an example, have [[Izzet Chemister]] and [[Experiment Kraj]]. If you flicker the Chemister, Kraj should lose track of the previous cards it exiled.

Another example would be a card with "Choose a color", then becomes a copy of a card that also has a "choose a color" ability. Current XMage uses the same color for the 2nd, but it should not.

Implementing originalId logic everywhere might be able to handle those issues, but it's quite complex to handle correctly, and must be custom re-implemented every time it's used.

github-actions[bot] commented 2 months ago

Izzet Chemister - (Gatherer) (Scryfall) (EDHREC)

{2}{R} Creature — Goblin Wizard 1/3 Haste {R}, {T}: Exile target instant or sorcery card from your graveyard. {1}{R}, {T}, Sacrifice Izzet Chemister: Cast any number of cards exiled with Izzet Chemister without paying their mana costs.

Experiment Kraj - (Gatherer) (Scryfall) (EDHREC)

{2}{G}{G}{U}{U} Legendary Creature — Ooze Mutant 4/6 Experiment Kraj has all activated abilities of each other creature with a +1/+1 counter on it. {T}: Put a +1/+1 counter on target creature.

JayDi85 commented 2 months ago

Izzet see exiled cards

Yep, that’s wrong usage of xmage’s exile zones (it was a standard code style for years — use shared source related exile zone instead ability related). Card must use some unique prefix for a zone name, so only linked ability will access to it. Izzet don’t have ruling for ref but [[Admonition Angel]] has:

Admonition Angel's landfall ability and its last ability are linked. The last ability refers only to cards exiled with its landfall ability. (2010-03-01)

BTW same problem for other effects with standard names for a keys like “damagedPermanent”, etc. It can be accessed by wrong effects in theory. The only reason it’s a not popular bug — there is a little amount of cards and mechanics with massive abilities combine (hello mutate).

github-actions[bot] commented 2 months ago

Admonition Angel - (Gatherer) (Scryfall) (EDHREC)

{3}{W}{W}{W} Creature — Angel 6/6 Flying Landfall — Whenever a land you control enters, you may exile target nonland permanent other than Admonition Angel. When Admonition Angel leaves the battlefield, return all cards exiled with it to the battlefield under their owners' control.

ssk97 commented 2 months ago

Yep, that’s wrong usage of xmage’s exile zones (it was a standard code style for years — use shared source related exile zone instead ability related). Card must use some unique prefix for a zone name, so only linked ability will access to it.

Even if you add a unique prefix, the above interaction still wouldn't work since both the original and the post-flicker ability would use the same prefix and thus see the same exile zone. This is wrong (assuming I understood the rules correctly. I haven't confirmed with a judge that the new Chemister permanent gives the same Kraj new abilities).

JayDi85 commented 2 months ago

Even if you add a unique prefix, the above interaction still wouldn't work since both the original and the post-flicker ability would use the same prefix and thus see the same exile zone

It must work with flicker too. Zone name = ability prefix + card/permanent id + zcc. There are special methods to find source zone with zcc offset (e.g. to support “on battlefield leave” logic).

ssk97 commented 2 months ago

Even if you add a unique prefix, the above interaction still wouldn't work since both the original and the post-flicker ability would use the same prefix and thus see the same exile zone

It must work with flicker too. Zone name = ability prefix + card/permanent id + zcc. There are special methods to find source zone with zcc offset (e.g. to support “on battlefield leave” logic).

Ability prefix: same

Permanent ID+ZCC: same because the Experiment Kraj is the card with the ability and it has stayed on the battlefield the whole time, its zcc has not changed. The Chemister the ability is being copied from has a new zcc, but that's not helpful here.

JayDi85 commented 2 months ago

I don't understand you.

Izzet + Kraj use case:

  1. Exile cards by Izzet;
  2. Put counter and give ability to Kraj;
  3. Exile cards by Kraj and gained Izzet's ability;
  4. Exile cards by Kraj and gained another's ability (for example);

Results on good implementation with existing tools (prefix + source zone + zcc):

ssk97 commented 2 months ago

Ah, my apologies, I wasn't clear enough in the scenario. Start with Izzet Chemister and Experiment Kraj on the battlefield.

1) Put a counter on Izzet, Kraj gains Izzet's Abilities 2) Kraj activates Izzet's 1st ability and exiles a card 3) Blink Izzet, Kraj loses abilities 4) Put a counter on Izzet, Kraj gains the new Izzet's Abilities 5) Kraj activates Izzet's 2nd ability

In step 5 there should be no cards found to be used, since it should be a new ability, copied from a new permanent. Izzet has a new zcc, but Kraj does not. AFAIK there is no reasonable way to have this interaction be correct with the current tools.

JayDi85 commented 2 months ago

Well, I see. It's a weird use case to "blink" a single ability instead whole object. It's incompatible with current "copy abilities from existing objects" logic (current copy logic keep originalId to support linked abilities). E.g. require some type of ability's zcc.

BUT I think that's whole thing with ability's zcc is wrong -- it's SAME ability by MTG rules cause it ref to printed:

607.1a An ability printed on an object within another ability that grants that ability to that object is considered to be “printed on” that object for these purposes.

If not then any [[Blood Moon]] or other "remove all abilities" must broke all choices and links after reset abilities to normal state.

github-actions[bot] commented 2 months ago

Blood Moon - (Gatherer) (Scryfall) (EDHREC)

{2}{R} Enchantment Nonbasic lands are Mountains.

ssk97 commented 2 months ago

I'm not certain about that but here's another, simpler example: Two Izzet Chemisters are on the battlefield, granting their abilities to one Experiment Kraj. The Kraj should have all four abilities, two pairs of links.

I guess originalId would work for this situation? That doesn't seem like a good solution though.

JayDi85 commented 2 months ago

Yes, originalId as prefix must work with two independent pairs if linked abilities (exile id = prefix/original + object id + zcc)

ssk97 commented 2 months ago

Yes, originalId as prefix must work with two independent pairs if linked abilities (exile id = prefix/original + object id + zcc)

Currently, this breaks if the card is copied - that causes all originalIds to be reset to new ones, and there's no way to update the linked ability's stored originalId when they're changed during that copy.