perf(productionupdate): Simplify and correct implementations of cancel functions in ProductionUpdate

[xezon]

• Follow up for bugfix(productionupdate): Prevent cancelling and refunding a single production entry when a number of its units already finished production #2399

This change simplifies the implementations of ProductionUpdate::cancelUnitCreate and ProductionUpdate::cancelUpgrade so that internally they can operate straight on the ProductionEntry pointer instead of looking for it in the list again by its ID.

And it reverts ProductionUpdate::cancelAndRefundAllProduction closer to what it did originally to prevent potential CRC mismatches (if cancelUpgrade would fail somehow) after #2399.

TODO

• Replicate in Generals
• Test against many replays

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Simplify production cancellation and preserve CRC-safe cleanup

🐞 Bug fix ✨ Enhancement 🕐 20-40 Minutes

AI Description

• Cancels queued units and upgrades directly from their production entries.
• Returns cancellation outcomes through the upgrade cancellation interface.
• Restores head-first cleanup behavior to reduce CRC divergence after cancellation failures.

Diagram

graph TD
  Bulk["Bulk Cleanup"] --> Head["Queue Head"] --> Cancel["Direct Entry Cancel"] --> Success{"Cancellation Succeeds?"}
  API["Public Cancel API"] --> Lookup["Queue Lookup"] --> Cancel
  Success -->|Yes| Refund["Refund and Remove"]
  Success -->|No| Policy["CRC Failure Policy"]

Loading

High-Level Assessment

The chosen approach is appropriate: public callers keep stable identifier/template-based APIs, while internal traversals use existing ProductionEntry pointers to avoid redundant searches. Retaining lookup-based internal cancellation would add unnecessary queue scans, and the restored head-first bulk loop better preserves deterministic CRC behavior when cancellation fails.

Files changed (2) +95 / -71

Bug fix (1) +90 / -68

ProductionUpdate.cppCancel production entries directly with CRC-safe bulk cleanup +90/-68

Cancel production entries directly with CRC-safe bulk cleanup

• Moves unit and upgrade cancellation logic into pointer-based helpers while preserving public queue lookup behavior. Bulk cancellation now repeatedly processes the queue head and explicitly handles failures according to CRC compatibility mode.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp

Refactor (1) +5 / -3

ProductionUpdate.hExpose upgrade cancellation results and entry-based helpers +5/-3

Expose upgrade cancellation results and entry-based helpers

• Changes the upgrade cancellation interface to return success or failure. Declares protected overloads that cancel units and upgrades directly from ProductionEntry pointers.

GeneralsMD/Code/GameEngine/Include/GameLogic/Module/ProductionUpdate.h

[greptile-apps]

Greptile Summary

This PR refactors production cancellation in both Generals variants to operate directly on queue entries, returns cancellation success for upgrades, and adjusts complete-queue cleanup to preserve retail-compatible behavior.

• Adds protected pointer-based cancellation helpers while retaining the public ID/template interfaces.
• Avoids redundant queue searches at internal call sites.
• Makes cancel-and-refund processing repeatedly consume the queue head and explicitly handles failed cancellation.
• Keeps the Generals and GeneralsMD implementations synchronized.

Confidence Score: 5/5

The PR appears safe to merge; no actionable regression or outstanding repository-rule violation was identified.

Internal callers supply live entries from the owning production queue, the public signature change is consistent across all implementations and callers, and the revised failure handling does not establish a new incorrect state.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Include/GameLogic/Module/ProductionUpdate.h	Changes upgrade cancellation to return success and declares internal queue-entry cancellation helpers.
Generals/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp	Centralizes unit and upgrade cancellation around live queue-entry pointers and revises complete-queue cleanup.
GeneralsMD/Code/GameEngine/Include/GameLogic/Module/ProductionUpdate.h	Mirrors the Generals interface and helper declaration changes.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp	Mirrors the Generals pointer-based cancellation and cleanup implementation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Cancellation request] --> B{Public or internal caller?}
    B -->|Public| C[Locate matching queue entry]
    B -->|Internal| D[Use current queue entry]
    C --> E{Entry found?}
    E -->|No| F[Return FALSE]
    E -->|Yes| D
    D --> G{Cancellation allowed?}
    G -->|No| F
    G -->|Yes| H[Refund production cost]
    H --> I[Remove entry from queue]
    I --> J[Delete entry]
    J --> K[Clear player upgrade state when applicable]
    K --> L[Return TRUE]

Loading

_{Reviews (2): Last reviewed commit: "Replicate in Generals" | Re-trigger Greptile}

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Players lose queued-production refunds 🐞 Bug ≡ Correctness

Description

cancelAndRefundAllProduction returns immediately when the head player upgrade cannot be cancelled,
leaving every later production entry untouched. When hasUpgradeInProduction is false for that head
entry, selling or destroying the producer eventually deletes the remaining entries without issuing
their promised refunds.

Code

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp[R1180-1182]

+#if RETAIL_COMPATIBLE_CRC
+				// Cannot cancel the head production... this loop is stuck now and can quit.
+				return;

Evidence

The helper explicitly fails when a queued player upgrade is absent from the player's in-production
state, and the newly added retail branch returns instead of visiting later entries. The method
promises to refund each production item, is called during selling, and the destructor merely deletes
any queue entries left behind, proving that those later items lose their refunds.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp[1050-1060]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp[1151-1187]
GeneralsMD/Code/GameEngine/Source/Common/System/BuildAssistant.cpp[1576-1581]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp[203-216]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`cancelAndRefundAllProduction` returns on a failed head-upgrade cancellation in retail-compatible builds, so all later queue entries remain unrefunded. Preserve the failed entry if required for compatibility, but continue cancelling and refunding subsequent entries safely.
## Issue Context
`cancelUpgrade(ProductionEntry*)` can return `FALSE` when a player upgrade is no longer marked in production. Selling calls this method before object destruction, whose destructor removes leftover queue entries without refunding them.
## Fix Focus Areas
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp[1155-1196]
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp[1050-1060]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[Skyaero42]

I'll see if I can run some replays against this.

[Caball009]

Can the title be simplified too? :)

[xezon]

Can the title be simplified too? :)

Simplified :)

[Skyaero42]

I'll see if I can run some replays against this.

4,000 replays run. No issues.
haven't reviewed the code itself yet.

[Skyaero42]

Code looks fine to me.

[xezon]

Replicated in Generals with 1 conflict.

D:\Projects\TheSuperHackers\GeneralsGameCode>FOR /F "delims=" %b IN ('git merge-base --fork-point main') DO git diff %b  1>changes.patch

D:\Projects\TheSuperHackers\GeneralsGameCode>git diff 0b9cf8b3e4be1dfc894ff8afe82c1afd98c10886  1>changes.patch

D:\Projects\TheSuperHackers\GeneralsGameCode>git apply -p2 --directory=Generals --reject --whitespace=fix changes.patch
Checking patch Generals/Code/GameEngine/Include/GameLogic/Module/ProductionUpdate.h...
Checking patch Generals/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp...
Hunk #6 succeeded at 1042 (offset -4 lines).
error: while searching for:
{
        // Empirically, in release the code can loop forever.  So we limit to 100 passes. jba. [8/31/2003]
        const Int productionLimit = 100;// With luck, we never queue up 100 units. [8/31/2003]

        Int i = 0;
        ProductionEntry *production = m_productionQueue;
        while( production != nullptr && i < productionLimit )
        {
                ProductionEntry *nextProduction = production->m_next;

                if( production->getProductionType() == PRODUCTION_UNIT )
                {
                        if( !cancelUnitCreate( production->getProductionID() ) )
                        {
                                removeFromProductionQueue( production );
                                deleteInstance( production );
                        }
                }
                else if( production->getProductionType() == PRODUCTION_UPGRADE )
                        cancelUpgrade( production->getProductionUpgrade() );
                else
                {
                        // unknown production type
                        DEBUG_CRASH(( "ProductionUpdate::cancelAndRefundAllProduction - Unknown production type '%d'", production->getProductionType() ));
                        return;
                }

                production = nextProduction;
                ++i;
        }
}


error: patch failed: Generals/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp:1145
Applied patch Generals/Code/GameEngine/Include/GameLogic/Module/ProductionUpdate.h cleanly.
Applying patch Generals/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp with 1 reject...
Hunk #1 applied cleanly.
Hunk #2 applied cleanly.
Hunk #3 applied cleanly.
Hunk #4 applied cleanly.
Hunk #5 applied cleanly.
Hunk #6 applied cleanly.
Rejected hunk #7.