bugfix(savegame): Restore the ESC menu after a failed load

[bobtista]

• Fixes ESC menu can remain disabled after a save game fails to load #3053

GameStateMap::xfer sets TheGameLogic->setLoadingSave(TRUE) and clears it only at the end of a successful load. When the load throws, GameState::loadGame takes its error path and resets the engine, but nothing clears the flag. canOpenQuitMenu() checks isLoadingSave(), so the ESC menu stays disabled in every game started afterwards.

Now the error path in GameState::loadGame clears the flag. The save format does not change.

Todo:

• Load a save with an invalid CHUNK_GameStateMap version: isLoadingSave() is 1 after the failed load before the fix, 0 after
• Test ESC in a game started after the failed load
• Replicate to Generals

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 862cfdb6-6696-48fb-a612-4558e62409cf

📥 Commits

Reviewing files that changed from the base of the PR and between ea71544 and edd87ae.

📒 Files selected for processing (1)

• Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

Walkthrough

When save-game loading fails, loadGame clears the loading-save state before returning SC_INVALID_DATA. Existing game-data cleanup and engine reset remain unchanged.

Changes

Save-load failure cleanup

Layer / File(s)	Summary
Clear loading state on failure GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp, Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp	Both loadGame error paths clear the loading-save state before returning SC_INVALID_DATA. Existing cleanup behavior remains unchanged.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to edd87

The failed-load cleanup restores the loading state checked by the quit menu in both variants. No remaining merge-blocking risk is established.

Pre-merge checks · 3 · 1

Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check		The PR also adds the same change to Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp. Issue #3053 applies to Generals: Zero Hour, and the current PR description marks replication…	Remove the unrelated change from the Generals path, or link a directly applicable issue and update the PR scope to include that target.

Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check		Issue #3053 requires failed CHUNK_GameStateMap loads to restore ESC-menu behavior in later single-player games. The PR clears TheGameLogic->setLoadingSave(FALSE) in the GameState::loadGame failu…
Title check		The title clearly identifies the save-game bug and the restored ESC menu behavior after a failed load.
Description check		The description directly explains the failed-load flag bug, the code change, the affected behavior, and the verification status.

Full details: Out of Scope Changes check

Explanation

The PR also adds the same change to Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp. Issue #3053 applies to Generals: Zero Hour, and the current PR description marks replication to Generals as unchecked. The linked issue provides no requirement for the separate Generals code path.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Caball009]

AFAICT the fix works.

[Caball009]

Just wondering, does it matter if this is done before or after the other stuff in this branch?

[bobtista]

No it doesn't matter. Only the quit menu checks this flag, and clearGameData and the engine reset do not read or change it.