unify(map): Merge and move GameLogic map headers and implementations to Core

[OmarAglan]

Merge with Rebase

• Relates to Merge Generals and Zero Hour code into one #555

This merges and moves MapReaderWriterInfo.h, PolygonTrigger, SidesList, and TerrainLogic together with their matching headers.

Target conditions preserve retail compatibility while sharing isolated Zero Hour improvements that do not affect the Generals CRC path:

• Retail-compatible Generals continues writing PolygonTriggers version 3 without layer names and keeps malformed triggers.
• Zero Hour and non-retail Generals use version 4 with layer names and discard triggers with fewer than two points.
• All targets use the existing version-aware player-data parser and 2048-team assertion; version 1 player data follows the legacy parsing path.
• The existing crater declaration and implementation are available to both games; this pull request adds no new caller in Generals.

Testing

• git diff --check
• Generals and Zero Hour games build
• Generals and Zero Hour WorldBuilders build

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Unify map: merge GameLogic map implementations

✨ Enhancement 🕐 20-40 Minutes

AI Description

• Unify Generals and Zero Hour map sources using product-gated compilation.
• Preserve per-game serialization formats and runtime behavior (no functional changes intended).
• Bring ZH-only fields/limits into shared implementations behind RTS_ZEROHOUR guards.

Diagram

graph TD
A["Map I/O (DataChunks)"] --> B["PolygonTrigger"] --> C{"Product flags"}
A --> D["SidesList"] --> C
A --> E["TerrainLogic"] --> C
C -->|"RTS_GENERALS"| F["Generals format/limits"]
C -->|"RTS_ZEROHOUR"| G["ZH fields/features"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep separate Generals/ZH sources (no unification)

• ➕ Zero risk of accidental cross-product behavior bleed
• ➕ Easier to reason about per-product code paths without #if clutter
• ➖ Ongoing duplication and drift between products
• ➖ Harder to do future refactors/moves of map code

2. Extract shared core into common module + product-specific adapters

• ➕ Minimizes preprocessor branching in core logic
• ➕ Clear separation of shared vs product-specific features (e.g., layerName, crater)
• ➖ Larger change surface area (file moves, new interfaces)
• ➖ More build-system churn; higher short-term risk than this PR’s scope

Recommendation: Given the stated goal (behavior preservation, no moves) the current approach—unifying implementations via RTS_GENERALS/RTS_ZEROHOUR guards—is appropriate and low-disruption. A follow-up PR could reduce #if density by extracting a shared core and isolating product-specific extensions, but that would be a larger architectural step than this merge-focused change.

Files changed (6) +132 / -9

Refactor (6) +132 / -9

PolygonTrigger.cppUnify polygon trigger serialization with ZH-only fields and v4 chunk +32/-0

Unify polygon trigger serialization with ZH-only fields and v4 chunk

• Adds RTS_ZEROHOUR-gated members initialization, reads/writes the ZH layer name when triggers chunk version supports it, and deletes invalid (<2 points) triggers for ZH. Switches chunk version selection to v3 for Generals and v4 for Zero Hour while keeping one implementation.

Generals/Code/GameEngine/Source/GameLogic/Map/PolygonTrigger.cpp

SidesList.cppGate ZH player-name chunk versions and team-count assertion by product +20/-0

Gate ZH player-name chunk versions and team-count assertion by product

• Introduces RTS_ZEROHOUR-only constants and parsing for optional dict payloads in the players data chunk. Splits the team allocation assertion threshold (1024 for Generals, 2048 for ZH) to match product behavior.

Generals/Code/GameEngine/Source/GameLogic/Map/SidesList.cpp

TerrainLogic.cppAdd ZH-only crater creation and align minor comment text +52/-1

Add ZH-only crater creation and align minor comment text

• Fixes a comment typo and adds the Zero Hour-only createCraterInTerrain implementation behind RTS_ZEROHOUR, enabling unified source without changing Generals behavior.

Generals/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp

PolygonTrigger.cppMirror unified polygon trigger logic with product-gated chunk versioning +16/-0

Mirror unified polygon trigger logic with product-gated chunk versioning

• Wraps ZH-only fields (render/selection flags, layer name IO, invalid trigger cleanup) in RTS_ZEROHOUR and selects the appropriate triggers chunk version (v3 Generals, v4 ZH) under one codepath.

GeneralsMD/Code/GameEngine/Source/GameLogic/Map/PolygonTrigger.cpp

SidesList.cppWrap ZH-only player-name chunk versions and preserve per-product team limits +10/-0

Wrap ZH-only player-name chunk versions and preserve per-product team limits

• Scopes player-name chunk version constants and optional dict reads to RTS_ZEROHOUR builds. Applies product-specific team-count assertion thresholds to keep Generals and ZH behavior consistent within unified sources.

GeneralsMD/Code/GameEngine/Source/GameLogic/Map/SidesList.cpp

TerrainLogic.cppEnsure crater creation stays ZH-only via RTS_ZEROHOUR guard +2/-8

Ensure crater creation stays ZH-only via RTS_ZEROHOUR guard

• Adjusts preprocessor structure so createCraterInTerrain is compiled only for Zero Hour, matching the unified-source strategy without introducing behavior in Generals builds.

GeneralsMD/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (2) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Retail parser reads v2 dictionaries 📎 Requirement gap ≡ Correctness

Description

ParsePlayersDataChunk unconditionally enables the Zero Hour version-2 dictionary layout for
retail-compatible Generals instead of retaining its legacy parser. Version-2 chunks will therefore
consume an extra integer and dictionaries on a target whose original parsing behavior must remain
unchanged.

Code

Generals/Code/GameEngine/Source/GameLogic/Map/SidesList.cpp[R437-440]

+	Int readDicts = 0;
+	if (info->version >= K_PLAYERS_NAMES_FOR_SCRIPTS_VERSION_2) {
+		readDicts = file.readInt();
+	}

Evidence

Rules 1 and 2 require preserving each game's original target-specific functionality. The changed
Generals implementation defines version 2 and reads its additional readDicts field without the
retail-compatibility guard used elsewhere in this PR.

Unify Generals and Zero Hour in One Codebase
Build Two Distinct Game Executables
Generals/Code/GameEngine/Source/GameLogic/Map/SidesList.cpp[432-448]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Retail-compatible Generals now uses the Zero Hour version-2 player-data parser, changing required target-specific behavior.
## Issue Context
Keep the legacy parser for `RTS_GENERALS && RETAIL_COMPATIBLE_CRC`; enable the version-2 dictionary format only for Zero Hour and non-retail Generals.
## Fix Focus Areas
- Generals/Code/GameEngine/Source/GameLogic/Map/SidesList.cpp[432-448]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Retail team limit changed 📎 Requirement gap ≡ Correctness

Description

The Generals team-count assertion is raised from the legacy 1024 limit to Zero Hour's 2048 limit
without a retail-compatibility guard. This changes required retail-compatible Generals behavior
rather than limiting the improvement to Zero Hour and non-retail builds.

Code

Generals/Code/GameEngine/Source/GameLogic/Map/SidesList.cpp[1134]

+	DEBUG_ASSERTCRASH(m_numTeams < 2048, ("%d teams have been allocated (so far). This seems excessive.", m_numTeams ));

Evidence

Rules 1 and 2 require game-specific original functionality to survive unification. The modified
Generals line now always asserts at 2048, and the file contains no RETAIL_COMPATIBLE_CRC guard to
preserve the legacy 1024 threshold.

Unify Generals and Zero Hour in One Codebase
Build Two Distinct Game Executables
Generals/Code/GameEngine/Source/GameLogic/Map/SidesList.cpp[1134-1134]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Retail-compatible Generals no longer retains its original 1024-team assertion.
## Issue Context
Select the 1024 assertion for `RTS_GENERALS && RETAIL_COMPATIBLE_CRC` and retain the 2048 assertion for Zero Hour and non-retail Generals.
## Fix Focus Areas
- Generals/Code/GameEngine/Source/GameLogic/Map/SidesList.cpp[1134-1134]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Crater method undeclared 🐞 Bug ⚙ Maintainability

Description

Generals' TerrainLogic.cpp now defines TerrainLogic::createCraterInTerrain() under RTS_ZEROHOUR, but
the Generals TerrainLogic class declaration has no such member, so compiling Generals sources with
RTS_ZEROHOUR would fail with a missing-member error.

Code

Generals/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp[R2873-2876]

+void TerrainLogic::createCraterInTerrain(Object *obj)
+{
+	if (obj->getGeometryInfo().getIsSmall())
+		return;

Evidence

The PR adds the createCraterInTerrain definition to the Generals source under RTS_ZEROHOUR, but
the Generals header does not declare the method at all; the Zero Hour header shows that this member
is expected to exist in ZH builds.

Generals/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp[2869-2918]
Generals/Code/GameEngine/Include/GameLogic/TerrainLogic.h[301-316]
GeneralsMD/Code/GameEngine/Include/GameLogic/TerrainLogic.h[314-316]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Generals/.../TerrainLogic.cpp` adds a definition for `TerrainLogic::createCraterInTerrain()` under `#if RTS_ZEROHOUR`, but the Generals header `Generals/.../TerrainLogic.h` does not declare this member function. If the Generals source set is ever compiled with `RTS_ZEROHOUR=1` (a plausible step given the PR’s unification direction), this will not compile.
### Issue Context
- The definition exists in the Generals source file under `#if RTS_ZEROHOUR`.
- The Generals `TerrainLogic` class declaration only exposes `flattenTerrain(...)` and does not mention `createCraterInTerrain(...)`.
- Zero Hour’s header *does* declare `createCraterInTerrain(...)`, showing the intended API surface.
### Fix Focus Areas
- Generals/Code/GameEngine/Include/GameLogic/TerrainLogic.h[306-316]
- Generals/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp[2869-2918]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. ZH trigger APIs missing 🐞 Bug ⚙ Maintainability

Description

Generals' PolygonTrigger.cpp now references Zero Hour-only members/APIs (layer name,
selection/render flags) under RTS_ZEROHOUR, but the Generals PolygonTrigger class definition does
not declare these, so compiling Generals sources with RTS_ZEROHOUR would fail.

Code

Generals/Code/GameEngine/Source/GameLogic/Map/PolygonTrigger.cpp[R178-181]

+#if RTS_ZEROHOUR
+		if (info->version >= K_TRIGGERS_VERSION_4) {
+			pTrig->setLayerName(layerName);
+		}

Evidence

The changed Generals PolygonTrigger.cpp contains RTS_ZEROHOUR-guarded code calling
setLayerName(...), but the Generals PolygonTrigger.h lacks that method and the related fields; the
Zero Hour header includes them, confirming they are ZH-only API surface.

Generals/Code/GameEngine/Source/GameLogic/Map/PolygonTrigger.cpp[146-183]
Generals/Code/GameEngine/Include/GameLogic/PolygonTrigger.h[64-140]
GeneralsMD/Code/GameEngine/Include/GameLogic/PolygonTrigger.h[80-130]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Generals/.../PolygonTrigger.cpp` now includes `#if RTS_ZEROHOUR` blocks that use Zero Hour-only APIs (`setLayerName/getLayerName`) and initialize/select/render state fields. However, `Generals/.../PolygonTrigger.h` does not declare these members or methods. Any attempt to build the Generals source set with `RTS_ZEROHOUR=1` (or to otherwise enable those codepaths) will fail to compile.
### Issue Context
- The PR adds `setLayerName(...)` calls and writes/reads `layerName` under `RTS_ZEROHOUR` in the Generals source file.
- The Generals header lacks `m_layerName`, `m_shouldRender`, `m_selected`, and the associated accessors.
- The Zero Hour header includes these members and methods, demonstrating the intended shape.
### Fix Focus Areas
- Generals/Code/GameEngine/Include/GameLogic/PolygonTrigger.h[64-140]
- Generals/Code/GameEngine/Source/GameLogic/Map/PolygonTrigger.cpp[146-183]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[Mauller]

You missed the other instances of RTS_GENERALS and RTS_ZEROHOUR

[OmarAglan]

/agentic_review

[OmarAglan]

You missed the other instances of RTS_GENERALS and RTS_ZEROHOUR

ok, this is bad, well im working on it, maybe im still lacking, well get them all.

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 21a5be6

[OmarAglan]

You missed the other instances of RTS_GENERALS and RTS_ZEROHOUR

i think i chnaged them all, hope so.

[stephanmeesters]

You should try to minimize these guards, only add as needed. Consider when it may affect the CRC and when it doesn’t. For example deleting polygon triggers you would expect impacts CRC, but adding a function or class member or define generally doesn’t.

[Mauller]

You should try to minimize these guards, only add as needed. Consider when it may affect the CRC and when it doesn’t. For example deleting polygon triggers you would expect impacts CRC, but adding a function or class member or define generally doesn’t.

One of the finer nuances here that determines if it affects the CRC is how the data is handled for the CRC.

There are some instances where a whole object get's CRC'ed instead of its portion of the CRC being generated from the objects specific CRC function.
This then causes problems when new fields are introduced to the class.

But what you mentioned is true as long as the above is not a problem.

[OmarAglan]

You should try to minimize these guards, only add as needed. Consider when it may affect the CRC and when it doesn’t. For example deleting polygon triggers you would expect impacts CRC, but adding a function or class member or define generally doesn’t.

well, yes that make sense, will look into it, but how can i test if my chnage will impact CRC, like replays or something, a general question?
i will try to minimize the Guards as possible.

[stephanmeesters]

well, yes that make sense, will look into it, but how can i test if my chnage will impact CRC, like replays or something, a general question? i will try to minimize the Guards as possible.

There is no standardized vgenerals replay testing right now but you could create a new replay in a map that has polygon triggers (or add them yourself, and make sure the trigger affects the game somehow) with AI's and then replay check them with -headless -replay myreplay.rep to see if the replay mismatches after adding the new code.

[OmarAglan]

updated and cleaned up the merge, it now has less guards, and now the behavior is:

• Retail-compatible Generals preserves malformed polygon triggers
• Retail-compatible Generals writes polygon trigger version 3 without layer names
• Non-retail Generals and Zero Hour remove malformed polygon triggers and write version 4 with layer names

ready for review

[OmarAglan]

/agentic_review

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 7177417

[stephanmeesters]

Note that "merge with rebase" is the same as "squash merge" when you only have one commit.

Perhaps close #3190 and put the move in here? Right now it's also confusing for people to review because of the depends on

[greptile-apps]

Greptile Summary

The PR consolidates shared map headers and implementations under Core while preserving target-specific map serialization behavior.

• Moves PolygonTrigger, SidesList, TerrainLogic, and MapReaderWriterInfo into the shared GameEngine source set.
• Selects PolygonTriggers format version 3 for retail-compatible Generals and version 4 for other configurations.
• Retains malformed polygon triggers only on the retail-compatible Generals path.
• Updates the Generals and Zero Hour build lists and records the completed operations in the unification script.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains.

No blocking failure remains.

Important Files Changed

Filename	Overview
Core/GameEngine/Source/GameLogic/Map/PolygonTrigger.cpp	Adds target-conditioned trigger validation and map-format serialization while moving the implementation into Core.
Core/GameEngine/Source/GameLogic/Map/SidesList.cpp	Moves the shared side and player-data implementation into Core without a substantive content change.
Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp	Moves the terrain implementation into Core while retaining the existing crater functionality.
Core/GameEngine/CMakeLists.txt	Enables the consolidated map headers and implementations in the shared GameEngine source set.
Generals/Code/GameEngine/CMakeLists.txt	Removes the now-shared map files from the Generals-local source set.
GeneralsMD/Code/GameEngine/CMakeLists.txt	Removes the now-shared map files from the Zero Hour-local source set.
scripts/cpp/unify_move_files.py	Records the completed map-file unification operations as inactive script entries.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    C[Core GameEngine map sources] --> G[Generals GameEngine]
    C --> Z[Zero Hour GameEngine]
    G --> D{Retail-compatible data?}
    D -->|Yes| V3[Write PolygonTriggers v3]
    D -->|No| V4[Write PolygonTriggers v4]
    Z --> V4

Loading

_{Reviews (11): Last reviewed commit: "unify(map): Move GameLogic map headers a..." | Re-trigger Greptile}

force push after review

[OmarAglan]

what is left for this pr to be merged?

[OmarAglan]

Added RETAIL_COMPATIBLE_DATA and used it for the PolygonTrigger output version and layer fields. I kept malformed trigger removal under RETAIL_COMPATIBLE_CRC because it changes the loaded game state, while the reader remains version driven guard.

[xezon]

Does the Polygon Trigger do anything if it is a single point? How will it change the CRC then?

[OmarAglan]

well for my understanding that a single point trigger cannot contain a point through pointInTrigger(), sript and AI paths such as attack and guard area actions and the supplies within perimeter condition use a trigger’s center and radius.

i suspect that deleting the trigger changes those lookups from a valid trigger to nullptr (Could be wrong, but this is probably the case here) so script or AI behavior can diverge and indirectly change the game CRC.

I think this is why retail Generals keeps its original load behavior here.

[xezon]

Can you verify that CRC mismatches when this is added or removed?

[OmarAglan]

i will try, and will share the finding here!

[OmarAglan]

Can you verify that CRC mismatches when this is added or removed?

after some testing, it didnt trigger a CRC mismatches, but im not 100% percent sure of that, i would like to keep it, what do you think?

[xezon]

Yes it does look a bit risky, but if this is provably not CRC relevant, then it would be nice to not classify it as such. Maybe someone else has an additional opinion on that.

[OmarAglan]

i suppose this now can be merged, let me know if there is anything i missed!

[xezon]

// Added in Zero Hour

[xezon]

This change can be finalized.