Snapshot cards into the zone they are actually in, not the controller's - #11419
Snapshot cards into the zone they are actually in, not the controller's#11419cuinhellcat wants to merge 1 commit into
Conversation
copyGameState placed every card in a zone belonging to its controller. That only matches reality on the battlefield: a graveyard, hand or library belongs to the card's owner. A card whose control has changed — a stolen creature, or one returned to play under someone else's control — was therefore filed under the wrong player, and restoring the snapshot moved it into that player's graveyard for good. Take the player from the zone the card is in, which is the controller on the battlefield and the owner everywhere else, and fall back to the controller for the stack, which has no player. Copies made for a stored game likewise keep their original owner instead of inheriting the controller. Found while looking into Card-Forge#9297, whose stack trace comes from the same mismatch: back when setCardInCopiedGame still inserted at the card's index, filing it under the wrong player inserted past the end of that player's shorter graveyard. That insert is gone from master and the exception no longer reproduces there, so this does not claim to close that report — only to fix the misplacement behind it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| // control has changed under the wrong player, so a stolen creature that died | ||
| // reappears in the thief's graveyard when the snapshot is used. | ||
| Player fromZonePlayer = fromCard.getZone().getPlayer(); | ||
| Player toPlayer = findBy(toGame, fromZonePlayer != null ? fromZonePlayer : fromCard.getController()); |
There was a problem hiding this comment.
technically fromCard.getZone().getPlayer() == fromCard.getController() shouldn't be different either way, since it falls back to owner
are you saying it's somehow not getting updated on LTB case?
There was a problem hiding this comment.
You are right: they cannot differ.
My reasoning was a stolen creature dying. The temporary controller is only cleared while the card is in play — doLoseControl returns early otherwise — so I expected a card sitting in its owner's graveyard while still naming the thief as controller. Testing that says otherwise:
same object? false id before=1 id after=1
after dying: zone=Graveyard zonePlayer=p2 controller=p2 owner=p2
Moving to the graveyard hands back a new card object, and a fresh object has no temporary controllers, so the controller falls back to the owner — exactly as you said.
The reproduction I had only "worked" because it called setController on a card already in a graveyard, which the engine never does. So this PR fixes nothing real. Closing it.
Problem
copyGameStatedecides which player's zone a card goes into by asking the card for its controller:That is only right on the battlefield. A graveyard, hand or library belongs to the card's owner. When control has changed — a stolen creature, or a card returned to play under someone else's control — the card is filed under the wrong player, and restoring the snapshot moves it into that player's graveyard for good. Cards silently change hands across an undo.
Fix
Take the player from the zone the card is actually in: the controller on the battlefield, the owner everywhere else, with a fallback to the controller for the stack, which has no player. Card copies made for a stored game likewise keep their original owner rather than inheriting the controller.
Reproduction
Put nine cards in one player's graveyard plus a tenth whose controller is the opponent, snapshot and restore: before the change the tenth ends up in the opponent's graveyard, after it stays where it belongs.
Relationship to #9297
This came out of looking into #9297, and I want to be precise about what it does and does not do. That report's stack trace is the same mismatch seen from a different angle:
The card kept the index it had in its owner's graveyard and was inserted at that index into the controller's shorter one. But
setCardInCopiedGameno longer inserts by index on master, and I could not get master to throw that exception. So this PR does not claim to close #9297 — it fixes the misplacement the crash grew out of, which is a bug on its own.If you would also like
Zone.addto clamp an out-of-range index defensively (it already guards the empty-zone case a few lines up), that is a two-line addition and I am happy to add it.Notes
AI disclosure
Written with Claude Code (Claude Opus 5), as requested in CONTRIBUTING; the agent is also recorded as co-author on the commit.