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
+57 −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
27 changes: 19 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 @@ -1374,8 +1374,16 @@ void ScriptDialog::OnLoad()
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);
// TheSuperHackers @bugfix OmarAglan Restore sides and teams when script parsing fails.
SidesList sidesBeforeImport;
sidesBeforeImport = m_sides;
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);
}
} catch(...) {
m_sides = sidesBeforeImport;
throw;
}
pDoc->setNextWaypointID(m_maxWaypoint);

Expand Down Expand Up @@ -1409,9 +1417,10 @@ void ScriptDialog::OnLoad()
curSide = m_curSelection.m_playerIndex;
} else {
Int j;
// TheSuperHackers @bugfix OmarAglan Match each imported player to the current map side by name.

@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.

Superfluous comment because no one will look back on this

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 +1472,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,13 +1642,15 @@ Bool ScriptDialog::ParsePlayersDataChunk(DataChunkInput &file, DataChunkInfo *in
{
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) {

@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.

Previously MAX_PLAYER_COUNT was a clamp, now it is a fail condition. Maybe just clamp numNames between 0 and MAX_PLAYER_COUNT?

return false;
}
Int i;
for (i=0; i<numNames; i++) {
if (i>=MAX_PLAYER_COUNT) break;
pThis->m_readPlayerNames[i] = file.readAsciiString();
}
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;
return file.atEndOfChunk();

@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:

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

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

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

Repository: 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'
done

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

}

/**
Expand Down
50 changes: 38 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 @@ -1541,8 +1541,16 @@ void ScriptDialog::OnLoad()
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);
// TheSuperHackers @bugfix OmarAglan Restore sides and teams when script parsing fails.
SidesList sidesBeforeImport;
sidesBeforeImport = m_sides;
try {
if (!file.parse(this)) {
throw(ERROR_CORRUPT_FILE_FORMAT);
}
} catch(...) {
m_sides = sidesBeforeImport;
throw;
Comment on lines +1551 to +1553

@greptile-apps greptile-apps 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.

P2 Failed imports retain script lists

When player validation rejects an import, this catch restores the sides but leaves the parsed script lists in memory. They stay allocated until another import; in a debug build, that next import also triggers the “Leftover scripts floating around” assertion. Release the pending lists when aborting. The Generals importer has the same failure path.

}
pDoc->setNextWaypointID(m_maxWaypoint);

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

// TheSuperHackers @bugfix OmarAglan Add imported players at their map indices after parsing succeeds.
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 @@ -1576,10 +1589,10 @@ void ScriptDialog::OnLoad()
curSide = m_curSelection.m_playerIndex;
} else {
Int j;
// TheSuperHackers @bugfix OmarAglan Match each imported player to the current map side by name.
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 +1652,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 +1830,21 @@ Bool ScriptDialog::ParsePlayersDataChunk(DataChunkInput &file, DataChunkInfo *in
readDicts = file.readInt();
}
Int numNames = file.readInt();
// TheSuperHackers @bugfix OmarAglan Reject player counts that cannot fit in the import array.
if (numNames < 0 || numNames > MAX_PLAYER_COUNT) {
return false;
}
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();
// TheSuperHackers @bugfix OmarAglan Validate the dictionary identity before adding a player.
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,19 +1856,23 @@ Bool ScriptDialog::ParsePlayersDataChunk(DataChunkInput &file, DataChunkInfo *in
}
}
if (nameFound == false) {
// TheSuperHackers @bugfix OmarAglan Check capacity before adding a side and using its script list.
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);
}
}
}
DEBUG_ASSERTCRASH(file.atEndOfChunk(), ("Unexpected data left over."));
return true;
return file.atEndOfChunk();
}

/**
Expand Down
Loading