perf(productionupdate): Simplify implementations of ProductionUpdate::cancelUnitCreate, ProductionUpdate::cancelUpgrade and revert ProductionUpdate::cancelAndRefundAllProduction closer to what it did originally

[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 simplifies production cancellation by adding pointer-based cancellation helpers, exposing cancellation success from cancelUpgrade, and restoring deterministic head-first cleanup behavior in cancelAndRefundAllProduction.

• Avoids redundant queue searches when the ProductionEntry is already available.
• Preserves the existing public ID- and template-based cancellation entry points.
• Handles cancellation failures explicitly during bulk production cleanup.

Confidence Score: 5/5

The PR appears safe to merge; no concrete correctness, security, or repository-rule violations were identified.

The updated interface and implementation remain consistent, all new helper calls use live entries from the owning queue, and the bulk-cleanup behavior explicitly handles each cancellation result without an established regression.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Include/GameLogic/Module/ProductionUpdate.h	Changes cancelUpgrade to return Bool and declares protected ProductionEntry-based cancellation helpers.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp	Implements pointer-based cancellation and explicit, deterministic failure handling during bulk cleanup.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[cancelAndRefundAllProduction] --> B{Queue head type}
    B -->|Unit| C[cancelUnitCreate entry]
    B -->|Upgrade| D[cancelUpgrade entry]
    C -->|Success| E[Refund and remove head]
    C -->|Failure| F[Remove head without refund]
    D -->|Success| E
    D -->|Failure, retail-compatible| G[Stop cleanup]
    D -->|Failure, non-retail| F
    E --> H{Queue remains and limit not reached?}
    F --> H
    H -->|Yes| B
    H -->|No| I[Finish]

Loading

_{Reviews (1): Last reviewed commit: "perf(productionupdate): Simplify impleme..." | Re-trigger Greptile}

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

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (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

---

Context sources

Review mode: ⚖️ Balanced: This changes production cancellation APIs and refund/queue behavior with CRC-sensitive control flow across multiple paths, warranting a careful single-pass review.

Tip of the day

💡 Did you know, you can tweak Display preferences with a live preview to see your comment before it ships

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗