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

xmage handling of Auras differs from the CR and is incompatible with rollbacks #9583

Open
awjackson opened this issue Sep 27, 2022 · 1 comment
Assignees
Labels
bug Bugs and errors

Comments

@awjackson
Copy link
Contributor

The following are the Comprehensive Rules for the enchant keyword: (2022/09/08)

702.5. Enchant
702.5a Enchant is a static ability, written “Enchant [object or player].” The enchant ability restricts what an Aura spell can target and what an Aura can enchant.
702.5b For more information about Auras, see rule 303, “Enchantments.”
702.5c If an Aura has multiple instances of enchant, all of them apply. The Aura’s target must follow the restrictions from all the instances of enchant. The Aura can enchant only objects or players that match all of its enchant abilities.

In xmage, the EnchantAbility class is completely cosmetic. Every place where the engine wants to know whether an Aura can become attached or remain attached to an object, it does so by extracting a Target from the Aura's SpellAbility. Besides being inaccurate to the CR, this has a number of practical consequences:

  • Some spells have multiple SpellAbilitys (for alternate casting methods like bestow), and tokens do not normally have one at all (because they are never cast). GameImpl.checkStateBasedActions actually has to do a whole lot of ugly blind groping to find the correct ability to extract a Target from.
  • If a continuous effect wants to turn a permanent into an Aura, or replace an Aura's enchant ability with a different one, it can't simply add or replace the EnchantAbility, because that ability doesn't do anything except display text. It has to modify the permanent's SpellAbility. For reasons that I admit I don't fully understand, this modification is not stable across GameState restores (which include but are by no means limited to player-requested rollbacks). One symptom of this is that Animate Dead and Dance of the Dead often spontaneously fall off for no apparent reason (Dance of the Dead rollback bug #7228, Dance of the Dead moved to GY seemingly for no reason #7794, Animate Dead self sacrifice bug #9420). Via debugging I have confirmed that this is caused by checkStateBasedActions checking the permanent the Aura is attached to against the spell's original "card in a graveyard" target.
  • The only reason the previous bug doesn't affect a lot more cards is that if checkStateBasedActions can't find a Target at all it logs an error in the server log but allows the Aura to remain attached to whatever it's currently attached to. There are a lot more cards in Magic that start as non-Aura permanents with no target and turn into Auras than that start as one kind of Aura and turn into a different one.
@xenohedron
Copy link
Contributor

xenohedron commented Jul 15, 2023

As part of this rework, should investigate CanBeEnchantedByPredicate and PermanentCanBeAttachedToPredicate edit: done in #10625, ended up not being very relevant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugs and errors
Projects
No open projects
Game engine improves
  
High priority
Development

No branches or pull requests

3 participants