Skip to content

bugfix(worldbuilder): Fix script player import #3408

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
OmarAglan wants to merge 1 commit into
base: main
Choose a base branch
from
+74 −20
Open

bugfix(worldbuilder): Fix script player import #3408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Generals/Code/Tools/WorldBuilder/include/ScriptDialog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ class ScriptDialog : public CDialog
Int m_maxWaypoint;

AsciiString m_readPlayerNames[MAX_PLAYER_COUNT];
Int m_numReadPlayerNames;

protected:
HTREEITEM addPlayer(Int playerIndx);
Expand Down
36 changes: 28 additions & 8 deletions Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ ScriptDialog::ScriptDialog(CWnd* pParent /*=nullptr*/)
: CDialog(ScriptDialog::IDD, pParent)
{
m_draggingTreeView = false;
m_numReadPlayerNames = 0;
//{{AFX_DATA_INIT(ScriptDialog)
// NOTE: the ClassWizard will add member initialization here
//}}AFX_DATA_INIT
Expand Down Expand Up @@ -1368,14 +1369,34 @@ void ScriptDialog::OnLoad()
m_firstTrigger = nullptr;
m_waypointBase = pDoc->getNextWaypointID();
m_maxWaypoint = m_waypointBase;
m_numReadPlayerNames = 0;
file.registerParser( "PlayerScriptsList", AsciiString::TheEmptyString, ScriptList::ParseScriptsDataChunk );
file.registerParser( "ObjectsList", AsciiString::TheEmptyString, ParseObjectsDataChunk );
file.registerParser( "PolygonTriggers", AsciiString::TheEmptyString, ParsePolygonTriggersDataChunk );
file.registerParser( "WaypointsList", AsciiString::TheEmptyString, ParseWaypointDataChunk );
file.registerParser( "ScriptTeams", AsciiString::TheEmptyString, ParseTeamsDataChunk );
file.registerParser( "ScriptsPlayers", AsciiString::TheEmptyString, ParsePlayersDataChunk );
if (!file.parse(this)) {
throw(ERROR_CORRUPT_FILE_FORMAT);
SidesList sidesBeforeImport;
sidesBeforeImport = m_sides;
ScriptList *scripts[MAX_PLAYER_COUNT];
Int count = 0;
try {

@xezon xezon Oct 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This try catch looks unnecessary. Unless DataChunkInput::parse throws?

if (!file.parse(this)) {
throw(ERROR_CORRUPT_FILE_FORMAT);
}
count = ScriptList::getReadScripts(scripts);
if (count > 1 && m_numReadPlayerNames < count) {
throw(ERROR_CORRUPT_FILE_FORMAT);
}
} catch(...) {
if (count == 0) {
count = ScriptList::getReadScripts(scripts);
}
for (Int i = 0; i < count; i++) {
deleteInstance(scripts[i]);
}
m_sides = sidesBeforeImport;

@coderabbitai coderabbitai Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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

throw;
}
pDoc->setNextWaypointID(m_maxWaypoint);

Expand All @@ -1399,8 +1420,6 @@ void ScriptDialog::OnLoad()
PolygonTrigger::addPolygonTrigger(pTrig);
}

ScriptList *scripts[MAX_PLAYER_COUNT];
Int count = ScriptList::getReadScripts(scripts);
Int i;
for (i=0; i<count; i++) {
if (scripts[i]->getScript() == nullptr && scripts[i]->getScriptGroup()==nullptr) continue;
Expand All @@ -1410,8 +1429,8 @@ void ScriptDialog::OnLoad()
} else {
Int j;
for (j=0; j<m_sides.getNumSides(); j++) {
AsciiString name = m_sides.getSideInfo(i)->getDict()->getAsciiString(TheKey_playerName);
if (name == m_readPlayerNames[j]) {
AsciiString name = m_sides.getSideInfo(j)->getDict()->getAsciiString(TheKey_playerName);
if (name == m_readPlayerNames[i]) {
curSide = j;
break;
}
Expand Down Expand Up @@ -1463,7 +1482,7 @@ void ScriptDialog::OnLoad()
}

} 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);

@coderabbitai coderabbitai Bot Oct 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.cpp

Repository: 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.

}
}

Expand Down Expand Up @@ -1633,11 +1652,12 @@ Bool ScriptDialog::ParsePlayersDataChunk(DataChunkInput &file, DataChunkInfo *in
{
ScriptDialog *pThis = (ScriptDialog *)userData;
Int numNames = file.readInt();
numNames = max(0, min(numNames, Int(MAX_PLAYER_COUNT)));

@coderabbitai coderabbitai Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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 outside 0..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

Int i;
for (i=0; i<numNames; i++) {
if (i>=MAX_PLAYER_COUNT) break;
pThis->m_readPlayerNames[i] = file.readAsciiString();
}
pThis->m_numReadPlayerNames = numNames;
DEBUG_ASSERTCRASH(file.atEndOfChunk(), ("Unexpected data left over."));

@xezon xezon Oct 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed? Many other functions also do it like that.

return true;
}
Expand Down
1 change: 1 addition & 0 deletions GeneralsMD/Code/Tools/WorldBuilder/include/ScriptDialog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ class ScriptDialog : public CDialog
Int m_maxWaypoint;

AsciiString m_readPlayerNames[MAX_PLAYER_COUNT];
Int m_numReadPlayerNames;

protected:
HTREEITEM addPlayer(Int playerIndx);
Expand Down
56 changes: 44 additions & 12 deletions GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ ScriptDialog::ScriptDialog(CWnd* pParent /*=nullptr*/)
: CDialog(ScriptDialog::IDD, pParent)
{
m_draggingTreeView = false;
m_numReadPlayerNames = 0;
m_autoUpdateWarnings = true;
//{{AFX_DATA_INIT(ScriptDialog)
// NOTE: the ClassWizard will add member initialization here
Expand Down Expand Up @@ -1535,14 +1536,34 @@ void ScriptDialog::OnLoad()
m_firstTrigger = nullptr;
m_waypointBase = pDoc->getNextWaypointID();
m_maxWaypoint = m_waypointBase;
m_numReadPlayerNames = 0;
file.registerParser( "PlayerScriptsList", AsciiString::TheEmptyString, ScriptList::ParseScriptsDataChunk );
file.registerParser( "ObjectsList", AsciiString::TheEmptyString, ParseObjectsDataChunk );
file.registerParser( "PolygonTriggers", AsciiString::TheEmptyString, ParsePolygonTriggersDataChunk );
file.registerParser( "WaypointsList", AsciiString::TheEmptyString, ParseWaypointDataChunk );
file.registerParser( "ScriptTeams", AsciiString::TheEmptyString, ParseTeamsDataChunk );
file.registerParser( "ScriptsPlayers", AsciiString::TheEmptyString, ParsePlayersDataChunk );
if (!file.parse(this)) {
throw(ERROR_CORRUPT_FILE_FORMAT);
SidesList sidesBeforeImport;
sidesBeforeImport = m_sides;
ScriptList *scripts[MAX_PLAYER_COUNT];
Int count = 0;
try {
if (!file.parse(this)) {
throw(ERROR_CORRUPT_FILE_FORMAT);
}
count = ScriptList::getReadScripts(scripts);
if (count > 1 && m_numReadPlayerNames < count) {
throw(ERROR_CORRUPT_FILE_FORMAT);
Comment on lines +1555 to +1556

@greptile-apps greptile-apps Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Failed Imports Leave Waypoint Links

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.

}
} catch(...) {
if (count == 0) {
count = ScriptList::getReadScripts(scripts);
}
for (Int i = 0; i < count; i++) {
deleteInstance(scripts[i]);
}
m_sides = sidesBeforeImport;
throw;
Comment thread
greptile-apps[bot] marked this conversation as resolved.
}
pDoc->setNextWaypointID(m_maxWaypoint);

Expand All @@ -1552,6 +1573,10 @@ void ScriptDialog::OnLoad()
REF_PTR_RELEASE(pUndo); // belongs to pDoc now.
m_sides = *TheSidesList;

for (Int sideIndex = sidesBeforeImport.getNumSides(); sideIndex < m_sides.getNumSides(); sideIndex++) {
addPlayer(sideIndex);
}

if (m_firstReadObject) {
AddObjectUndoable *pUndo = new AddObjectUndoable(pDoc, m_firstReadObject);
pDoc->AddAndDoUndoable(pUndo);
Expand All @@ -1566,8 +1591,6 @@ void ScriptDialog::OnLoad()
PolygonTrigger::addPolygonTrigger(pTrig);
}

ScriptList *scripts[MAX_PLAYER_COUNT];
Int count = ScriptList::getReadScripts(scripts);
Int i;
for (i=0; i<count; i++) {
if (scripts[i]->getScript() == nullptr && scripts[i]->getScriptGroup()==nullptr) continue;
Expand All @@ -1577,9 +1600,8 @@ void ScriptDialog::OnLoad()
} else {
Int j;
for (j=0; j<m_sides.getNumSides(); j++) {
// Using i as an index assumes that i < m_sides.getNumSides. Is that safe???
AsciiString name = m_sides.getSideInfo(i)->getDict()->getAsciiString(TheKey_playerName);
if (name == m_readPlayerNames[j]) {
AsciiString name = m_sides.getSideInfo(j)->getDict()->getAsciiString(TheKey_playerName);
if (name == m_readPlayerNames[i]) {
curSide = j;
break;
}
Expand Down Expand Up @@ -1639,7 +1661,7 @@ void ScriptDialog::OnLoad()


} 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);

@coderabbitai coderabbitai Bot Oct 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.cpp

Repository: 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 -80

Repository: 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 -30

Repository: 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.h

Repository: 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;
 		}

}
}

Expand Down Expand Up @@ -1817,12 +1839,17 @@ Bool ScriptDialog::ParsePlayersDataChunk(DataChunkInput &file, DataChunkInfo *in
readDicts = file.readInt();
}
Int numNames = file.readInt();
numNames = max(0, min(numNames, Int(MAX_PLAYER_COUNT)));

@macroscopeapp macroscopeapp Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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).

Suggested change
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).

@greptile-apps greptile-apps Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Invalid Player Counts Accepted

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.

Int i;
for (i=0; i<numNames; i++) {
if (i>=MAX_PLAYER_COUNT) break;
pThis->m_readPlayerNames[i] = file.readAsciiString();
if (readDicts) {
Dict sideDict = file.readDict();
Bool hasPlayerName;
AsciiString playerName = sideDict.getAsciiString(TheKey_playerName, &hasPlayerName);
if (!hasPlayerName || playerName != pThis->m_readPlayerNames[i]) {
return false;
}
bool nameFound = false;
for (Int j=0; j < pThis->m_sides.getNumSides(); j++) {
AsciiString name = pThis->m_sides.getSideInfo(j)->getDict()->getAsciiString(TheKey_playerName);
Expand All @@ -1834,17 +1861,22 @@ Bool ScriptDialog::ParsePlayersDataChunk(DataChunkInput &file, DataChunkInfo *in
}
}
if (nameFound == false) {
if (pThis->m_sides.getNumSides() >= MAX_PLAYER_COUNT) {
return false;
}
// This side doesn't currently exist, so add it.
pThis->m_sides.addSide(&sideDict);
SidesInfo* sides = pThis->m_sides.findSideInfo(playerName);
if (sides == nullptr) {
return false;
}
ScriptList* pList = newInstance(ScriptList);
SidesInfo* sides = pThis->m_sides.findSideInfo(pThis->m_readPlayerNames[i]);
// A script list must be created.
sides->setScriptList(pList);
// Update the dialog.
pThis->addPlayer(i);
}
}
}
pThis->m_numReadPlayerNames = numNames;
DEBUG_ASSERTCRASH(file.atEndOfChunk(), ("Unexpected data left over."));
return true;
}
Expand Down
Loading