AISkirmishPlayer::processBaseBuilding() base building fails to select automatic structures and mishandles GLA rebuild holes

[diqezit]

Prerequisites

• I have searched for similar issues and confirmed this is not a duplicate

Game Version

• Command & Conquer Generals
• Command & Conquer Generals: Zero Hour
• Other (please specify below)

Bug Description

Note: In shipped AIData.ini skirmish build lists, most structures use AutomaticallyBuild = No (only CommandCenter uses Yes). Bug 1 is latent in vanilla gameplay -- shipped skirmish scripts handle building construction via priority path. Bug primarily affects modded build lists or maps that add AutomaticallyBuild = Yes entries or omit that field (default m_automaticallyBuild = true).

AISkirmishPlayer::processBaseBuilding() has two interacting logic errors in its build-list scan. First error passes wrong template to buildability gate: calls canMakeUnit(dozer, bldgPlan) using accumulated candidate (often nullptr) instead of current entry template (curPlan), blocking selection for build-list entries that rely on AutomaticallyBuild = Yes or default m_automaticallyBuild value when field is omitted. Second error occurs when a destroyed GLA building is replaced by a rebuild hole: build-list entry objectID is updated to hole, but outer bldg pointer is not re-synced in that pass (due to a shadowed local bldg and already looked up old ID), allowing a hole entry to be treated as "missing" and potentially selected for construction, which can lead to duplicate/shifted rebuilds and loss of hole reference. Bug 1 currently masks Bug 2 for automatic-build path because canMakeUnit(dozer, nullptr) rejects entries before they can become candidates; fixing Bug 1 alone increases likelihood that Bug 2 will manifest and can make GLA AI rebuild behavior worse than current state.

File: GeneralsMD/Code/GameEngine/Source/GameLogic/AISkirmishPlayer.cpp

Problem

1) Buildability check uses accumulated candidate instead of current entry

Build list loop evaluates each entry to find best building to construct. For automatic entries code checks whether dozer can build structure before considering it as a candidate:

if (TheBuildAssistant->canMakeUnit(dozer, bldgPlan) != CANMAKE_OK) {
    if (info->isBuildable()) {
        AsciiString bldgName = info->getTemplateName();
        bldgName.concat(" - Dozer unable to build - money or technology missing.");
        TheScriptEngine->AppendDebugMessage(bldgName, false);
    }
    continue;
}

Check passes bldgPlan (accumulated best candidate shared across entire loop) instead of curPlan (template for entry currently being evaluated). bldgPlan starts as nullptr each pass and only gets assigned after this check succeeds:

if (info->isBuildable())
{
    if (bldgPlan == nullptr) {
        bldgPlan = curPlan;    // never reached when bldgPlan starts null
        bldgInfo = info;
    }
}

BuildAssistant::canMakeUnit() has a hard null guard:

if( builder == nullptr || whatToBuild == nullptr )
    return CANMAKE_NO_PREREQ;

canMakeUnit(dozer, nullptr) always fails. continue fires before bldgPlan = curPlan can execute. Same pattern repeats for each automatic entry: canMakeUnit(dozer, nullptr) fails and candidate assignment is skipped.

Debug message misleads: reports "money or technology missing" when actual cause is a null template pointer passed to check.

When a priority entry exists earlier in list, it sets bldgPlan to its own template before automatic entries get evaluated. Subsequent automatic entries then run canMakeUnit(dozer, priorityTemplate) which tests whether dozer can build priority building, not one actually being considered. However, when a priority candidate already exists, candidate assignment if (bldgPlan == nullptr) { bldgPlan = curPlan; } is always skipped regardless of gate result, so automatic entries cannot override priority candidate. Wrong-template check is therefore harmless in this scenario -- only no-prior-priority case produces observable consequences.

2) GLA rebuild hole entry not blocked from candidate selection

When a GLA building gets destroyed, code detects this (findObjectByID returns null for old ID) and scans all game objects for a matching rebuild hole. If found, it updates build list entry:

if (priorID == spawnerID) {
    DEBUG_LOG(("AI Found hole to rebuild %s", curPlan->getName().str()));
    info->setObjectID(obj->getID());    // objectID now points to hole
}

Outer bldg variable used later to decide whether to skip entry remains nullptr. Adding bldg = obj at this point would not fix problem because of variable shadowing: inner Object *bldg declared at start of if (info->getObjectID() != INVALID_ID) block captures any assignment within that scope and outer variable stays untouched.

bldg = TheGameLogic->findObjectByID( info->getObjectID() );     // OUTER bldg -- nullptr (destroyed)

if (info->getObjectID() != INVALID_ID) {
    Object *bldg = TheGameLogic->findObjectByID( info->getObjectID() );   // INNER bldg shadows outer
    if (bldg==nullptr) {
        // ... find hole, set objectID = holeID ...
        // any "bldg = obj" here assigns to INNER, not OUTER
    }
}   // INNER goes out of scope

if (bldg) {
    continue;       // checks OUTER -- still nullptr, does NOT skip
}

After inner scope ends, two guards that should prevent entry from becoming a candidate both fail:

• if (info->getObjectID() == INVALID_ID && ...) fails because objectID now holds holeID, not INVALID_ID
• if (bldg) { continue; } fails because outer bldg still holds nullptr

Entry passes into candidate selection as if no building exists at that location.

This also bypasses rebuild delay (RebuildDelayTimeSeconds, 30 seconds in shipped AIData.ini): objectTimestamp was set before hole scan, but delay guard requires objectID == INVALID_ID as first condition -- now false because objectID holds holeID. Entry reaches candidate selection without waiting for intended delay.

GLA rebuild holes are fully autonomous. RebuildHoleBehavior::update() waits for a configurable delay, spawns an unselectable worker, worker calls ai->construct() to rebuild original structure at hole position, hole masks itself during reconstruction, and on completion both hole and worker get destroyed. processBaseBuilding() has no role in this process.

When AI dozer gets dispatched to same location, hole object (KINDOF_IMMOBILE) blocks placement at original position. buildStructureWithDozer() position adjustment logic (with 120-cell search radius for skirmish AI) finds alternate spot nearby. Result: two copies of same building -- one at original position from hole worker, one shifted nearby from dozer.

Build list entry then gets overwritten with dozer-built building ID, losing hole reference. When hole reconstruction completes, AIPlayer::onStructureProduced() cannot re-associate rebuilt structure with build list because expected hole ID no longer exists in any entry.
Hole-scan loop also lacks a break after a match, causing unnecessary full-object iteration.

How the two bugs interact

Bug 1 masks Bug 2 in automatic-build path. When bldgPlan holds nullptr, canMakeUnit(dozer, nullptr) rejects every automatic entry including hole entries before they reach candidate selection. Fixing Bug 1 alone changes check to canMakeUnit(dozer, curPlan) which evaluates correct template and may return CANMAKE_OK for hole entries. This increases likelihood that Bug 2 manifests for GLA when a build-list structure is destroyed and gating conditions pass.

Why This Matters

Bug 1 -- all factions, whenever build lists rely on AutomaticallyBuild = Yes or default value

AI has three paths to select a building for construction:

Path	Selection mechanism	Affected
Priority builds	isPriorityBuild() set by script calls to buildSpecificAIBuilding()	Works
Power plants	Separate powerPlan override when underpowered	Works
Automatic builds	canMakeUnit gate then bldgPlan = curPlan	Blocked / unreliable

Third path handles structures not explicitly requested by skirmish scripts, or build list entries marked for automatic building.

Observable impact depends on how thoroughly skirmish scripts use buildSpecificAIBuilding() to cover build list and how AutomaticallyBuild is configured. In shipped AIData.ini, only CommandCenter has AutomaticallyBuild = Yes and all other buildings are covered by scripts via priority path, so bug is latent in vanilla gameplay. Bug primarily affects modded build lists or maps where additional entries use AutomaticallyBuild = Yes (or omit field); in those cases buildings relying on automatic selection may never be selected/built.

In theory GLA could be more affected than USA or China because GLA has no power mechanic and power-plant override path provides no fallback. In shipped data, skirmish scripts cover GLA buildings via priority path, so this difference does not manifest in vanilla gameplay.

Bug 2 -- GLA faction, when rebuild holes are present after buildings get destroyed

When active (e.g. after Bug 1 fix, and under conditions where hole entry reaches candidate selection), destruction of a GLA AI building from build list can produce duplicate/shifted rebuilds:

• Hole worker rebuilds at original position (intended, autonomous)
• AI dozer may build a copy at a shifted position nearby due to hole blocking placement (unintended)
• Build list entry can lose track of hole (ID overwritten), corrupting future rebuild cycles
• Resources wasted on duplicate structures
• Dozer time spent on redundant building instead of other tasks

In principle, repeated destruction/rebuild cycles could degrade base layout and consume rebuild counts prematurely. In shipped data, only CommandCenter has both AutomaticallyBuild = Yes and Rebuilds > 0, and Bug 1 further masks this path by rejecting hole entries before they can become candidates. Risk is primarily relevant for modded build lists where additional GLA structures use AutomaticallyBuild = Yes with Rebuilds > 0.

When This Happens

Bug 1 is present on every call to AISkirmishPlayer::processBaseBuilding(), but only has practical impact for build list entries that reach automatic-build path (entries with AutomaticallyBuild = Yes, or where field is omitted and default applies). In shipped vanilla AIData.ini most entries use AutomaticallyBuild = No, so visible effect depends on build list data and script coverage. Where automatic entries are used, AI may fail to select/build structures intended to be constructed automatically.

Bug 2 is masked in automatic-build path by Bug 1 (because canMakeUnit(dozer, nullptr) rejects entries before they can become candidates). After fixing Bug 1, hole entries are more likely to pass buildability gate, increasing chance that Bug 2 manifests. Issue occurs in pass where a destroyed building objectID is updated to a rebuild hole ID, but outer bldg pointer is still stale lookup result (nullptr). This creates a short window (often a single processBaseBuilding() pass) where hole entry can be treated as "missing" and selected for construction. On subsequent passes, findObjectByID(holeID) returns hole object and entry is correctly skipped. However, a single pass can be sufficient to dispatch a dozer and begin a duplicate/shifted rebuild (depending on isLocationSafe(), dozer availability, and resources).

Technical Details

Bug 1 loop trace (automatic-build path, no prior priority candidate)

Preconditions:

• info->isAutomaticBuild() == true
• isLocationSafe(...) passes
• a dozer is available

Step	bldgPlan (accumulated)	Check performed	Result
Loop start	nullptr	--	No candidate selected yet
Automatic entry 1	nullptr	canMakeUnit(dozer, nullptr) = CANMAKE_NO_PREREQ	continue -- candidate assignment skipped
Automatic entry 2	nullptr	canMakeUnit(dozer, nullptr) = CANMAKE_NO_PREREQ	continue -- candidate assignment skipped
Automatic entry N	nullptr	canMakeUnit(dozer, nullptr) = CANMAKE_NO_PREREQ	continue -- candidate assignment skipped
End of loop	nullptr	--	No automatic candidate selected

Bug 1 loop trace (priority candidate already selected earlier)

Step	bldgPlan (accumulated)	Check performed	Result
Priority entry (e.g. Barracks)	Barracks template	Priority path sets bldgPlan = curPlan	Candidate is now Barracks
Later automatic entry (e.g. Refinery)	Barracks template	canMakeUnit(dozer, bldgPlan) checks Barracks instead of Refinery	Wrong template validated for current entry

Note: In this scenario, wrong-template check has no practical effect: candidate assignment if (bldgPlan == nullptr) is always false when a priority candidate exists, so automatic entries cannot become candidates regardless of gate outcome. Only no-prior-priority case produces observable consequences.

---

Bug 2 state trace (destroyed GLA building replaced by a rebuild hole)

Note: This trace shows how entry can slip past "already built" skip in same pass where objectID is updated to hole. Whether a duplicate is actually started still depends on later gates (isLocationSafe, dozer availability, and for automatic path -- Bug 1 buildability gate unless fixed).

Point in code	info->objectID	Outer bldg value	Outcome
Start of entry processing	destroyedBuildingID	nullptr	findObjectByID(destroyedBuildingID) returns null
Inner scope opens	destroyedBuildingID	nullptr	Inner Object* bldg declared -- shadows outer bldg
Hole is found	holeID	nullptr	info->setObjectID(holeID) -- outer bldg not updated
Inner scope closes	holeID	nullptr	Inner bldg goes out of scope -- outer still nullptr
Rebuild-delay guard	holeID	nullptr	info->objectID == INVALID_ID is false -- delay guard does not run
"Already built" guard	holeID	nullptr	if (bldg) continue fails -- outer bldg still null
After guards	holeID	nullptr	Entry proceeds as if missing -- may reach candidate selection

Bug 2 on subsequent passes

Point in code	info->objectID	Outer bldg value	Outcome
Start of entry processing	holeID	hole object	findObjectByID(holeID) returns hole
"Already built" guard	holeID	hole object	if (bldg) continue succeeds -- entry skipped

Bug 2 exists only in pass where hole is first discovered and objectID is updated, but outer bldg still holds stale lookup result. Subsequent passes behave correctly until hole completes reconstruction.

Suggested Fix

Three changes in AISkirmishPlayer::processBaseBuilding(), all within existing build list loop. No other files modified.

Bug 1: Use current entry template (curPlan) in dozer buildability gate instead of accumulated candidate bldgPlan:

			if (TheBuildAssistant->canMakeUnit(dozer, curPlan)!=CANMAKE_OK) {

Bug 2 (break): Stop scanning once matching rebuild hole is found:

						if (priorID == spawnerID) {
							DEBUG_LOG(("AI Found hole to rebuild %s", curPlan->getName().str()));
							info->setObjectID(obj->getID());
							break;
						}

Bug 2 (re-sync): After if (info->getObjectID() != INVALID_ID) { ... } block where objectID may change to INVALID_ID or a hole ID, re-fetch outer bldg before "already built" skip guard:

			// objectID may have changed above -- re-sync before skip guard
			bldg = TheGameLogic->findObjectByID(info->getObjectID());
			if (bldg) {
				continue; // already built or rebuild hole exists
			}

If entry is updated to point at a rebuild hole, bldg becomes hole object and entry is skipped for that pass. This also resolves rebuild delay bypass -- if (bldg) continue fires before delay guard is reached, so entry with a hole is skipped regardless of objectTimestamp state

Reproduction Steps

Bug 1 (any faction)

Note: In vanilla AIData.ini most structures use AutomaticallyBuild = No, so to make this reliably reproducible set AutomaticallyBuild = Yes for at least one non-CommandCenter structure in relevant SkirmishBuildList.

1. Start a Skirmish match versus any AI (any difficulty)
2. Set AutomaticallyBuild = Yes for a non-CC structure in AI SkirmishBuildList (e.g. AmericaWarFactory) and ensure scripts are not forcing it via priority
3. Let match run long enough for base building to progress (or use AI debug output)
4. Observe that structures relying on automatic-build path are not selected/built
5. In debug builds, note AI may log "Dozer unable to build - money or technology missing" even when AI has sufficient resources and prerequisites (because canMakeUnit receives a null template pointer)

Bug 2 (GLA, easiest to reproduce after applying Bug 1 fix only)

1. Apply Bug 1 fix (use curPlan in canMakeUnit check) without Bug 2 fixes
2. Start a Skirmish match versus GLA AI
3. Destroy a GLA build-list structure that produces a rebuild hole. In vanilla AIData.ini only GLACommandCenter has AutomaticallyBuild = Yes and Rebuilds > 0; to reproduce with other structures (e.g. Arms Dealer, Barracks, Palace) set AutomaticallyBuild = Yes and Rebuilds > 0 for those entries
4. Confirm a rebuild hole appears at destroyed location
5. Move units away so area becomes "safe" (to increase chance isLocationSafe() passes)
6. Observe AI dozer/worker attempting to rebuild same structure near hole (due to placement being blocked at exact hole position)
7. After completion, observe duplicate/shifted structures

Additional Context

• Bug 2 fixes (re-syncing outer bldg pointer after objectID may change, and breaking out of hole-scan loop after first match) are safe to apply independently. They only ensure that build list entries pointing at an existing rebuild hole are treated as "object exists" and skipped, preventing hole entries from reaching candidate selection in same pass they are discovered. If objectID was not modified, re-fetch returns same object pointer and does not change behavior

• Bug 1 fix (using curPlan in canMakeUnit check) should not be applied without Bug 2 fixes. Fixing Bug 1 increases chance that rebuild-hole entries pass buildability gate and can reach candidate selection, which can lead to duplicate/shifted rebuilds for GLA in cases where a build-list structure is destroyed and location is considered safe. This can make GLA rebuild behavior worse than current state where Bug 1 masks hole-handling issue in automatic-build path

Files confirming Bug 1 (canMakeUnit)

• Common/BuildAssistant.cpp
  Confirms failure mechanism. BuildAssistant::canMakeUnit() begins with a hard null guard: if (builder == nullptr || whatToBuild == nullptr) return CANMAKE_NO_PREREQ; -- passing nullptr (which bldgPlan initially is) guarantees a failure and blocks construction loop

• GameLogic/SidesList.cpp (and Common/BuildListInfo.h)
  BuildListInfo constructor confirms m_automaticallyBuild flag defaults to true, meaning entries that omit AutomaticallyBuild field will enter automatic-build path. In shipped AIData.ini most entries explicitly set AutomaticallyBuild = No; default is relevant for modded or custom build lists that omit field

Files confirming Bug 2 (GLA holes)

• GameLogic/Module/RebuildHoleBehavior.cpp
  Confirms autonomy of rebuild hole. RebuildHoleBehavior::update() shows that hole counts down its own timer, spawns a worker, masks itself, and autonomously calls ai->construct() to rebuild structure. processBaseBuilding() should not interfere with this process

• GameLogic/AIPlayer.cpp
buildStructureWithDozer() confirms position adjustment: first check (NO_ENEMY_OBJECT_OVERLAP) passes for friendly hole, second check (NO_OBJECT_OVERLAP) fails because hole is KINDOF_IMMOBILE, code then searches nearby positions within 120-cell radius for skirmish AI. When alternate position is found, info->setObjectID() overwrites hole ID with new duplicate building ID. onStructureProduced() then cannot re-associate hole-rebuilt structure with build list because expected hole ID no longer exists in any entry