unify(worldbuilder): Merge Generals and Zero Hour WorldBuilder by OmarAglan · Pull Request #3025 · TheSuperHackers/GeneralsGameCode

OmarAglan · 2026-07-28T10:51:08Z

Merge with Rebase

Relates to Merge Generals and Zero Hour code into one #555
Merge after unify(worldbuilder): Align PolygonTrigger serialization and editor prerequisites #3024
Second of three pull requests to unify WorldBuilder

Generals and Zero Hour currently maintain separate WorldBuilder implementations that have drifted apart. They need to be aligned before the duplicate editor trees can be moved to Core.

This pull merges the two implementations:

Generals gains the newer WorldBuilder features from Zero Hour, including layers, the ruler, team object properties, ambient sound editing, and the expanded editor resources.
Shared editor behavior and build manifests are aligned while game-specific behavior remains behind RTS_GENERALS and RTS_ZEROHOUR.
Generals keeps its own blend-tile serialization and camera-direction behavior.
Each game keeps its own splash bitmap and other intentional product-specific differences.
Exported script bundles continue using PolygonTriggers version 3; this pull does not introduce a new bundle format.
Review cleanup removes an obsolete debug block, preserves the existing Zero Hour file endings, and documents why object-center culling is not used for range overlays.

No WorldBuilder files move to Core here. That remains the final pull in the series.

This branch is based on #3024, so GitHub will show the prerequisite commit until #3024 is merged with rebase.

Testing

git diff --check
Confirmed every source and header newly listed by the CMake manifests exists in its WorldBuilder tree
Confirmed the merge commit changes only WorldBuilder paths
Confirmed the WorldBuilder merge is one commit on top of unify(worldbuilder): Align PolygonTrigger serialization and editor prerequisites #3024
Generals WorldBuilder runs successfully in local testing
Fork GenCI: all 13 Generals and Zero Hour build configurations passed

greptile-apps · 2026-07-28T10:53:51Z

Greptile Summary

This pull aligns the separate Generals and Zero Hour WorldBuilder implementations.

Adds layers, ruler tooling, expanded object and team properties, ambient-sound editing, and additional visualization controls to Generals.
Aligns WorldBuilder resources, build manifests, script editing, trigger handling, and map serialization while retaining target-specific behavior.
Fixes layer-rename synchronization for objects, polygon triggers, and the active-layer name.
Removes invalid polygon triggers from layer membership before unlinking them globally.

Confidence Score: 3/5

The PR is not yet safe to merge because script-bundle round trips still lose polygon-trigger layer assignments.

The layer-rename and invalid-trigger cleanup issues are fixed at current HEAD, but ScriptDialog still writes and reads polygon triggers using a format with no layer-name field and imports them without registering layer membership.

Files Needing Attention: Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp

Important Files Changed

Filename	Overview
Generals/Code/Tools/WorldBuilder/src/LayersList.cpp	Adds polygon-trigger layer membership and active-layer behavior, and now synchronizes persisted assignments when layers are renamed.
Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp	Expands script verification and side-bundle handling while retaining the existing version-3 polygon-trigger bundle format.
Generals/Code/Tools/WorldBuilder/src/WaypointOptions.cpp	Safely removes invalid triggers from layer membership before unlinking them from the global trigger list.
Generals/Code/Tools/WorldBuilder/src/WaterOptions.cpp	Applies the corresponding layer-membership cleanup when invalid water triggers are removed.
Generals/Code/GameEngine/Source/GameLogic/Map/PolygonTrigger.cpp	Adds trigger-layer serialization support while preserving retail Generals map compatibility through conditional version selection.

_{Reviews (10): Last reviewed commit: "unify(worldbuilder): Merge WorldBuilder" | Re-trigger Greptile}

stephanmeesters

Incomplete review

stephanmeesters · 2026-07-28T18:13:13Z

 		/***************POLYGON TRIGGERS DATA ***************/
-		chunkWriter.openDataChunk("PolygonTriggers", 	K_TRIGGERS_VERSION_3);
+		// Version 4 preserves polygon trigger layer assignments in script bundles.
+		chunkWriter.openDataChunk("PolygonTriggers", 	K_TRIGGERS_VERSION_4);


Safe to do this?

well, as the added comments says version 4 is required because this record now contains the layer name and The updated reader only consumes it for version 4 and continues to load versions 1 to 3. note that old builds will not understand newly written version 4 bundles. so it probably safe.

more explanation: adding layerName changes the serialized record layout, so keeping version 3 would cause the following trigger ID and flags to be read at the wrong offsets. Both writers and readers are updated together, and the reader only consumes layerName for version 4 or newer, so existing version 1–3 bundles still load.

The expected limitation is that older WorldBuilder builds cannot read bundles written in version 4.

this can be another pr that fix this, if needed

I think that the retail version of vanilla Generals will not be able to open the maps saved with the version 4.

Could be discussed first but I suggest to introduce a new macro in GameDefines.h RETAIL_COMPATIBLE_WORLDBUILDER (enabled by default). So for PolygonTriggers it should keep using version 3 for generalsv when saving maps when that macro is enabled.

Another option would be to keep the existing PolygonTriggers chunk at version 3 and store layer assignments in a separate optional chunk instead of changing the original record layout.

For example:

PolygonTriggers (version 3) trigger count trigger name trigger ID water and river flags points ... PolygonTriggerLayers (version 1) assignment count trigger name layer name ...

The new WorldBuilder would write and register parsers for both chunks. It would load the version 3 triggers first, then apply the layer assignments from PolygonTriggerLayers.

Trigger names would probably be the safest key here because WorldBuilder already treats them as unique identifiers, and the script-bundle importer does not preserve the serialized trigger IDs.

The existing DataChunkInput::parse() implementation skips chunks for which no parser is registered, so retail WorldBuilder should read the version 3 PolygonTriggers data and ignore PolygonTriggerLayers. This would need to be confirmed with an actual retail WorldBuilder test.

The same extension chunk could be used for exported script bundles, allowing their main PolygonTriggers chunk to remain at version 3 as well.

it has big tradeoffs like for example, Saving the map again with retail WorldBuilder would remove the unsupported layer-assignment chunk, and Both the map loader and script-bundle importer would need to register the new parser.
we also should consider missing or invalid trigger names would need to fall back safely to the default trigger layer, and we would need to test maps and script bundles in both the new and retail WorldBuilder builds.

this would preserve retail readability without requiring users to disable a compatibility macro, while Zero Hour could retain its existing version 4 format.

But its complex, and im not in favor of it really, a clean change would be a macro as you suggested, we need another opinion,

Why is this logic added to GeneralsMD, while Generals doesn't have a version 4? This doesn't seem right

OmarAglan · 2026-07-29T09:34:55Z

While reviewing the code yesterday, I found a few issues that are worth addressing. I don’t think they all need to be fixed in this PR unless they are required for correctness; the remaining items could be handled in one or two focused follow up PRs.

This points to the Generals implementation, but the corresponding Zero Hour files should be checked as well.

Preserve the default trigger layer during reload

In WorldBuilderDoc.cpp:403, triggers belonging to the default layer are serialized with an empty layer name. During reload, addPolygonTriggerToLayersList() tries the previously active layer before falling back to the default trigger layer. If a map is reopened after another layer was selected, default layer triggers can be moved into that active layer.

The script-bundle import path in ScriptDialog.cpp:1565 has the same ambiguity.
Guard zero-parameter entries before opening the parameter editor

The new rich edit keyboard path in EditAction.cpp:434 calls EditParameter::edit() without first checking whether the action has any parameters. Several common actions, including victory, defeat, and null operations, have no parameters. Typing while one of these entries is focused can therefore pass a null pointer to the editor and cause a crash.

The same issue appears in the condition editor at EditCondition.cpp:437.
Apply visibility settings to optimized trees

In wbview3d.cpp:1017, updateTrees() adds optimized trees without checking FLAG_DONT_RENDER or the Show Models setting. Hiding a layer only updates the optimized tree’s position, while rebuilding the tree list after disabling models adds it again.

As a result, optimized trees can remain visible after their layer or all models has been hidden.
Keep the default sound index valid in the sorted combo box

In mapobjectprops.cpp:1733, IDC_SOUND_COMBO uses CBS_SORT. The code stores the index returned when inserting Default, then inserts (None) afterward. Because the combo box sorts its contents, the second insertion can move Default and make the stored index stale.

On the first UI refresh, this can remove an actual sound instead of Default, leave duplicate default entries, or save the default label as an ambient sound name.
Initialize the ruler length before circle mode uses it

In RulerTool.cpp:38, m_savedLength is not initialized. Switching directly to circle mode before measuring a line reads that value through getLength() and uses it as the radius, which can produce unpredictable measurements or geometry.

Skyaero42

I stopped reviewing at some point

There are quite a few issues with the merge.
Particularly, I see code added to GeneralsMD files that I'm unable to find in Generals.
The size of this review (> 10k changes) makes it hard to review. I recommend smaller changes.

Skyaero42 · 2026-07-29T18:29:07Z

 	while (count>0) {
 		count--;
 		triggerName = file.readAsciiString();
+		if (info->version >= K_TRIGGERS_VERSION_4) {


I think @stephanmeesters mentioned it already (but I don't see his comment appear here), this needs to be behind ZH guards or the Generals World Builder will no longer work properly.

#if RTS_ZEROHOUR if (info->version >= K_TRIGGERS_VERSION_4) { layerName = file.readAsciiString(); } #endif

I kept the reader version gated rather than Zero Hour-only. With the updated writer logic in #3024, Generals writes version 3 by default, but it can still write version 4 when RETAIL_COMPATIBLE_WORLDBUILDER is disabled. The Generals reader therefore still needs to understand version 4. Zero Hour continues writing and reading version 4.

Skyaero42 · 2026-07-29T18:30:34Z

 		numPoints = file.readInt();
 		PolygonTrigger *pTrig = newInstance(PolygonTrigger)(numPoints+1);
 		pTrig->setTriggerName(triggerName);
+		if (info->version >= K_TRIGGERS_VERSION_4) {


Needs to be behind RTS_ZEROHOUR guards

The same reasoning applies here. Generals can receive version 4 data when compatibility is disabled, so it needs to apply the serialized layer name. The version check ensures that version 1–3 files retain their existing layout and behavior.

Skyaero42 · 2026-07-29T18:30:40Z

 void PolygonTrigger::WritePolygonTriggersDataChunk(DataChunkOutput &chunkWriter)
 {
-	chunkWriter.openDataChunk("PolygonTriggers", 	K_TRIGGERS_VERSION_3);
+	chunkWriter.openDataChunk("PolygonTriggers", 	K_TRIGGERS_VERSION_4);


Needs to be behind RTS_ZEROHOUR guards

Updated in #3024. Both writer implementations now use RETAIL_COMPATIBLE_WORLDBUILDER && RTS_GENERALS: Generals writes version 3 by default, while Zero Hour and non-compatible Generals builds write version 4.

Skyaero42 · 2026-07-29T18:30:44Z

 		chunkWriter.writeInt(count);
 		for (pTrig=PolygonTrigger::getFirstPolygonTrigger(); pTrig; pTrig = pTrig->getNext()) {
 			chunkWriter.writeAsciiString(pTrig->getTriggerName());
+			chunkWriter.writeAsciiString(pTrig->getLayerName());


Needs to be behind RTS_ZEROHOUR guards

Updated alongside the version selection. layerName is written only when the version 4 branch is selected, using !(RETAIL_COMPATIBLE_WORLDBUILDER && RTS_GENERALS) in both games.

Skyaero42 · 2026-07-29T18:34:48Z

    "include/WaypointTool.h"
    "include/WBFrameWnd.h"
    "include/WBHeightMap.h"
+    "include/WBPopupSlider.h"


Am I missing something or is this file not actually being added in this PR?

WBPopupSlider.h already exists in both WorldBuilder trees on main, This PR only adds the existing header to the CMake source list, so there is no new file in this diff.

Skyaero42 · 2026-07-29T18:49:46Z

 			pNextTrig = pTrig->getNext();
 			pTrig->setNextPoly(nullptr);
+			// Register the imported trigger with its serialized WorldBuilder layer.
+			TheLayersList->addPolygonTriggerToLayersList(pTrig, pTrig->getLayerName());


Why is this added to a GeneralsMD (zerohour) file, while it is a feature not present in Generals?

Skyaero42 · 2026-07-29T18:49:56Z

 		count--;
 		Bool isWater = false;
 		triggerName = file.readAsciiString();
+		if (info->version >= K_TRIGGERS_VERSION_4) {


Why is this added to a GeneralsMD (zerohour) file, while it is a feature not present in Generals?

Skyaero42 · 2026-07-29T18:50:01Z

 		numPoints = file.readInt();
 		PolygonTrigger *pTrig = newInstance(PolygonTrigger)(numPoints+1);
 		pTrig->setTriggerName(triggerName);
+		if (info->version >= K_TRIGGERS_VERSION_4) {


Why is this added to a GeneralsMD (zerohour) file, while it is a feature not present in Generals?

OmarAglan · 2026-07-29T19:16:40Z

I stopped reviewing at some point

There are quite a few issues with the merge.

Particularly, I see code added to GeneralsMD files that I'm unable to find in Generals.

The size of this review (> 10k changes) makes it hard to review. I recommend smaller changes.

Ok i can spilt this pr to small review able pr, the question is how much?
I cant split it to 2 or 3 pr, what do you think?

greptile-apps · 2026-07-30T08:51:35Z

Want your agent to iterate on Greptile's feedback? Try greploops.

This was referenced Jul 28, 2026

unify(worldbuilder): Move unified WorldBuilder to Core OmarAglan/GeneralsGameCode#3

Closed

unify(worldbuilder): Merge Generals and Zero Hour WorldBuilder OmarAglan/GeneralsGameCode#2

Closed

OmarAglan force-pushed the unify/worldbuilder-merge branch from c56bc00 to bc2980a Compare July 28, 2026 11:12

greptile-apps Bot reviewed Jul 28, 2026

View reviewed changes

Comment thread Generals/Code/Tools/WorldBuilder/src/LayersList.cpp

OmarAglan force-pushed the unify/worldbuilder-merge branch from bc2980a to 0a40354 Compare July 28, 2026 11:17

greptile-apps Bot reviewed Jul 28, 2026

View reviewed changes

Comment thread Generals/Code/Tools/WorldBuilder/src/LayersList.cpp

OmarAglan force-pushed the unify/worldbuilder-merge branch from 0a40354 to 44c1641 Compare July 28, 2026 11:21

This was referenced Jul 28, 2026

unify(worldbuilder): Move unified WorldBuilder to Core OmarAglan/GeneralsGameCode#4

Closed

unify(worldbuilder): Move unified WorldBuilder to Core #3026

Open

stephanmeesters reviewed Jul 28, 2026

View reviewed changes

OmarAglan force-pushed the unify/worldbuilder-merge branch 3 times, most recently from 7f8a34a to 09aa190 Compare July 29, 2026 08:43

Skyaero42 requested changes Jul 29, 2026

View reviewed changes

OmarAglan force-pushed the unify/worldbuilder-merge branch 2 times, most recently from c30ce18 to 68d5ff3 Compare July 30, 2026 08:49

OmarAglan force-pushed the unify/worldbuilder-merge branch from 68d5ff3 to 54a07b7 Compare July 30, 2026 09:37

OmarAglan added 2 commits July 30, 2026 12:48

unify(worldbuilder): Add Generals compatibility prerequisites

672a5b4

unify(worldbuilder): Merge WorldBuilder

854debb

OmarAglan force-pushed the unify/worldbuilder-merge branch from 54a07b7 to 854debb Compare July 30, 2026 09:48

Uh oh!

Conversation

OmarAglan commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

greptile-apps Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Uh oh!

Uh oh!

Uh oh!

stephanmeesters left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OmarAglan Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OmarAglan commented Jul 29, 2026

Uh oh!

Skyaero42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OmarAglan commented Jul 29, 2026

Uh oh!

greptile-apps Bot commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

OmarAglan commented Jul 28, 2026 •

edited

Loading

greptile-apps Bot commented Jul 28, 2026 •

edited

Loading

OmarAglan Jul 29, 2026 •

edited

Loading