bugfix(ai): Fix isSupplySourceAttacked SCAN_RATE using frames instead of seconds

[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, by multiplying the SCAN_RATE by LOGICFRAMES_PER_SECOND.

This changes AI behavior, so it may break retail compatibility. It's removed with RETAIL_COMPATIBLE_CRC enabled.

Verified using a custom map where a tank attacks a supply truck. After the first hit, the tank is deleted to prevent any more attacks.

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

With change:

withsupplyfix.mp4

Without change:

withoutsupplyfix.mp4

SupplyAttack2386.zip

Included replays ran successfully.

This is a small commit, but I'm trying to get the hang of working with this codebase and the testing process

[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
Description check	✅ Passed	The description clearly explains the change to the attack-recency window, the retail compatibility condition, the related issue, and the reported test procedure.
Title check	✅ Passed	The title clearly identifies the AI bug and the incorrect use of frames instead of seconds in isSupplySourceAttacked.
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…

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] Fixes AI supply source attack detection logic.

The PR appears safe to merge; no outstanding findings remain.

Summary

The PR extends the non-retail supply-attack history window from 10 frames to 10 seconds while retaining a 10-frame refresh interval. The latest changes also exclude never-damaged supply units from attack detection in both game variants.

_{Reviews (9) · Last reviewed commit: "bugfix(ai): Ignore undamaged units in su..."}

[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

[xezon]

This is not good, because it compares against a magic value. Better add a new function to the interface that tells whether this has damage. Judging by other code, it looks like the common way to check for damage is to test the DamageInfo.in.m_sourceID, but needs verifying whether this is really reliable.

[WebbontheWeb]

I don't think I could use DamageInfo.in.m_sourceId, since it can be invalid even after damage has been taken.

For example, ScriptActions::doNamedDamage sets damageInfo.in.m_sourceID = INVALID_ID;. It also looks like it can be set to healers, so I'd have to filter those out as well.

Is having the magic value within the interface function alright, with something like this in ActiveBody.h?
virtual Bool hasLastDamageTimestamp() const override { return m_lastDamageTimestamp != 0xffffffff; }

If not, I could also create a constant for the value within ActiveBody.h:
constexpr const UnsignedInt InvalidBodyTimestamp = ~0u;

Additionally, while investigating I noticed that StealthUpdate::allowedToStealth has basically the same check as the one I added. Would it be a good idea to update that as well?
if( self->getBodyModule()->getLastDamageTimestamp() != 0xffffffff )

Sorry for all the questions

[xezon]

// A prior EA comment indicated that the intent was to look for 10 seconds into the attack history.

[xezon]

Comment is now obsolete.

[xezon]

Comment is now obsolete.

Changes made.