Skip to content

Fix NPE when restoring a snapshot taken before a card was created - #11416

Open
cuinhellcat wants to merge 1 commit into
Card-Forge:masterfrom
cuinhellcat:snapshot-restore-npe-fix
Open

Fix NPE when restoring a snapshot taken before a card was created#11416
cuinhellcat wants to merge 1 commit into
Card-Forge:masterfrom
cuinhellcat:snapshot-restore-npe-fix

Conversation

@cuinhellcat

Copy link
Copy Markdown

Problem

GameSnapshot.copyGameState() copies the snapshot's cards back into the running game, but it never removes cards that the running game has and the snapshot does not — tokens, copies and effect cards that were created after the snapshot was taken.

The loop that restores attachments afterwards iterates the current battlefield and looks every card up in the snapshot:

for (Card newCard : toGame.getCardsIn(ZoneType.Battlefield)) {
    Card fromCard = fromGame.findById(newCard.getId());

    if (fromCard.isAttachedToEntity()) {   // NPE: fromCard is null for cards the snapshot never had

For a card the snapshot does not know, findById returns null and the next line throws.

Interestingly assignGameState already anticipates such cards a few lines further down — it prints "Missing card " + c and continues — but by then copyGameState has already thrown.

When it shows up

Not in the current use of snapshots: rolling back a cancelled cast (GameActionUtil.rollbackAbility) creates no new objects, so every card on the battlefield is also in the snapshot. Any restore that reaches back past the creation of a token or a copy throws instead — which is what happens as soon as snapshots are used to step further back than the action in progress.

Fix

Remove those cards from the game as part of the restore. Returning to a state in which they did not exist is exactly what a restore means, and it also gives the "Missing card" branch below nothing left to report.

Notes

  • Built against master; forge-game compiles clean.
  • No test added, per the note in CONTRIBUTING about agent-added tests. I do have a regression test for this (it reproduces the NPE reliably by adding a card after the snapshot and restoring) and am happy to include it if you would like it.
  • This came out of a personal multi-step undo experiment: https://github.com/cuinhellcat/mtg-forge-rewind — the fix is independent of that and useful on its own, so it is offered separately.

AI disclosure

Written with Claude Code (Claude Opus 5), as requested in CONTRIBUTING; the agent is also recorded as co-author on the commit.

GameSnapshot.copyGameState() puts the snapshot's cards back into the
running game, but never removes cards the running game has and the
snapshot does not: tokens, copies and effect cards that came into being
after the snapshot was taken. The loop restoring attachments then walks
the battlefield, looks every card up in the snapshot via findById, and
dereferences the null it gets back for those cards.

Rolling back a cancelled cast does not hit this, because it creates no
new objects, but any restore reaching back past the creation of a token
or a copy throws.

Remove those cards from the game while restoring, which is what going
back to a state in which they did not exist means.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread forge-game/src/main/java/forge/game/GameSnapshot.java
@tool4ever

Copy link
Copy Markdown
Contributor

Hallo 👋

FYI there's already a PR related to your experiment: #10294
but it's probably dead at this point :/
(mostly just need a clean GUI connection with existing Undo)
imo code from Claude is better in quality than other agents, but things get a bit messy if human controller can't understand at least either Java or some software architecture in general to push back if it hallucinates:
then you'd essentially just be our copy-paster and bill payer 😛

@cuinhellcat

Copy link
Copy Markdown
Author

Hallo 👋

FYI there's already a PR related to your experiment: #10294 but it's probably dead at this point :/ (mostly just need a clean GUI connection with existing Undo) imo code from Claude is better in quality than other agents, but things get a bit messy if human controller can't understand at least either Java or some software architecture in general to push back if it hallucinates: then you'd essentially just be our copy-paster and bill payer 😛

I have no problem in being just a bill payer (I have this subscription and I can use some of my contingent for this project.)

As long as copy pasting helps, I will do it.

But I totally understand your point. I will switch to testing, what claude code did and only post here, if tests work on my end... If I cause trouble, I will immediately step back. Just tell me to stop, I will do so.

@tool4ever tool4ever left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Jetz72 Jetz72 added the Experimental Restore Relating to the state-based Undo/Restore feature. label Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Experimental Restore Relating to the state-based Undo/Restore feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants