bugfix(ai): Fix supply attack detection timing and unrecorded damage/healing timestamp handling

[WebbontheWeb]

• Fixes SCAN_RATE in AIPlayer::isSupplySourceAttacked() uses frames instead of seconds, causing AI to "forget" attacks after 0.33 seconds #2386

Updates AIPlayer::isSupplySourceAttacked() to check within the last 10 seconds, instead of the last 10 frames.

It splits SCAN_RATE into RefreshRate and ScanWindow, the RefreshRate stays at 10 frames, while ScanWindow is 10 * LOGICFRAMES_PER_SECOND

Also includes updates to the damage and healing timestamp getters. Now getLastHealingTimestamp and getLastDamageTimestamp return nullptr when there's been no damage or no healing to prevent initial placeholder timestamps from being treated as recent healing or damage.

All this changes AI behavior and may break retail compatibility. It's removed with RETAIL_COMPATIBLE_CRC enabled.

Verified using a custom map where

1. Run a supply source under attack check
2. Attack a supply truck once
3. Wait 4 seconds
4. Run the supply source attack check again

After 4 seconds, a script checks whether the supply source is under attack. If true, victory. If false, defeat.

Running the supply source check twice ensures that RefreshRate is being handed separately than ScanWindow.

Before updates:

withoutsupplyfix.mp4

After updates:

newsupplyfix.mp4

Map:

SupplyAttack2386.zip

Included replays ran successfully.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 4612069c-d695-40f5-b134-ec1ba11e69e4

📥 Commits

Reviewing files that changed from the base of the PR and between 7d340d2 and 63f40b9.

📒 Files selected for processing (2)

• Generals/Code/GameEngine/Source/GameLogic/AI/AIPlayer.cpp
• GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIPlayer.cpp

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

Walkthrough

Both game variants set the supply-attack scan interval to 10 frames in RETAIL_COMPATIBLE_CRC builds and to 10 seconds in other builds. The interval also controls the associated attack-recency and damage-timestamp checks.

Changes

Supply attack scan interval

Layer / File(s)	Summary
Set the build-dependent scan interval Generals/Code/GameEngine/Source/GameLogic/AI/AIPlayer.cpp, GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIPlayer.cpp	Both implementations use a 10-frame interval in RETAIL_COMPATIBLE_CRC builds and a 10-second interval otherwise. The selected interval applies to the surrounding attack-recency and damage-timestamp checks.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: caball009

Merge Risk: ⚪ Minimal · up to 63f40

Non-CRC builds now retain supply attacks for 10 seconds, while CRC-compatible builds preserve their prior behavior. No actionable merge-blocking risk remains; the change is ready for normal checks.

Architecture Summary

Architecture risk: 🔵 Low · up to 63f40

The change affects 2 systems.

Changed systems: Generals, GeneralsMD

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — Generals (service) was modified; 1 changed file maps to changed impact.
• observed — GeneralsMD (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in Generals/Code/GameEngine/Source/GameLogic/AI/AIPlayer.cpp: AIPlayer::isSupplySourceAttacked now uses a 10-frame scan interval under RETAIL_COMPATIBLE_CRC and a 10-second interval otherwise, replacing the previous unconditional 10-frame interval. The selected interval also governs the surrounding attack-recency and damage-timestamp checks.
• observed — Modified behavior in GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIPlayer.cpp: isSupplySourceAttacked now uses a 10-frame scan interval when RETAIL_COMPATIBLE_CRC is enabled; otherwise it uses 10 logic-seconds. This replaces the previous unconditional 10-second interval.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	Issue #2386 requires AIPlayer::isSupplySourceAttacked() to retain attacks for 10 seconds, or 300 logic frames, in Generals and Zero Hour. Both changed files use 10*LOGICFRAMES_PER_SECOND only when…	Use 10*LOGICFRAMES_PER_SECOND for SCAN_RATE in the RETAIL_COMPATIBLE_CRC configuration, or explicitly update the linked issue to exclude that configuration.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The pull request changes only AIPlayer::isSupplySourceAttacked() in the Generals and Zero Hour source files. The build conditional relates to the stated retail-compatibility constraint. No unrelated…
Title check	✅ Passed	The title describes the supply attack timing change, which is the main change. The timestamp-handling clause is not reflected in the provided file summaries.
Description check	✅ Passed	The description explains the supply attack timing change and its testing. It also describes timestamp getter changes.

Full details: Linked Issues check

Explanation

Issue #2386 requires AIPlayer::isSupplySourceAttacked() to retain attacks for 10 seconds, or 300 logic frames, in Generals and Zero Hour. Both changed files use 10*LOGICFRAMES_PER_SECOND only when RETAIL_COMPATIBLE_CRC is disabled. They still use 10 frames when the flag is enabled. Issue #2386 does not exclude that configuration. The issue states no separate automated-test requirement.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Changes how damage and healing timestamps are tracked and checked.

The PR appears safe to merge; the remaining issue is non-blocking getter documentation.

Summary

The PR separates the supply-attack scan interval from its detection window and changes damage and healing timestamp getters to represent unrecorded events with null pointers in non-retail builds.

• Both game variants update the getter callers, including AI, drawable, and containment code.
• The latest changes remove retail-compatibility qualifications from getter documentation, making that documentation inaccurate.

_{Reviews (16) · Last reviewed commit: "bugfix(ai): Remove non-retail behaviour ..."}

[Skyaero42]

This change is fine pending a few nits.

[xezon]

Comments can be restyled, otherwise makes sense.

[Caball009]

Greptile still has an open review with a concern that looks valid.

[WebbontheWeb]

Considering the original code had a comment indicating that 10 seconds was intended for SCAN_RATE, I don't believe that's an issue.

If we think it is, I could update to separate the "refresh rate" from SCAN_RATE.

m_supplySourceAttackCheckFrame could be incremented using the previous 10 frame value, while the rest could use the corrected SCAN_RATE.

[xezon]

I sent an inquiry to kabuse about this.

[xezon]

-TanSo-

yes, the condition works fine, however, exactly like presented in the PR, the 10 second delay makes the use of this script very inconsistent. The proposed solution would be a good implementation. I have set the refresh time to 2 seconds on my own build and run with it just fine

[WebbontheWeb]

Thanks for the feedback, it's been updated. I split SCAN_RATE into REFRESH_RATE (how often isSupplySourceAttacked can be checked) and SCAN_WINDOW (how far back it's checking). I wasn't sure how to keep the EA comment, since it doesn't accurately describe either of the variables, so I moved it above with a disclaimer.

I also updated my map script to verify that the refresh is tracked separately from the scan. Before the tank attacks it now checks whether the supply truck has been attacked. It checks again 4 seconds after the attack.

The previous version doesn't register that it has been attacked (defeat):

previoussupplyfix.mp4

While the updated version does (victory):

newsupplyfix.mp4

Map:

SupplyAttack2386.zip

Changes made.

[xezon]

What started as a small fix, became a broader fix. The change title and description needs updating.

[xezon]

This makes sense to me.