Skip to content

unify(common-ini): Move shared INI loaders to Core#2395

Open
OmarAglan wants to merge 4 commits intoTheSuperHackers:mainfrom
OmarAglan:unify/common-ini
Open

unify(common-ini): Move shared INI loaders to Core#2395
OmarAglan wants to merge 4 commits intoTheSuperHackers:mainfrom
OmarAglan:unify/common-ini

Conversation

@OmarAglan
Copy link

@OmarAglan OmarAglan commented Mar 4, 2026

Merge with Rebase

This change merges Common INI loader code and moves shared INI loader files to Core.

Most INI files were almost identical already, so this is primarily a mechanical unification:

  • moved 24 files from GeneralsMD/Code/GameEngine/Source/Common/INI/* to Core/GameEngine/Source/Common/INI/*
  • deleted duplicate copies from Generals/Code/GameEngine/Source/Common/INI/*
  • commented file entries in Generals and GeneralsMD CMakeLists
  • uncommented matching entries in Core CMakeLists

Follow-up fix included:

  • keep MultiplayerStartingMoneyChoice parsing Zero Hour-only (RTS_ZEROHOUR) in INIMultiplayer.cpp to preserve
    Generals behavior and avoid unresolved external Errors.

No intended behavior change for existing Generals paths.

@greptile-apps
Copy link

greptile-apps bot commented Mar 4, 2026

Greptile Summary

This PR unifies 24 shared INI loader files by moving them from GeneralsMD/Code/GameEngine/Source/Common/INI/ to Core/GameEngine/Source/Common/INI/ and deleting the duplicate copies from Generals/Code/GameEngine/Source/Common/INI/. The three CMakeLists files are updated symmetrically: entries are uncommented in Core and commented out in Generals and GeneralsMD.

Key changes:

  • 24 INI .cpp files moved to Core with no functional modifications — a clean mechanical unification.
  • Core/GameEngine/Source/Common/INI/INIMultiplayer.cpp is a newly synthesized file that merges both game variants. It includes a full implementation of parseMultiplayerStartingMoneyChoiceDefinition, which was previously an intentional empty stub in the Generals build.
  • Generals/Code/GameEngine/Source/Common/MultiplayerSettings.cpp now implements addStartingMoneyChoice (previously only in GeneralsMD) and correctly initializes m_gotDefaultStartingMoney = false in the constructor, fixing a previously uninitialized member field.
  • scripts/cpp/unify_move_files.py records the 24 migrated files as commented-out history entries.

Note on stated intent vs. implementation: The PR description says the follow-up fix was to "keep MultiplayerStartingMoneyChoice parsing Zero Hour-only (RTS_ZEROHOUR) to preserve Generals behavior", but the chosen approach instead implements addStartingMoneyChoice in the Generals build, enabling the full parse path for both targets. This is a functional behavior change from the previous empty-stub design — Generals' MultiplayerStartingMoneyChoice INI blocks (if any exist in the data files) will now be processed and stored. This should be verified against the Generals data assets to confirm it is intentional and benign.

Confidence Score: 3/5

  • Safe to merge mechanically, but verify whether Generals data files contain MultiplayerStartingMoneyChoice blocks before landing
  • The CMakeLists wiring and file removals are correct and symmetric. The linker fix (adding addStartingMoneyChoice to Generals) is sound and resolves the unresolved-external problem. However, the approach diverges from the PR description's stated intent to keep the feature Zero Hour-only, and the previous inline comments about indentation/column alignment in INIMultiplayer.cpp remain unresolved. The uninitialized-member fix is a genuine improvement. Score reflects that the core goal is achieved but there is an undocumented behavior change for Generals and outstanding style issues.
  • Core/GameEngine/Source/Common/INI/INIMultiplayer.cpp — verify Generals data file behavior; Generals/Code/GameEngine/Source/Common/MultiplayerSettings.cpp — review the new addStartingMoneyChoice implementation matches GeneralsMD semantics

Important Files Changed

Filename Overview
Core/GameEngine/Source/Common/INI/INIMultiplayer.cpp New Core file replacing both Generals and GeneralsMD stubs; implements all three multiplayer parsers, but parseMultiplayerStartingMoneyChoiceDefinition is now active for Generals (no #ifdef RTS_ZEROHOUR guard), changing behavior relative to the old empty-stub approach; column alignment in startingMoneyFieldParseTable also not aligned to codebase convention.
Generals/Code/GameEngine/Source/Common/MultiplayerSettings.cpp Adds addStartingMoneyChoice implementation and initializes previously-uninitialized m_gotDefaultStartingMoney = false in the constructor; both changes are correct and resolve the linker error described in the PR.
Core/GameEngine/CMakeLists.txt Uncomments 24 INI source file entries in the Core build, correctly mirroring the removals in Generals and GeneralsMD CMakeLists files.
Generals/Code/GameEngine/CMakeLists.txt Comments out 24 INI source file entries previously compiled from the Generals tree, deferring to the new Core versions; change is symmetric with Core additions.
GeneralsMD/Code/GameEngine/CMakeLists.txt Comments out the same 24 INI source entries in the Zero Hour build to use the unified Core versions instead; straightforward and correct.
scripts/cpp/unify_move_files.py Adds 24 commented-out unify_file(...) entries to document the files that were migrated in this PR; purely historical record-keeping, no functional impact.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph Before["Before this PR"]
        G_INI["Generals/…/INI/INIMultiplayer.cpp\n(parseMultiplayerStartingMoneyChoice = empty stub)"]
        MD_INI["GeneralsMD/…/INI/INIMultiplayer.cpp\n(full implementation)"]
        G_MP["Generals/…/MultiplayerSettings.cpp\n(NO addStartingMoneyChoice impl,\nm_gotDefaultStartingMoney uninitialised)"]
        MD_MP["GeneralsMD/…/MultiplayerSettings.cpp\n(addStartingMoneyChoice impl)"]
    end

    subgraph After["After this PR"]
        CORE_INI["Core/…/INI/INIMultiplayer.cpp ✨\n(full implementation for BOTH builds)"]
        G_MP2["Generals/…/MultiplayerSettings.cpp ✨\n(addStartingMoneyChoice added,\nm_gotDefaultStartingMoney = false)"]
        MD_MP2["GeneralsMD/…/MultiplayerSettings.cpp\n(unchanged)"]
    end

    subgraph CMake["CMakeLists changes"]
        C_CMAKE["Core/CMakeLists.txt — uncommented 24 INI files"]
        G_CMAKE["Generals/CMakeLists.txt — commented out 24 INI files"]
        MD_CMAKE["GeneralsMD/CMakeLists.txt — commented out 24 INI files"]
    end

    Before --> After
    CORE_INI -->|"compiled into"| G_MP2
    CORE_INI -->|"compiled into"| MD_MP2
    C_CMAKE -.->|"enables"| CORE_INI
    G_CMAKE -.->|"disables old"| G_INI
    MD_CMAKE -.->|"disables old"| MD_INI
Loading

Comments Outside Diff (1)

  1. Core/GameEngine/Source/Common/INI/INIMultiplayer.cpp, line 98-109 (link)

    Behavior change for Generals contradicts PR description

    The PR description states the follow-up fix was to "keep MultiplayerStartingMoneyChoice parsing Zero Hour-only (RTS_ZEROHOUR) ... to preserve Generals behavior", but the actual implementation does the opposite: parseMultiplayerStartingMoneyChoiceDefinition is now a full, active implementation compiled into both Generals and Zero Hour builds.

    Previously, the Generals-specific stub (Generals/Code/GameEngine/Source/Common/INI/INIMultiplayer.cpp) was an intentional no-op:

    void INI::parseMultiplayerStartingMoneyChoiceDefinition( INI* ini )
    {
    }

    Now, if Generals' multiplayer.ini contains any MultiplayerStartingMoneyChoice blocks, they will be parsed, stored in m_startingMoneyList, and potentially set m_defaultStartingMoney — a runtime behavior change for Generals. Worth confirming whether Generals' data files actually contain such blocks; if they do, this is not a "no intended behavior change" situation.

Last reviewed commit: 833457b

@greptile-apps
Copy link

greptile-apps bot commented Mar 4, 2026

Additional Comments (1)

Core/GameEngine/Source/Common/INI/INIMultiplayer.cpp, line 3
GPL header copyright year out of date

This file is newly created in this PR (2026), but the GPL license header still carries 2025 as the copyright year. Per the project's convention of keeping license headers current, this should be updated to the current year.

**	Copyright 2026 Electronic Arts Inc.

Context Used: Rule from dashboard - What: Flag newly created code comments that reference dates prior to the current year (2026).

Why: ... (source)

@OmarAglan OmarAglan requested a review from Skyaero42 March 5, 2026 07:17
@Skyaero42 Skyaero42 changed the title unify(common-ini): move shared INI loaders to Core unify(common-ini): Move shared INI loaders to Core Mar 11, 2026
@github-actions
Copy link

⚠️ Title/Commit Validation Failed

Invalid commit messages:

  • bugfix(common-ini): enable starting money choice parsing in Generals
  • unify(common-ini): move shared INI loaders to Core
  • fix(common-ini): keep starting money parser zh-only
    PR titles and commit messages must follow conventional commits format:
type: Description
type(scope): Description

Allowed types: bugfix, build, chore, ci, docs, fix, feat, perf, refactor, revert, style, test, tweak, unify

See CONTRIBUTING.md for details.

@Skyaero42
Copy link

With unifying we usually do two steps:

  • unify code between Generals and Zero hour
  • merge/move the files to core.

At the moment it is hard to review INIMultiplayer.cpp and MultiPlayerSettings.cpp and whether the unification is done correctly. I would suggest:

  • Open a new PR in which INIMultiplayer.cpp and MultiPlayerSettings.cpp are unified across the two games
  • Once that has been merged into main, rebase this PR and ensure that all files are just move/delete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants