bugfix: Fix issue where builders could resume completed tasks after being disabled

[Stubbjax]

This change fixes an issue introduced by #1870 that allows builders to resume an already completed task after being disabled.

When assigning a new build task to a builder, if the target building is an already-completed building, then the new task is ignored.

Before

BEFORE.mp4

After

AFTER.mp4

[greptile-apps]

Greptile Summary

This PR fixes a regression from #1870 where a builder (Dozer/Worker) could incorrectly resume a completed build task after recovering from a temporary disable (EMP, underpowered, hacked). The fix moves previous-task capture into setDisabledUntil via a new setPreviousTask method and adds an OBJECT_STATUS_UNDER_CONSTRUCTION guard in resumePreviousTask so that a finished building is never re-targeted.

• New setPreviousTask method is called in setDisabledUntil before the disable flag is set, correctly capturing task info while m_task[task] is still populated; a DOZER_TASK_INVALID guard prevents the out-of-bounds array access that was flagged in the prior review.
• resumePreviousTask restructured to only call newTask for DOZER_TASK_BUILD when the target is still under construction, clearing m_previousTask unconditionally afterward; repair and fortify tasks still do not resume after re-enabling (previously flagged regression).
• internalCancelTask cleaned up by removing the now-redundant previous-task save lines, since capture responsibility has moved to setDisabledUntil.

Confidence Score: 4/5

Safe to merge for the build-task fix specifically, but the repair and fortify task paths still do not resume after an EMP/underpowered disable, leaving builders silently idle — a pre-existing regression flagged in the prior review that this PR does not yet address.

The build-task completion guard is correct and the DOZER_TASK_INVALID out-of-bounds issue is properly resolved. The resumePreviousTask refactor, however, only handles DOZER_TASK_BUILD; a builder disabled while repairing or fortifying will silently idle after recovery instead of resuming work. This defect was noted in the previous review and remains unresolved here.

Files Needing Attention: DozerAIUpdate.cpp and WorkerAIUpdate.cpp (both Generals/ and GeneralsMD/) — specifically the resumePreviousTask function, which needs to handle DOZER_TASK_REPAIR and DOZER_TASK_FORTIFY alongside DOZER_TASK_BUILD.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp	Adds setPreviousTask call during disable to capture current task before cancellation; correctly placed before the actual task cancellation, and guarded by the not-already-disabled check.
Generals/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/DozerAIUpdate.cpp	Adds setPreviousTask with DOZER_TASK_INVALID guard (fixing prior OOB concern), restructures resumePreviousTask to check OBJECT_STATUS_UNDER_CONSTRUCTION for BUILD tasks, but silently drops REPAIR and FORTIFY tasks on resume.
Generals/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/WorkerAIUpdate.cpp	Mirror of DozerAIUpdate changes for WorkerAI; same fix and same repair/fortify task-drop regression present.
Generals/Code/GameEngine/Include/GameLogic/Module/DozerAIUpdate.h	Adds setPreviousTask pure virtual and override declarations to the interface and concrete class; correct additions.
Generals/Code/GameEngine/Include/GameLogic/Module/WorkerAIUpdate.h	Adds setPreviousTask override declaration; mirrors DozerAIUpdate.h changes correctly.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/DozerAIUpdate.cpp	Zero Hour mirror of Generals DozerAIUpdate changes; identical logic and identical repair/fortify regression.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/WorkerAIUpdate.cpp	Zero Hour mirror of Generals WorkerAIUpdate changes; identical logic and identical repair/fortify regression.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp	Zero Hour mirror of Generals Object.cpp disable-path change; correctly applies the same fix.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[setDisabledUntil called\nEMP / Underpowered / Hacked] --> B{Already disabled\nby same type?}
    B -- Yes --> Z[No action]
    B -- No --> C[setPreviousTask getCurrentTask]
    C --> D{getCurrentTask ==\nDOZER_TASK_INVALID?}
    D -- Yes --> E[Guard: return early\nNo save]
    D -- No --> F[m_previousTask = task\nm_previousTaskInfo = m_task task]
    F --> G[Disable flag set\nm_disabledTillFrame = frame]
    G --> H[Next update tick:\ninternalCancelTask called]
    H --> I[m_task task cleared\nbut m_previousTaskInfo already saved]
    I --> J[... time passes ...\nDisable lifts]
    J --> K[resumePreviousTask called]
    K --> L{m_previousTask ==\nDOZER_TASK_INVALID?}
    L -- Yes --> M[return early]
    L -- No --> N{m_previousTask ==\nDOZER_TASK_BUILD?}
    N -- No --> O[Clear m_previousTask\nBuilder stays idle\n⚠ REPAIR/FORTIFY dropped]
    N -- Yes --> P[findObjectByID target]
    P --> Q{target exists AND\nstill UNDER_CONSTRUCTION?}
    Q -- No --> R[Clear m_previousTask\nBuilder stays idle\nFix: skip completed builds]
    Q -- Yes --> S[newTask resume build]
    S --> T[Clear m_previousTask]

Loading

_{Reviews (7): Last reviewed commit: "fix: Reverse condition" | Re-trigger Greptile}

[xezon]

Should this perhaps go into DozerAIUpdate::resumePreviousTask and here it should be an assert?

[Stubbjax]

Good point. I suppose it could seeing as this is the only way it can happen. Having to resolve the target from the task info does make the code a bit more complex though.

[xezon]

This looks a bit strange. Wasn't cancelTask supposed to take of this? For example from Object::onDisabledEdge.

[Stubbjax]

Yes, but there was no distinction on how the disabling happened here. A unit entering a transport causes it to become disabled, and thus it would attempt to resume the task after exiting the transport.

[xezon]

setPreviousTask is confusing and can be removed again and

	m_previousTask = task;
	m_previousTaskInfo = m_task[task];

readded to internalCancelTask.

I tested that with the EMP test case shown in the video and it worked.

[Stubbjax]

The problem with this approach is that commanding a currently-constructing Dozer to enter a Chinook will have the Dozer attempt to resume the construction after exiting the Chinook.

[bobtista]

Yeah true. Maybe capture should not return to internalCancelTask. Could we move the capture into Object::onDisabledEdge(TRUE) before cancelTask, gated on DISABLED_UNDERPOWERED, DISABLED_EMP, DISABLED_SUBDUED, or DISABLED_HACKED? The disabled mask is set by then, the capture becomes symmetric with resumption, and it is no longer coupled to the audio-deduplication block. Could also use the !RETAIL_COMPATIBLE_CRC guard as the resume path.

[xezon]

@Stubbjax Is this now fixed up?

[Stubbjax]

@Stubbjax Is this now fixed up?

It should be.

[xezon]

&& target->testStatus(OBJECT_STATUS_UNDER_CONSTRUCTION)

[bobtista]

Yes this seems backwards, OBJECT_STATUS_UNDER_CONSTRUCTION is set while construction is unfinished and cleared when complete, so the current condition skips interrupted builds and resumes completed ones.

[xezon]

setPreviousTask is confusing and can be removed again and

	m_previousTask = task;
	m_previousTaskInfo = m_task[task];

readded to internalCancelTask.

I tested that with the EMP test case shown in the video and it worked.

[xezon]

This needs polishing.

[bobtista]

This changes behavior for DOZER_TASK_REPAIR. In non-retail-compatible builds, main attempts to reissue any saved task, but this implementation only reissues DOZER_TASK_BUILD and then clears the saved repair task. Is this what we want?
If not, could have the OBJECT_STATUS_UNDER_CONSTRUCTION check remain specific to BUILD while preserving existing non-build resumption behavior. DOZER_TASK_FORTIFY follows the same code path, but does anything use it? This applies to the mirrored implementations too.