unify(worldbuilder): Add engine prerequisites for WorldBuilder unification - #3024
unify(worldbuilder): Add engine prerequisites for WorldBuilder unification#3024OmarAglan wants to merge 1 commit into
Conversation
|
| Filename | Overview |
|---|---|
| Core/GameEngine/Include/Common/GameDefines.h | Defines retail-compatible WorldBuilder output as the default shared configuration. |
| Generals/Code/GameEngine/Source/GameLogic/Map/PolygonTrigger.cpp | Adds version-4 layer-name parsing and conditional version-3/version-4 writing while preserving invalid triggers for snapshot compatibility. |
| Generals/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp | Prevents malformed retained triggers from being returned as named gameplay areas. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Map/PolygonTrigger.cpp | Retains malformed triggers and aligns conditional writer behavior with the Generals implementation. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp | Applies the same valid-area lookup restriction to Zero Hour. |
| Generals/Code/GameEngine/Include/Common/ThingTemplate.h | Exposes vision, ambient-sound, and weapon-template information required by WorldBuilder. |
| Generals/Code/GameEngine/Include/Common/WellKnownKeys.h | Registers the ambient-sound map-object properties consumed by the editor. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Map["PolygonTriggers map chunk"] --> Reader["Version-aware reader"]
Reader --> TriggerList["Complete trigger list"]
TriggerList --> Snapshot["Save/snapshot restoration by ID"]
TriggerList --> Lookup["Named gameplay-area lookup"]
Lookup --> Valid{"At least two points?"}
Valid -->|Yes| Gameplay["Scripts and AI"]
Valid -->|No| Hidden["Retained only for compatibility"]
Writer["Map writer"] --> Target{"Build target and compatibility mode"}
Target -->|Retail-compatible Generals| V3["Version 3 without layer names"]
Target -->|Zero Hour or compatibility disabled| V4["Version 4 with layer names"]
Reviews (7): Last reviewed commit: "unify(worldbuilder): Add Generals compat..." | Re-trigger Greptile
f749bac to
628bfe3
Compare
Skyaero42
left a comment
There was a problem hiding this comment.
This will make the Generals World builder fail loading maps.
Can you explain why? |
628bfe3 to
3bf6fb4
Compare
|
Changes:
|
3bf6fb4 to
8ad31ca
Compare
| loc.y = file.readInt(); | ||
| loc.z = file.readInt(); | ||
| pTrig->addPoint(loc); | ||
| } |
There was a problem hiding this comment.
Missing
+ if (numPoints<2) {
+ DEBUG_LOG(("Deleting polygon trigger '%s' with %d points.",
+ pTrig->getTriggerName().str(), numPoints));
+ deleteInstance(pTrig);
+ continue;
+ }
There was a problem hiding this comment.
I initially copied the numPoints < 2 filtering into Generals for parity, but tracing the save-loading path showed that this would break existing saves. GameLogic expects the polygon-trigger count and IDs loaded from the map to match the snapshot. Removing a zero or one point trigger during map loading can therefore cause restoration to fail with SC_INVALID_DATA.
I removed that filtering from both Generals and Zero Hour instead. This keeps the implementations unified while preserving save compatibility.
tell me if we can do anything else
There was a problem hiding this comment.
after giving it some thought, reintroducing the parser deletion would prevent existing saves from restoring when their trigger counts or IDs no longer match the map.
so a safer solution is retaing malformed triggers internally for snapshot compatibility, but treat triggers with fewer than two points as invalid and exclude them from TerrainLogic::getTriggerAreaByName(), preventing ai guards and script conditions from using the sentinel derived center and radius while preserving the trigger list needed during save restoration.
i hope it works :)
There was a problem hiding this comment.
How about:
if (info->version >= K_TRIGGERS_VERSION_4 && numPoints<2) {
DEBUG_LOG(("Deleting polygon trigger '%s' with %d points.",
pTrig->getTriggerName().str(), numPoints));
deleteInstance(pTrig);
continue;
}and then restore the PolygonTrigger::isValid() ?
edit: this is assuming all of the ZH maps are K_TRIGGERS_VERSION_4
There was a problem hiding this comment.
good direction, but i do not think we can assume every map loaded by Zero Hour uses PolygonTriggers version 4. The reader supports versions 1 to 4, and Zero Hour may load older or Generals maps.
so im suggesting preserving the existing behavior per target so Zero Hour continues filtering malformed triggers for every version, while Generals filters them only for version 4. Generals versions 1–3 would retain them for compatibility with existing saves.
we would also keep isValid() requiring at least two points and retain the TerrainLogic check, because the older Generals triggers that must remain for snapshot identity still must not be exposed as usable gameplay areas.
There was a problem hiding this comment.
more explanation for what i did (Used Chatgpt to explain and make it easier to read and discuss):
What is a malformed trigger?
A polygon trigger normally needs at least two points. A trigger with zero or one point is malformed.
These triggers cause two separate problems:
- Save compatibility
- Invalid gameplay geometry
Why saves are sensitive
When loading a save, the game:
- Loads polygon triggers from the original map.
- Loads the trigger snapshot from the save.
- Requires the map and save to contain the same trigger count and IDs.
If the parser deletes a trigger that existed when the save was created—or retains one that was previously deleted—the counts differ and loading fails with SC_INVALID_DATA.
Historical behavior
The two games behaved differently:
Zero Hour has Deleted triggers with fewer than two points
We should also keep:
m_numPoints < 2in isValid() and keep the TerrainLogic validity check. Those protect Generals when malformed triggers must remain internally for save compatibility.
96f3cbe to
a66869c
Compare
a66869c to
672a5b4
Compare
|
I do not understand the purpose of this PR. If this is a merge (& move) bringing Worldbuilder files into core, it should not contain TheSuperHackers comments. Use WinMerge or similar program to merge the files (except for copyright header). Then in a second commit or PR, move the files to core. Look up some of the merged PR's that do unification of the games how it is done. If this is not a merge & move in that sense, than it needs better explanation on what the purpose is of this PR. |
Merge with Rebase
This is not the WorldBuilder merge or move pull request.
This pull contains only engine-side prerequisites required before Generals can use the unified WorldBuilder implementation. No Tools/WorldBuilder files are changed here.
The follow-up pull merges the two WorldBuilder trees, and the final pull moves the unified implementation to Core.
RETAIL_COMPATIBLE_WORLDBUILDERis enabled by default. Generals therefore writes retail-compatible version 3 trigger data, while Zero Hour continues writing version 4 data with layer names.ThingTemplateexposes the vision, ambient sound, and weapon information used by WorldBuilder.No WorldBuilder files are changed here. The follow-up merge remains focused on the editor implementation itself.
Testing
git diff --checkmain