Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. WalkthroughWorldBuilder adds a control for optional side-data export. Script export writes versioned player data, with Generals retail-compatible builds retaining version 1. Import handling reads side dictionaries from version-2-or-later data and updates side lists and team ownership. ChangesSide-data export and import
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Merge Risk: 🟡 Moderate · up to Importing scripts with sides into a map that already has sides can show the imported side under the wrong tree entry or not at all. Fix the index passed to addPlayer before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new Generals importer can terminate the editor on inconsistent or capacity-exceeding side records. Failed imports can also leave partially imported sides available for later acceptance. Exposure requires opening a script bundle in the editor; retail-compatible export remains protected. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
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}
| SidesInfo* sides = pThis->m_sides.findSideInfo(pThis->m_readPlayerNames[i]); | ||
| // A script list must be created. | ||
| sides->setScriptList(pList); |
There was a problem hiding this comment.
A v2 script bundle can provide a player-name string that differs from the name in its side dictionary. The code adds the dictionary unchanged, then searches using the separate string and dereferences a null result, crashing WorldBuilder. The same dereference is possible when the map is already at the side limit and no new side can be added. Validate the name and confirm the side was added before using it.
How this was verified: The bundle supplies the two names separately, and the side lookup returns null when no stored side matches the player-name string.
Knowledge Base Used: Content pipeline tools
| // A script list must be created. | ||
| sides->setScriptList(pList); | ||
| // Update the dialog. | ||
| pThis->addPlayer(i); |
There was a problem hiding this comment.
When a v2 bundle adds a player to a map that already has sides, the new side is appended after those sides, but addPlayer(i) receives its position in the bundle instead. The tree node then points to an existing player, so the imported player's scripts do not appear under the correct player and the later refresh cannot reliably find its node. Use the new side's actual index.
| pThis->addPlayer(i); | |
| pThis->addPlayer(pThis->m_sides.getNumSides() - 1); |
Knowledge Base Used: Content pipeline tools
![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);1 participant
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 player-matching correction is kept in a separate bugfix branch. 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.
test how the world builder looks (WIP)
AI assistance was used for implementation and local verification.