magefree / mage

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

[VOW] Disturb-related bugs with Dawnhart Geist and Brine Comber #9686

Closed sprangg closed 1 year ago

sprangg commented 1 year ago

Tested [[Dawnhart Geist]] effect with a couple of disturb auras and it didn't trigger. It triggers ok from normally cast enchantments.

[[Brine Comber]] and its aura side don't trigger from disturb auras.

github-actions[bot] commented 1 year ago

Dawnhart Geist - (Gatherer) (Scryfall) (EDHREC)

{1}{W} Creature — Spirit Warlock 1/3 Whenever you cast an enchantment spell, you gain 2 life.

Brine Comber // Brinebound Gift - (Gatherer) (Scryfall) (EDHREC)

{1}{W}{U} Creature — Spirit 1/1 Whenever Brine Comber enters the battlefield or becomes the target of an Aura spell, create a 1/1 white Spirit creature token with flying. Disturb {W}{U} (You may cast this card from your graveyard transformed for its disturb cost.) :arrows_counterclockwise: Enchantment — Aura Enchant creature Whenever Brinebound Gift enters the battlefield or enchanted creature becomes the target of an Aura spell, create a 1/1 white Spirit creature token with flying. If Brinebound Gift would be put into a graveyard from anywhere, exile it instead.

awjackson commented 1 year ago

This is caused by the way we implement transforming spells partially as a continuous effect--it's split into transformCardSpellStatic which is not a continuous effect and which replaces the name, the supertypes and the abilities, and transformCardSpellDynamic which is a continuous effect and which replaces the color, types, subtypes, and replaces the abilities again. The problem is that the continuous effect is not created until the very end of the spell casting (after super.activate() returns) and that is too late for various trigger to see that the spell is an Enchantment Aura and not a creature.

@JayDi85 @weirddan455 do you know why transforming spells is implemented in this strange half-and-half way? I don't see why it needs a continuous effect at all--if we can change the name and the supertype statically surely there's no problem with changing the other characteristics that way? The Card field of a Spell is a copy, so it's not like we're permanently modifying the original card.

weirddan455 commented 1 year ago

I'm not sure. IIRC I put a PR in to implement Disturb but a lot of it got re-written by other devs before it got merged. I believe the effects you mentioned have been around longer than that (before I started contributing to the project) and were used for cards like [[Delver of Secrets]].

It's possible we need to revisit how the Disturb mechanic is implemented since it gets transformed as it's cast rather than after it's already on the battlefield like the older transform cards were. Maybe @theelk801 has some insight?

github-actions[bot] commented 1 year ago

Delver of Secrets // Insectile Aberration - (Gatherer) (Scryfall) (EDHREC)

{U} Creature — Human Wizard 1/1 At the beginning of your upkeep, look at the top card of your library. You may reveal that card. If an instant or sorcery card is revealed this way, transform Delver of Secrets. :arrows_counterclockwise: Creature — Human Insect 3/2 Flying

awjackson commented 1 year ago

The methods I named are only used by Disturb. Transforming permanents use a completely different set of methods

weirddan455 commented 1 year ago

Oh I guess I'm mis-remembering. It's been a while since I looked at that code. I'll take a look later.

weirddan455 commented 1 year ago

There was discussion on this in #8201. It sounds like the real solution to this is to refactor Disturb to work like MDFCs do.

awjackson commented 1 year ago

We can't use the MDFC system as-is for TDFCs because the permanents would change UUIDs each time they transform and that really, really can't happen. All the bugs and workarounds for MDFCs boil down to the fact that they change UUIDs when they cross zones and the xmage engine wasn't originally designed with that in mind. Having objects that change UUIDs while remaining in the same zone is on a whole other level of "the engine wasn't designed for that".