Implement Multi-Step Undo

[neumannt]

This pull request implements multi-step undo by pressing Ctrl-Z, guarded behind the experimental snapshot flag. It consists of two patches:

1. it fixes the snapshot mechanism to really keep track of all state (and adds a test that explicitly checks that all state is covered). Some state was not covered correctly, e.g., number of card draws, speed, etc.
2. uses the mechanism for multi-step undo, by keeping the last 10 states available in a list and going to the previous state if requested.

Claude Code helped with creating the patch, and was used to check if all state was covered in the snapshot mechanism. I manually tested state reconstruction with complex card states, e.g., planeswalkers. The old code was definitively incorrect/incomplete, and the whole feature is disabled by default, thus it should be an improvement for sure.

[tool4ever]

In general this seems desirable to level up this feature, but it could use some fine tuning:
right now the "Undo" button will almost always be available and that means toggling auto-pass gets much harder to reach

[neumannt]

Hm, do you think it would be better if undo is only available via keyboard shortcut then? That would not clutter the interface. But people would have to know about that shortcut.

[tool4ever]

nah, I think Undo is also pretty essential to have as a direct GUI element for new users

either the days of overloading the cancel button this way need to end or some intuitive way to access both gets added (e.g. for desktop right click still seems available 🤔)

[tool4ever]

I think given that's is still gated by the experimental opt-in + we've now made the dock access more prominent as part of yield improvements this would be ok GUI wise for future iteration. 🤷‍♂️

But I still have some reservations about all those additional setters which are transferring full objects between games. That almost certainly should be avoided or it risks introducing weird bugs of the states bleeding into each other...

Also because I've implemented showing size of undo stack in the meantime this should be aligned.

[neumannt]

I agree that this is a bit brittle. I tried to address that by introducing a test that uses reflection to see if new attributes were introduced, but it is not perfect. And I realized in the meantime that while I do check that for the state, I did not introduce such automatic checks for the card state. That would be needed, too, but that makes the patch even more invasive.
Hm, I am not sure. I thought it would be nice to piggy-back the undo mechanism on the snapshot logic, but I agree that the whole setup is a bit brittle.

[cuinhellcat]

@tool4ever pointed me here from #11416 — I had been building the same feature without knowing this PR existed. Rather than compete with it, here is what I have that might be useful to you.

Two of my PRs were duplicates of yours and are now closed (#11420 foretold, #11422 controller) — your patch covers both.

The GUI part, if you want it

You mentioned Ctrl-Z only, and @tool4ever asked for a direct GUI element. I have one working against the current snapshot code, and it ports to your list of states with almost no change:

• a Rewind submenu in the in-game Game menu, one entry per step
• each entry says where it leads, rebuilt every time the menu opens: 1 action back — back to turn 5, Main 1
• steps with no snapshot behind them are greyed out rather than hidden, so the depth is visible
• step count as a preference (Rewind steps, next to the experimental restore checkbox) instead of a constant

Happy to open it as a PR against your branch, hand over the patch, or just let you take the idea — whichever suits.

One thing you will run into

Restoring the state is not enough on its own: auto-pass immediately carries the game forward again past the position you just went back to. From a live game of mine, four rewind clicks in a row:

Restoring game state with timestamp of :147
Restoring game state with timestamp of :147
Restoring game state with timestamp of :147
Restoring game state with timestamp of :147

Each restore was undone by auto-pass before the player could act, and each one stashed a fresh snapshot at the same spot. What fixed it here: switch off autoPassUntilEndOfTurn on restore — it was a decision about a turn that no longer stands — and skip the auto-pass check once, so the player actually gets the prompt they rewound to. Cheap, and invisible when auto-pass is off.

Gaps still open after your patch

From playing with this rather than reading it, four things your diff does not touch. All are separate PRs already, independent of this one:

• Fix NPE when restoring a snapshot taken before a card was created #11416 — cards the snapshot does not have (tokens, copies, effect cards) are never removed on restore, so the attachment loop dereferences a null. Fatal for any real rewind, invisible for a cancelled cast.
• Clean up the ability when a cast is rolled back via snapshot #11418 — rollbackAbility returns early on the snapshot path and skips the ability cleanup: announced values, targets, castSA, and the frozen stack all survive. Looks like Known Rollback issues #10049 and Canceling casts sometimes freezes the game #8762 from the outside.
• Snapshot cards into the zone they are actually in, not the controller's #11419 — cards are filed under their controller rather than the player whose zone they are in, so a stolen card lands in the wrong graveyard.
• Keep melded permanents intact across a snapshot #11421 — a melded permanent comes back as two, or as one that is both melded and loose on the battlefield.

On the brittleness

Your GameSnapshotCoverageTest is the right answer to the problem I kept hitting the slow way — I found meld, control and foretold one at a time by playing until something looked wrong. A reflection check would have named all three at once. If extending it to card state is what is blocking this PR, I am glad to take a run at that part.

Written with Claude Code, same as your patch; every claim above has a test that fails before the change and passes after.

[tool4ever]

thanks for the summary
imo you don't need to wait for this one-shot PR since it's a bit stale and almost too large anyway 🤷‍♂️
also ideally not much extra tests are written since we should eventually just reuse the Snapshot for running our normal ones, see discussion in #11203

a) but the problem like with most AI PR is clean architecture that makes the copying not pollute all relevant classes too much
b) I like the idea of showing additional info about the saved state and I guess skipping some in the order doesn't matter 🤔
But I'm still hoping we can somehow connect it to the original Undo button instead of more menu work...