Move a cast spell to the graveyard when it's cast, not after it resolves - #416
Open
DragosIonita23 wants to merge 1 commit into
Open
Conversation
fx.Spell kept a cast spell in its caster's hand for the spell's entire resolution: the HAND -> GRAVEYARD move was scheduled via two layers of ScheduleAfter, both deferred until after the spell's own effect (and anything that effect triggered) had already run. Any nested effect that inspected hand/graveyard state during that window saw stale data: - Soulswap putting Jagila into play let Jagila's "opponent discards 3 at random" discard Soulswap itself out of the caster's hand. - Soulswap/Miraculous Rebirth putting Phal Eega into play couldn't have Phal Eega recur the spell that just cast it, because that spell wasn't in the graveyard yet. - Pincer Scarab's power (+2000 per card in the opponent's hand) still counted a spell the opponent had just cast, keeping it out of reach of effects like Apocalypse Vise that destroy by total power. Fixed by moving the card to the graveyard synchronously, as the first thing that happens on SpellCast, before the card's own effect body (registered later in the same c.Use chain) runs. Cards that redirect the final resting zone still do so from SpellResolved, same as before: - fx.Charger now relocates from the graveyard instead of hand. - Super Terradragon Bailas Gale (dm08/earth_dragon.go) used to prevent the graveyard move by cancelling the whole SpellResolved context; it now explicitly moves the spell back from the graveyard instead. Audited the rest of the card set for code that hard-coded the old "still in hand while resolving" assumption (self-referential MoveCard calls, `!= card.ID` exclusion filters paired with hand selections, and hand-size arithmetic computed during a spell's own resolution) and fixed two real regressions: - Boomerang Comet (dm03/spells.go) moved itself from hand to the mana zone with a hand-rolled MoveCard instead of fx.Charger; that fails silently since the card already left hand, so it now moves from the graveyard. - Mega Detonator (dm04/spells.go) computed its max discard count as (hand size - 1) to exclude itself; that now undercounts the remaining hand by one, so the subtraction and the now-redundant exclusion filter are removed. A few other cards had the same now-redundant `!= card.ID` guar (Enigmatic Cascade, Propeller Mutant) which were harmless but whose comments were actively wrong about current behavior; updated t accuracy. Left fx.SwapHandAndMana's defensive exclusion in place since it has its own unit test asserting that contract directly. Also updated Morbid Medicine's "empty graveyard" test: it now sees itself in its own graveyard scan (as a spell, not a creature, so still not selectable), which means the return-prompt opens wit to pick instead of being skipped - a minor, correct side effect. Added regression coverage: Soulswap (self-discard via Jagila, self- recursion via Phal Eega, both from hand and from a broken shie Miraculous Rebirth (same two interactions with Emeral and Phal Eega), Pincer Scarab vs. Apocalypse Vise, Super Terradragon Bailas Ga prior coverage), Boomerang Comet, and Mega Detonator. Each new test was verified to fail against the pre-fix code and pass against the Full suite green under `go test ./... -race`, and `tests/cards to rule out flakiness from the random-discard interactions exercised here (Jagila, Ghost Touch, Propeller Mutant).
Contributor
Author
|
@sindreslungaard This one is a bigger one, but it solves older problems with spell casting and spell resolution various interactions with other cards with special triggered effects, e.g. a spell should not be considered still in hand after casting. I feel it's an important one, and please comment here if you feel I missed some spells / specific interactions in this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Move a cast spell to the graveyard when it's cast, not after it resolves
fx.Spell kept a cast spell in its caster's hand for the spell's entire resolution: the HAND -> GRAVEYARD move was scheduled via two layers of ScheduleAfter, both deferred until after the spell's own effect (and anything that effect triggered) had already run. Any nested effect that inspected hand/graveyard state during that window saw stale data:
Fixed by moving the card to the graveyard synchronously, as the first thing that happens on SpellCast, before the card's own effect body (registered later in the same c.Use chain) runs. Cards that redirect the final resting zone still do so from SpellResolved, same as before:
Audited the rest of the card set for code that hard-coded the old "still in hand while resolving" assumption (self-referential MoveCard calls,
!= card.IDexclusion filters paired with hand selections, and hand-size arithmetic computed during a spell's own resolution) and fixed two real regressions:A few other cards had the same now-redundant
!= card.IDguar (Enigmatic Cascade, Propeller Mutant) which were harmless but whose comments were actively wrong about current behavior; updated t accuracy. Left fx.SwapHandAndMana's defensive exclusion in place since it has its own unit test asserting that contract directly.Also updated Morbid Medicine's "empty graveyard" test: it now sees itself in its own graveyard scan (as a spell, not a creature, so still not selectable), which means the return-prompt opens wit to pick instead of being skipped - a minor, correct side effect.
Added regression coverage: Soulswap (self-discard via Jagila, self- recursion via Phal Eega, both from hand and from a broken shie Miraculous Rebirth (same two interactions with Emeral and Phal Eega), Pincer Scarab vs. Apocalypse Vise, Super Terradragon Bailas Ga prior coverage), Boomerang Comet, and Mega Detonator. Each new test was verified to fail against the pre-fix code and pass against the
Full suite green under
go test ./... -race, and `tests/cards to rule out flakiness from the random-discard interactions exercised here (Jagila, Ghost Touch, Propeller Mutant).📝 Summary
Provide a brief summary of your changes and the motivation behind them.
🎴 New Cards Added
🐞 Bugs Fixed
🔧 Other Changes
✅ Checklist
Please confirm the following before submitting your PR:
./sim/tests)📸 Screenshots (if applicable)
If there are any visual changes to the frontend, please include some screenshots or screen recordings of it