Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBoth WorldBuilder versions validate imported player data and restore side state when parsing fails. They add tree entries for new sides and match imported scripts to map sides by player name. ChangesWorldBuilder script import
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: 🟡 Moderate · up to Script imports now match players by name and reject several kinds of malformed data, which fixes the original wrong-player assignment. However, a rejected import may still leave partial changes in the map document. Address this before merging. A malformed negative player count is also still accepted for single-list imports. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 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 |
6d06903 to
d8e9198
Compare
OmarAglan
changed the title
 |
![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: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
8bacc1f7-0461-4011-b086-f2fad5acd7c1
📒 Files selected for processing (2)
Generals/Code/Tools/WorldBuilder/src/ScriptDialog.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; 3 remain after this review.
|
|
||
| } catch(...) { | ||
| DEBUG_CRASH(("threw exception in ScriptDialog::OnLoad")); | ||
| ::AfxMessageBox("Unable to import scripts. The file contains invalid data or exceeds the player limit.", MB_OK); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n '#define DEBUG_ASSERTCRASH|DEBUG_ASSERTCRASH\(' Generals/Code/GameEngine/Include Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp | head -35
sed -n '1400,1480p' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 7578
Clear pending script lists after a failed import.
If PlayerScriptsList is parsed before ScriptsPlayers fails, the engine retains the pending lists because this OnLoad handles the error before calling ScriptList::getReadScripts(scripts). A later valid import without PlayerScriptsList can retrieve those stale lists and add a single list’s scripts to the selected side. Clear the pending lists at this failed-import boundary; the multi-list name check does not cover this path.
| } | ||
| DEBUG_ASSERTCRASH(file.atEndOfChunk(), ("Unexpected data left over.")); | ||
| return true; | ||
| return file.atEndOfChunk(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1360,1490p' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
sed -n '1530,1670p' GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
sed -n '1635,1660p' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
sed -n '1825,1885p' GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 12572
🏁 Script executed:
#!/bin/bash
set -o pipefail
rg -n "m_readPlayerNames|ParsePlayersDataChunk|WritePlayersDataChunk|ParseScriptsDataChunk|PlayerScriptsList|ScriptsPlayers" Generals/Code/Tools/WorldBuilder GeneralsMD/Code/Tools/WorldBuilder
printf '\\n--- name/list writers elsewhere in the two code trees ---\\n'
rg -n "WritePlayersDataChunk|ScriptsPlayers|PlayerScriptsList|WriteScriptsDataChunk" Generals/Code GeneralsMD/Code
printf '\\n--- targeted diff ---\\n'
git diff --unified=4 f8ba7eb44b688db14e2ff24d7172474f70be1587 d8e91982809a3fe8e244cff45739e4608dbd0b3d -- Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp | rg -n -C 8 "ParsePlayersDataChunk|m_readPlayerNames|OnLoad|return file.atEndOfChunk|MAX_PLAYER_COUNT"Repository: TheSuperHackers/GeneralsGameCode
Length of output: 14515
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\\n' '--- Generals export/import ---'
sed -n '1170,1245p' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
sed -n '1390,1455p' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
printf '%s\\n' '--- GeneralsMD export/import ---'
sed -n '1335,1415p' GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
sed -n '1565,1625p' GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
printf '%s\\n' '--- script list serialization implementations ---'
sed -n '565,660p' Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/Scripts.cpp
sed -n '570,675p' GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/Scripts.cpp
printf '%s\\n' '--- dialog constructors and name-array uses ---'
rg -n -C 5 "ScriptDialog::ScriptDialog|m_readPlayerNames" Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp Generals/Code/Tools/WorldBuilder/include/ScriptDialog.h GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp GeneralsMD/Code/Tools/WorldBuilder/include/ScriptDialog.hRepository: TheSuperHackers/GeneralsGameCode
Length of output: 26614
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\\n' '--- import action wiring and function entry ---'
rg -n -C 4 "OnLoad|ON_.*(LOAD|IMPORT)|ID_.*(LOAD|IMPORT)" Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp Generals/Code/Tools/WorldBuilder/include/ScriptDialog.h GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp GeneralsMD/Code/Tools/WorldBuilder/include/ScriptDialog.h
sed -n '1300,1385p' Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
sed -n '1490,1555p' GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
printf '%s\\n' '--- script-list parser entrypoint ---'
rg -n -C 9 "Bool ScriptList::ParseScriptsDataChunk|ParseScriptsDataChunk\\(" Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/Scripts.cpp GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/Scripts.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 13983
🏁 Script executed:
#!/bin/bash
set -o pipefail
for f in Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp; do
echo "--- $f ---"
rg -n "ScriptDialog::OnLoad|ON_BN_CLICKED\\(IDC_LOAD|openDataChunk\\(\"ScriptsPlayers\"|m_readPlayerNames\\[i\\]|return file.atEndOfChunk|m_readPlayerNames\\[MAX_PLAYER_COUNT\\]" "$f" "${f/\\/src\\//\\/include\\/ScriptDialog.h}"
if [[ "$f" == Generals/Code/* ]]; then
nl -ba "$f" | sed -n '1208,1228p;1408,1438p;1641,1654p'
else
nl -ba "$f" | sed -n '1368,1395p;1580,1612p;1825,1868p'
fi
done
for f in Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/Scripts.cpp GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/Scripts.cpp; do
echo "--- $f ---"
rg -n "ScriptList::ParseScriptsDataChunk|ScriptList::ParseScriptListDataChunk|ScriptList::WriteScriptsDataChunk|ScriptList::getReadScripts" "$f"
nl -ba "$f" | sed -n '495,635p'
doneRepository: TheSuperHackers/GeneralsGameCode
Length of output: 22825
Reject multi-list imports with too few player names.
Both parsers accept a complete player-name chunk even when it contains fewer names than the imported script lists. When the same dialog loads another file, an unread name slot can retain a previous import’s name. The second list can then be matched to the wrong map side.
Reset and retain the parsed-name count for each load. Before committing the import or matching lists, reject when count > 1 && parsedNameCount < count. Clean up staged lists and restore parser-added sides on rejection. Keep the count == 1 path unchanged; GeneralsMD supports one selected list with multiple exported side names.
|
|
||
| } catch(...) { | ||
| DEBUG_CRASH(("threw exception in ScriptDialog::OnLoad")); | ||
| ::AfxMessageBox("Unable to import scripts. The file contains invalid data or exceeds the player limit.", MB_OK); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n '#define DEBUG_ASSERTCRASH|DEBUG_ASSERTCRASH\(' GeneralsMD/Code/GameEngine/Include GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp | head -35
sed -n '1580,1660p' GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 7582
🏁 Script executed:
printf '%s\n' '--- WorldBuilder OnLoad ---'
sed -n '1485,1665p' GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp | nl -ba -v1485
printf '%s\n' '--- Script producer/getter ---'
sed -n '520,645p' GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/Scripts.cpp | nl -ba -v520
sed -n '645,695p' GeneralsMD/Code/GameEngine/Source/Common/System/DataChunk.cpp | nl -ba -v645
printf '%s\n' '--- DataChunk parse ---'
rg -n 'DataChunkInput::parse|DEBUG_ASSERTCRASH' GeneralsMD/Code/GameEngine/Source/Common/System/DataChunk.cpp GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/Scripts.cpp GeneralsMD/Code/GameEngine/Include GeneralsMD/Code/GameEngine/Source/Common | head -100
printf '%s\n' '--- Macro definitions ---'
rg -n '#\\s*define\\s+DEBUG_ASSERTCRASH|DEBUG_ASSERTCRASH' GeneralsMD/Code/GameEngine/Include/Common GeneralsMD/Code/GameEngine/Include | head -80Repository: TheSuperHackers/GeneralsGameCode
Length of output: 32896
🏁 Script executed:
printf '%s\n' '--- PR diff for OnLoad ---'
git diff --unified=8 f8ba7eb44b688db14e2ff24d7172474f70be1587 d8e91982809a3fe8e244cff45739e4608dbd0b3d -- GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp | sed -n '1,240p'
printf '%s\n' '--- OnLoad bindings and player-name writes ---'
rg -n -C 3 'OnLoad\\(|m_readPlayerNames|ParsePlayersDataChunk|ON_COMMAND.*(LOAD|IMPORT)|ID_.*(LOAD|IMPORT)' GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.h
printf '%s\n' '--- DEBUG_ASSERTCRASH macro definition repository-wide ---'
rg -n '#\\s*define\\s+DEBUG_ASSERTCRASH' GeneralsMD/Code | head -30Repository: TheSuperHackers/GeneralsGameCode
Length of output: 6779
🏁 Script executed:
printf '%s\n' '--- ScriptDialog message map and OnLoad references ---'
rg -n -C 4 'OnLoad|BEGIN_MESSAGE_MAP|ON_COMMAND|ON_BN_CLICKED' GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.h | head -150
printf '%s\n' '--- ScriptPlayers parser and m_readPlayerNames ---'
rg -n -C 3 'm_readPlayerNames|ParsePlayersDataChunk' GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.hRepository: TheSuperHackers/GeneralsGameCode
Length of output: 9620
Clear pending script lists when parsing fails.
If PlayerScriptsList is parsed before a later chunk fails, ScriptList::ParseScriptsDataChunk has already stored its lists. The parse-error catch restores m_sides but leaves those lists pending. After the message box, OnLoad returns and can be called again. A successful import without PlayerScriptsList can then pass the stale list to getReadScripts and add its scripts to the selected side. Drain and delete pending lists in this failure catch.
Suggested fix
} catch(...) {
m_sides = sidesBeforeImport;
+ ScriptList *pendingScripts[MAX_PLAYER_COUNT];
+ Int pendingCount = ScriptList::getReadScripts(pendingScripts);
+ for (Int pendingIndex = 0; pendingIndex < pendingCount; pendingIndex++) {
+ deleteInstance(pendingScripts[pendingIndex]);
+ }
throw;
}
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
This change adds a lot of new comments. Are all of these warranted by user facing problems?
| curSide = m_curSelection.m_playerIndex; | ||
| } else { | ||
| Int j; | ||
| // TheSuperHackers @bugfix OmarAglan Match each imported player to the current map side by name. |
There was a problem hiding this comment.
Superfluous comment because no one will look back on this
| if (i>=MAX_PLAYER_COUNT) break; | ||
| pThis->m_readPlayerNames[i] = file.readAsciiString(); | ||
| } | ||
| DEBUG_ASSERTCRASH(file.atEndOfChunk(), ("Unexpected data left over.")); |
There was a problem hiding this comment.
Why was this removed? Many other functions also do it like that.
| // TheSuperHackers @bugfix OmarAglan Restore sides and teams when script parsing fails. | ||
| SidesList sidesBeforeImport; | ||
| sidesBeforeImport = m_sides; | ||
| try { |
There was a problem hiding this comment.
This try catch looks unnecessary. Unless DataChunkInput::parse throws?
| ScriptDialog *pThis = (ScriptDialog *)userData; | ||
| Int numNames = file.readInt(); | ||
| // TheSuperHackers @bugfix OmarAglan Reject player counts that cannot fit in the import array. | ||
| if (numNames < 0 || numNames > MAX_PLAYER_COUNT) { |
There was a problem hiding this comment.
Previously MAX_PLAYER_COUNT was a clamp, now it is a fail condition. Maybe just clamp numNames between 0 and MAX_PLAYER_COUNT?
d8e9198 to
03065d0
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| readDicts = file.readInt(); | ||
| } | ||
| Int numNames = file.readInt(); | ||
| numNames = max(0, min(numNames, Int(MAX_PLAYER_COUNT))); |
There was a problem hiding this comment.
🟡 Medium src/ScriptDialog.cpp:1842
Over-capacity files are accepted as valid, with player records beyond MAX_PLAYER_COUNT silently discarded. Clamping numNames before the loop prevents the parser from detecting the declared excess, and the remaining-record check is debug-only; reject values above MAX_PLAYER_COUNT before clamping (while retaining the existing handling for negative counts).
| numNames = max(0, min(numNames, Int(MAX_PLAYER_COUNT))); | |
| if (numNames > MAX_PLAYER_COUNT) { | |
| return false; | |
| } | |
| numNames = max(0, numNames); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp around line 1842:
Over-capacity files are accepted as valid, with player records beyond `MAX_PLAYER_COUNT` silently discarded. Clamping `numNames` before the loop prevents the parser from detecting the declared excess, and the remaining-record check is debug-only; reject values above `MAX_PLAYER_COUNT` before clamping (while retaining the existing handling for negative counts).
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
a98255a8-b1a5-451f-92e3-d29b114c9b67
📒 Files selected for processing (4)
Generals/Code/Tools/WorldBuilder/include/ScriptDialog.hGenerals/Code/Tools/WorldBuilder/src/ScriptDialog.cppGeneralsMD/Code/Tools/WorldBuilder/include/ScriptDialog.hGeneralsMD/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; 3 remain after this review.
| for (Int i = 0; i < count; i++) { | ||
| deleteInstance(scripts[i]); | ||
| } | ||
| m_sides = sidesBeforeImport; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Roll back document changes when an import fails. Both parsers can add waypoint links before the new post-parse name-count check rejects an import. Restoring m_sides leaves those links in the document without the imported objects.
Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp#L1398-L1398: stage or remove added waypoint links on failure, and dispose of staged objects and triggers.GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp#L1565-L1565: apply the same rollback and staged-resource cleanup.
📍 Affects 2 files
Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp#L1398-L1398(this comment)GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp#L1565-L1565
| { | ||
| ScriptDialog *pThis = (ScriptDialog *)userData; | ||
| Int numNames = file.readInt(); | ||
| numNames = max(0, min(numNames, Int(MAX_PLAYER_COUNT))); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Reject invalid serialized player counts in both parsers. Clamping a negative count to zero lets a one-list import pass validation with invalid player data.
Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp#L1655-L1655: fail parsing when the declared count is outside0..MAX_PLAYER_COUNT.GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp#L1842-L1842: apply the same declared-count validation before reading names.
📍 Affects 2 files
Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp#L1655-L1655(this comment)GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp#L1842-L1842
| readDicts = file.readInt(); | ||
| } | ||
| Int numNames = file.readInt(); | ||
| numNames = max(0, min(numNames, Int(MAX_PLAYER_COUNT))); |
There was a problem hiding this comment.
When a script file declares more than MAX_PLAYER_COUNT player names, this clamp reads only the first 16, but the parser still reports success. The unread data can trigger an assertion in debug builds; in release builds, the malformed file can be imported silently. Negative counts are also accepted as zero. The Generals importer has the same issue. Reject counts outside the supported range instead.
| if (count > 1 && m_numReadPlayerNames < count) { | ||
| throw(ERROR_CORRUPT_FILE_FORMAT); |
There was a problem hiding this comment.
If this new check rejects a file containing a WaypointsList, parsing has already added its links to the map. The catch restores sides and deletes pending script lists, but it does not remove those links. The import reports failure while still changing the map; the Generals importer has the same failure path. Add links only after validation succeeds, or remove them when import fails.
2 participants
Relates to #555.
Fixes player-name matching when importing scripts in both WorldBuilder versions.
The lookup previously compared current side
iwith imported playerj, then selected sidej. Different player ordering could assign scripts to the wrong player. More imported players than existing sides could also cause an invalid side access.Compare current side
jwith imported playeriinstead. Single-list imports retain the existing selected-player behavior.Also addresses the player-import findings raised on #3409:
The side-dictionary fixes apply to Zero Hour here; #3409 brings the corrected implementation to Generals.
Validation:
AI assistance was used for implementation and local verification.