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

Multiple copies of Linked Abilities incorrectly see each other #12752

Open
ssk97 opened this issue Aug 29, 2024 · 16 comments
Open

Multiple copies of Linked Abilities incorrectly see each other #12752

ssk97 opened this issue Aug 29, 2024 · 16 comments
Labels
Developers Discussion Discussion about redesign or changes engine

Comments

@ssk97
Copy link
Contributor

ssk97 commented Aug 29, 2024

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.

@ssk97 ssk97 added Developers Discussion Discussion about redesign or changes engine labels Aug 29, 2024
Copy link

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
Copy link
Member

JayDi85 commented Aug 29, 2024

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
Copy link
Contributor Author

ssk97 commented Aug 29, 2024

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.

Copy link

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
Copy link
Member

JayDi85 commented Aug 29, 2024

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).

Copy link

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
Copy link
Contributor Author

ssk97 commented Aug 29, 2024

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
Copy link
Member

JayDi85 commented Aug 29, 2024

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
Copy link
Contributor Author

ssk97 commented Aug 29, 2024

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
Copy link
Member

JayDi85 commented Aug 29, 2024

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):

  • Izzet's view:
    • own exiled cards -- will see due same source zone, ability prefix and zcc;
    • Kraj's exiled cards by Izzet's gained ability -- will NOT see due diff source zone;
    • Kraj's exiled cards by another's gained ability -- will NOT see due diff source zone;
    • after blink - will NOT see any exiled cards due diff zcc;
  • Kraj's view:
    • own exiled cards by Izzet's gained ability -- will see due same source zone, ability prefix and zcc;
    • own exiled cards by other's gained ability -- will NOT see due diff ability prefix;
    • Izzet's exiled cards -- will NOT see due diff source zone;
    • after blink - will NOT see any exiled cards due diff zcc;

@ssk97
Copy link
Contributor Author

ssk97 commented Aug 29, 2024

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
Copy link
Member

JayDi85 commented Aug 29, 2024

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.

Copy link

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

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

@ssk97
Copy link
Contributor Author

ssk97 commented Aug 29, 2024

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
Copy link
Member

JayDi85 commented Aug 29, 2024

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

@ssk97
Copy link
Contributor Author

ssk97 commented Sep 10, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developers Discussion Discussion about redesign or changes engine
Projects
None yet
Development

No branches or pull requests

2 participants