fix(ini): replay user GameData.ini after SagePatch overrides - #264
Conversation
SagePatch.ini is loaded after Data\INI\GameData with INI_LOAD_OVERWRITE, so its generated defaults silently clobbered values from a user-authored loose Data/INI/GameData.ini (reported: MaxCameraHeight had no effect). Replay the loose file after SagePatch.ini when it exists, restoring user precedence. Installs without a loose GameData.ini are unaffected and the BIG-archived original is never reloaded. Also fix the percent sign in the generated terrain comment being eaten as a printf format specifier, and document the precedence rule in the generated header. Applied to both Generals and Zero Hour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning
|
| Layer / File(s) | Summary |
|---|---|
Reload loose GameData.ini after SagePatch Generals/Code/GameEngine/Source/Common/GameEngine.cpp, GeneralsMD/Code/GameEngine/Source/Common/GameEngine.cpp, docs/WORKLOG/* |
Both game engines reload an existing loose GameData.ini after SagePatch.ini, allowing its values to override SagePatch values. Generated comments and the worklog document the load order and related corrections. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Merge Risk: 🔵 Low · up to d637d
The PR changes GameData precedence so loose user files can override generated settings, but debug builds may still apply a later override and produce different results. The change is otherwise mergeable with explicit owner awareness of this bounded debug-build behavior.
Suggested reviewers: fbraz3
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely describes replaying the user GameData.ini after SagePatch overrides, which is the main change. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.) |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Comment @coderabbitai help to get the list of available commands.
Problem
Values set in a user-authored loose
Data/INI/GameData.iniare silently ignored. Reported symptom: settingMaxCameraHeight = 510.0there had no effect in game.Root cause
GameEngine::init()loads GameData in this order:Data\INI\Default\GameData(BIG-archived stub)Data\INI\GameData— a loose user file shadows the archived copy here, sinceFileSystem::openFile()resolves local before archiveSagePatch.inifrom the user data dir, withINI_LOAD_OVERWRITESagePatch ships a hardcoded
GameDatablock (MaxCameraHeight = 350.0,MinCameraHeight = 100.0,EnforceMaxCameraHeight = No, ...), so its generated defaults overwrite whatever the user authored — with no error or log line. Since SagePatch.ini is recreated on every launch if deleted, there was no way for the standard loose-INI modding channel to win. This also affects mods, which ship loose INI files into the game directory and cannot edit per-user SagePatch files.Fix
After loading
SagePatch.ini, replay the looseData/INI/GameData.iniif one exists locally (guarded byTheLocalFileSystem->doesFileExist(), matching the local-first resolution the load itself uses). Precedence becomes: archived defaults → SagePatch → user file.GameData.iniare unaffected (guard is false).INI_LOAD_OVERWRITE,parseGameDataDefinition()is plain field assignment with nonewOverride()push. Determinism accounting slightly improves: the loose file is CRC'd by theinitSubsystemload, while SagePatch is not, so effective values now match whatxferCRCsaw.Also in this change:
~5% more terraincomment: the%was consumed as afprintfformat specifier, producing~5more terrainin every SagePatch.ini written to date.Known trade-off: a loose
GameData.inimust be a complete copy of the archived file (it shadows, it doesn't merge), so every key in it becomes authoritative — including stock values the user copied without meaning to change. Concretely, stock pinsFramesPerSecondLimit = 30, which reverts SagePatch's 60 FPS default unless the user edits that line too. Keys absent from stock (e.g.TerrainDrawDistanceScale) keep their SagePatch values.Applied to both Generals and Zero Hour. Both
z_generalsandg_generalsbuild and link cleanly onmacos-vulkanagainst currentmain.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
GameData.inisettings now correctly override default values fromSagePatch.iniwhen both files are present.Documentation