Repository navigation
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. WalkthroughWorldBuilder adds an option to export side data. Script export writes versioned player data, with Generals retail-compatible builds retaining version 1. Import validates and reads side dictionaries, updates side lists and script trees, and preserves a valid selected team owner. ChangesSide-data export and import
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ExportOptions
participant ScriptDialog
participant PlayerDataChunk
ExportOptions->>ScriptDialog: Provide side-export setting
ScriptDialog->>PlayerDataChunk: Write versioned player data and optional side dictionaries
Merge Risk: 🔵 Low · up to Imports may retain allocated data when a malformed file fails to parse. The previously reported missing tree item appears addressed; confirm error-path cleanup before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Compatibility gates, player-identity checks, capacity limits, and parse-failure restoration constrain the change. No introduced security vulnerability was established, but recovery after later import failures and the full downstream effects of imported side fields remain incompletely assessed. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
01404c5e-443a-4e11-bd76-3f3b6bf198af
📒 Files selected for processing (8)
Generals/Code/Tools/WorldBuilder/include/ExportScriptsOptions.hGenerals/Code/Tools/WorldBuilder/res/WorldBuilder.rcGenerals/Code/Tools/WorldBuilder/res/resource.hGenerals/Code/Tools/WorldBuilder/src/ExportScriptsOptions.cppGenerals/Code/Tools/WorldBuilder/src/ScriptDialog.cppGeneralsMD/Code/Tools/WorldBuilder/res/WorldBuilder.rcGeneralsMD/Code/Tools/WorldBuilder/src/ExportScriptsOptions.cppGeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
| // A script list must be created. | ||
| sides->setScriptList(pList); | ||
| // Update the dialog. | ||
| pThis->addPlayer(i); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1655,1710p' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
rg -n 'addPlayer' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 2073
🏁 Script executed:
printf '%s\\n' '--- addPlayer ---'
sed -n '385,435p' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
printf '%s\\n' '--- side collection definitions ---'
rg -n 'SidesList::addSide|addSide\\s*\\(|class SidesList|SidesList::getNumSides' Generals/CodeRepository: TheSuperHackers/GeneralsGameCode
Length of output: 1946
🏁 Script executed:
printf '%s\n' '--- addPlayer remainder ---'
sed -n '415,455p' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
printf '%s\n' '--- side collection symbol locations ---'
rg -n 'addSide|class SidesList' Generals/Code | head -80Repository: TheSuperHackers/GeneralsGameCode
Length of output: 3896
🏁 Script executed:
printf '%s\n' '--- SidesList and addSide definitions/usages ---'
rg -n 'class SidesList|struct SidesList|SidesList::|addSide[[:space:]]*\(' --glob '*.{h,cpp}' .
printf '%s\n' '--- ScriptDialog import/tree flow ---'
rg -n -C 5 'ParsePlayersDataChunk|DeleteAllItems|addPlayer[[:space:]]*\(|OnLoad|LoadScript' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 14035
🏁 Script executed:
printf '%s\n' '--- SidesList declaration and index lookup ---'
sed -n '132,226p' Core/GameEngine/Include/GameLogic/SidesList.h
sed -n '390,407p' Core/GameEngine/Source/GameLogic/Map/SidesList.cpp
printf '%s\n' '--- SidesList append implementation ---'
sed -n '598,614p' Core/GameEngine/Source/GameLogic/Map/SidesList.cpp
printf '%s\n' '--- OnLoad post-parse flow ---'
sed -n '1359,1495p' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
printf '%s\n' '--- reviewed diff hunk ---'
git diff --unified=4 5ae042cafb4f08c5ec264acbdfc2db964a41b295 fc90243e3240e59f3849d82e10e1073f2e8fae0c -- Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp | rg -n -C 12 'addPlayer\(i\)|addSide\(&sideDict\)'Repository: TheSuperHackers/GeneralsGameCode
Length of output: 8860
🏁 Script executed:
rg -n -C 20 'ScriptDialog::reloadPlayer|reloadPlayer[[:space:]]*\(' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp Generals/Code/Tools/WorldBuilder/include/ScriptDialog.hRepository: TheSuperHackers/GeneralsGameCode
Length of output: 26581
🏁 Script executed:
sed -n '629,700p' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 2053
Pass the appended side’s index to addPlayer.
If the destination already has sides, or earlier imported records were skipped, i can differ from the new side’s index. For example, with two existing sides, a new first imported record appends at index 2, but addPlayer(0) inserts another tree item for side 0. The later reloadPlayer(2) cannot find the new side’s item and returns.
Suggested fix
pThis->m_sides.addSide(&sideDict);
ScriptList* pList = newInstance(ScriptList);
- SidesInfo* sides = pThis->m_sides.findSideInfo(pThis->m_readPlayerNames[i]);
+ Int sideIndex;
+ SidesInfo* sides = pThis->m_sides.findSideInfo(pThis->m_readPlayerNames[i], &sideIndex);
// A script list must be created.
sides->setScriptList(pList);
// Update the dialog.
- pThis->addPlayer(i);
+ pThis->addPlayer(sideIndex);
fc90243 to
f16cf9a
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| throw(ERROR_CORRUPT_FILE_FORMAT); | ||
| } | ||
| } catch(...) { | ||
| m_sides = sidesBeforeImport; |
There was a problem hiding this comment.
🟠 High src/ScriptDialog.cpp:1404
A parse failure leaves imported waypoint links in the active document, so a file with valid waypoints followed by invalid player/script data produces a partial import that can be saved. ParseWaypointDataChunk calls pDoc->addWaypointLink(...) during file.parse(this), but this catch block restores only m_sides; roll back those document-side changes as well, or make the import transactional.
🤖 Copy this AI Prompt to have your agent fix this:
In file @Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp around line 1404:
A parse failure leaves imported waypoint links in the active document, so a file with valid waypoints followed by invalid player/script data produces a partial import that can be saved. `ParseWaypointDataChunk` calls `pDoc->addWaypointLink(...)` during `file.parse(this)`, but this catch block restores only `m_sides`; roll back those document-side changes as well, or make the import transactional.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp (1)
1396-1405: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe restore covers sides only. Imported objects and triggers still leak when parsing fails.
A parse can fail after
ParseObjectsDataChunkorParsePolygonTriggersDataChunkhas allocatedm_firstReadObjectorm_firstTrigger. The catch block restoresm_sidesand rethrows. It does not free those lists. The objects stay unowned. The next import sets the pointers tonullptragain. The impact is a memory leak in an editor error path.The comment also says "sides and teams."
SidesListholds the team data, so that part is correct.Free the partially read lists in the catch block before you rethrow:
} catch(...) { m_sides = sidesBeforeImport; if (m_firstReadObject) { deleteInstance(m_firstReadObject); m_firstReadObject = nullptr; } // free the m_firstTrigger chain in the same way throw; }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
08fff1ad-a655-4c27-8513-5ecaf7dab8b9
📒 Files selected for processing (5)
Generals/Code/Tools/WorldBuilder/include/ExportScriptsOptions.hGenerals/Code/Tools/WorldBuilder/res/WorldBuilder.rcGenerals/Code/Tools/WorldBuilder/src/ScriptDialog.cppGeneralsMD/Code/Tools/WorldBuilder/res/WorldBuilder.rcGeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
| } catch(...) { | ||
| m_sides = sidesBeforeImport; | ||
| throw; |
There was a problem hiding this comment.
If a bundle contains waypoint links and then fails a player-count or dictionary-name check, parsing has already added those links to the document. This catch restores the sides but leaves links to waypoints that were never imported. Parsed script lists can also remain pending for the next import. The same rollback behavior occurs in the Zero Hour editor.
Knowledge Base Used: Game development tools
1 participant
Depends on #3408. Merge #3408 first; the second commit contains the alignment changes.
Follow up for #3368.
Relates to #555.
Aligns the import/export portions of ScriptDialog and ExportScriptsOptions with Zero Hour.
Generals inherits side-dictionary import, team-owner selection handling and the export-side option. Both editors use the aligned export dialog, sized to contain its controls.
Compatibility:
The import fixes originate in #3408. This branch builds on that commit and aligns Generals with the corrected Zero Hour importer. Script-warning changes and Core moves remain outside this PR.
Validation:
Both VC6 Release WorldBuilder targets build.
All 16 targeted VC6 compilations pass across both games, debug/release and DATA=0/1.
1,904 extracted writer/reader fixture cases pass.
Focused team-owner and export-dialog checks pass.
249 additional import-validation, capacity, tree-index and rollback cases pass per game.
test how the world builder looks (WIP)
AI assistance was used for implementation and local verification.