[BOUNTY #2434] Fix GLA Cash Bounty using incorrect value for China Red Guard

[zhaog100]

Summary

Fixes the GLA Cash Bounty giving incorrect (doubled) bounty values for China Red Guards and any other unit produced in batches via QuantityModifier.

Problem

When a unit is produced in quantity (e.g. China Red Guards cost $300 for 2 units), the BuildCost on the ThingTemplate reflects the total cost for the whole batch, not the per-unit cost. Cash Bounty was using this total cost for each individual unit:

• Before fix: Level 1 Cash Bounty = 5% x $300 = $15 per Red Guard
• After fix: Level 1 Cash Bounty = 5% x $150 = $7 per Red Guard

This affected all 3 bounty levels and any other unit produced with a QuantityModifier.

Solution

In Player::doBountyForKill(), look up the victim's producer and check its ProductionUpdate module's QuantityModifiers. If the unit's template matches a quantity modifier entry with quantity > 1, divide the template cost by that quantity to get the correct per-unit cost.

Changes

• Generals/Code/GameEngine/Source/Common/RTS/Player.cpp - Added include + bounty cost correction logic
• GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp - Same fix for Zero Hour

Edge Cases Handled

• Producer destroyed before victim: Falls back to template cost (graceful degradation)
• Units without a producer (e.g. spawned by scripts): No change, uses template cost
• Units produced singly (quantity == 1): No change, uses template cost

Closes #2434

[greptile-apps]

Greptile Summary

This PR fixes a long-standing bug where GLA Cash Bounty paid out the full batch cost (e.g. $300 for a Red Guard pair) as the per-unit bounty rather than the correct per-unit cost ($150), yielding double the intended reward. The fix is applied symmetrically to both Generals/ and GeneralsMD/ (Zero Hour).

Key changes:

• getQuantityForTemplate() is added as a public method on the concrete ProductionUpdate class; it walks the producer's m_quantityModifiers list and returns the matching quantity (or 1 if not found).
• Player::doBountyForKill() (both codebases) now looks up the victim's producer, dynamic_casts to ProductionUpdate*, and divides costToBuild by the quantity when it is > 1.
• All documented edge cases (no producer, producer destroyed, quantity == 1) fall back to the original unmodified cost — no regressions expected.

Remaining non-blocking observations:

• The dynamic_cast to the concrete class couples Player to ProductionUpdate directly rather than going through ProductionUpdateInterface. The previous review comment suggested a virtual interface method; this is a cleaner long-term design but not a functional concern.
• getQuantityForTemplate() resolves template names via TheThingFactory->findTemplate() on every invocation (once per kill). For the very small number of quantity modifiers per building this is harmless in practice, but a pre-resolved pointer cache in QuantityModifier would be a minor optimisation.

Confidence Score: 4/5

• PR is safe to merge; the bug fix is correct and all edge cases degrade gracefully.
• The previous compile-blocking issue (calling a private macro-generated accessor through the interface pointer) has been resolved by adding getQuantityForTemplate() as a public method on the concrete class. The logic is sound, integer division is correct for known data values, and the fallback paths are all guarded. The only remaining items are a P2 architectural preference (interface-level virtual vs. dynamic_cast to concrete) and a minor per-call template name lookup — neither is a bug.
• No files require special attention; both Player.cpp variants and their corresponding ProductionUpdate pairs are symmetric and correct.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/Common/RTS/Player.cpp	Adds per-unit cost correction in doBountyForKill() by dynamic-casting the producer's interface pointer to the concrete ProductionUpdate class and calling getQuantityForTemplate(). Logic is correct and edge cases are gracefully handled; minor architectural concern around the dynamic_cast bypassing the interface.
GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp	Identical fix mirrored for Zero Hour (GeneralsMD); same observations apply as for the Generals variant.
Generals/Code/GameEngine/Include/GameLogic/Module/ProductionUpdate.h	Adds public getQuantityForTemplate() declaration to the concrete ProductionUpdate class. Uses #pragma once correctly; license header is valid.
GeneralsMD/Code/GameEngine/Include/GameLogic/Module/ProductionUpdate.h	Same header change mirrored for Zero Hour; correct "Zero Hour(tm)" branding in file header, #pragma once in place.
Generals/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp	Implements getQuantityForTemplate() by iterating over m_quantityModifiers and resolving template names via TheThingFactory at call time; logic is correct but template name lookup is repeated on every call rather than being cached.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp	Same implementation mirrored for Zero Hour; same template-lookup-per-call observation applies.

Sequence Diagram

sequenceDiagram
    participant K as Killer (Object)
    participant V as Victim (Object)
    participant GL as TheGameLogic
    participant PU as ProductionUpdate (Producer)
    participant TF as TheThingFactory
    participant P as Player (bounty recipient)

    K->>V: (unit killed)
    P->>V: doBountyForKill(killer, victim)
    P->>V: getTemplate()->calcCostToBuild()
    Note over P: costToBuild = batch cost from template
    P->>V: getProducerID()
    alt producerID != INVALID_ID
        P->>GL: findObjectByID(producerID)
        GL-->>P: producer (Object*)
        P->>P: dynamic_cast<ProductionUpdate*>(producer->getProductionUpdateInterface())
        alt cast succeeds
            P->>PU: getQuantityForTemplate(victim->getTemplate())
            loop each QuantityModifier
                PU->>TF: findTemplate(m_templateName)
                TF-->>PU: ThingTemplate*
                PU->>PU: isEquivalentTo(thingTemplate)?
            end
            PU-->>P: quantity (Int)
            alt quantity > 1
                P->>P: costToBuild = costToBuild / quantity
            end
        end
    end
    P->>P: bounty = ceil(costToBuild * m_cashBountyPercent)
    P->>P: getMoney()->deposit(bounty)

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/Object/Update/ProductionUpdate.cpp
Line: 1408

Comment:
**Template name lookup on every kill**

`TheThingFactory->findTemplate(it->m_templateName)` is a name-string lookup performed for every modifier entry on every kill event. For the typical case (1–2 entries per building) this is negligible, but the result is always the same for a given producer — the modifier list never changes at runtime. Consider caching the resolved `ThingTemplate*` pointers inside `QuantityModifier` (e.g. a `mutable const ThingTemplate* m_cachedTemplate`) and resolving lazily on first use to make the hot path a simple pointer comparison.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/Common/RTS/Player.cpp
Line: 2044

Comment:
**`dynamic_cast` to concrete type bypasses interface**

`dynamic_cast<ProductionUpdate*>` couples `Player::doBountyForKill()` directly to the concrete class rather than going through `ProductionUpdateInterface`. The previous review already suggested the cleaner alternative — exposing a virtual `getQuantityForTemplate` (or equivalent) on the interface itself — which would avoid the RTTI cast and allow any future alternate implementation of `ProductionUpdateInterface` to participate correctly.

The current approach is safe and falls back gracefully when the cast fails (i.e. returns `nullptr`), but an interface-level virtual method would be the preferred idiomatic pattern for this codebase. The same applies to `GeneralsMD/Code/GameEngine/Source/Common/RTS/Player.cpp` line 2444.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (2): Last reviewed commit: "fix: move quantity modifier lookup into ..." | Re-trigger Greptile}

[WWB2-account]

Units without a producer (e.g. spawned by scripts): No change, uses template cost

What if we spawn Red Guard? According to this they would still give the full template cost bounty.