SHP Vehicle FireUp Modification #1894
Conversation
WalkthroughRefactors firing logic from timer-based to frame-based across infantry and unit hooks. Updates TechnoExt delayed-fire API to accept explicit weapon and frame parameters, removes FiringAnimationTimer, adds a pause-aware hook, and adjusts comparisons and rearm thresholds accordingly. Minor formatting change in WeaponType code. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Unit as UnitClass
participant Techno as TechnoExt
participant Weap as WeaponTypeClass
participant Anim as Animation
Note over Unit,Techno: Frame-based firing progression (replaces timer-driven)
Unit->>Anim: Read Animation.Value
Unit->>Unit: Compute frames, CurrentFiringFrame
Unit->>Weap: Resolve weapon & extension
Unit->>Unit: Compute frame & firingFrame
alt Fire-up active
Unit->>Techno: HandleDelayedFireWithPauseSequence(pThis, pWeapon, idx, frame, firingFrame)
alt Sequence paused
Techno-->>Unit: SkipFiring (paused)
else Frame matches firingFrame
Techno-->>Unit: Proceed to fire
end
else Fire-up inactive
Unit->>Techno: HandleDelayedFireWithPauseSequence(... for pause-only)
Techno-->>Unit: Skip or continue
end
sequenceDiagram
autonumber
participant Engine as Game Engine
participant Techno as TechnoExt
participant Hook as UnitClass_AI_AnimationPaused
Engine->>Hook: Animation tick
Hook->>Techno: Check DelayedFireSequencePaused
alt Paused
Hook-->>Engine: SkipGameCode (0x7363DE)
else Not paused
Hook-->>Engine: Continue normal flow
end
Note over Engine,Techno: On target change
Engine->>Techno: ResetDelayedFireTimer()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Ext/Infantry/Hooks.Firing.cpp(2 hunks)src/Ext/Techno/Body.cpp(2 hunks)src/Ext/Techno/Body.h(1 hunks)src/Ext/Techno/Hooks.cpp(2 hunks)src/Ext/Unit/Hooks.Firing.cpp(3 hunks)src/Ext/WeaponType/Body.cpp(1 hunks)
🔇 Additional comments (8)
src/Ext/Techno/Body.h (1)
271-271: Verify all call sites have been updated to match the new signature.The method signature change from 3 parameters to 5 parameters is a breaking API change. While the AI summary indicates updates in
Body.cpp,Hooks.cpp,Infantry/Hooks.Firing.cpp, andUnit/Hooks.Firing.cpp, please confirm that all callers ofHandleDelayedFireWithPauseSequencehave been updated to provide the newpWeaponandframeparameters.Run the following script to verify all call sites:
src/Ext/Techno/Hooks.cpp (1)
996-997: Verify ResetDelayedFireTimer() exists in TechnoExt.The refactor replaces
FiringAnimationTimer.Stop()withResetDelayedFireTimer(), which aligns with the timer-to-frame migration described in the PR summary. Ensure this method is implemented in TechnoExt.Run the following script to verify the method exists:
src/Ext/Infantry/Hooks.Firing.cpp (5)
91-91: LGTM! Variable rename improves clarity.Renaming
firingFrametofireUpmore accurately reflects that this is the base fire-up frame from the register, not the calculated firing frame. This improves readability, especially sincefiringFrameis later recalculated asfireUp + cumulativeDelay.
118-119: LGTM! Explicit frame calculations improve clarity.Extracting the current animation frame into
frameand calculatingfiringFrameupfront makes the logic much clearer and easier to maintain. This is a good refactoring that replaces implicit calculations with explicit, reusable variables.
124-124: LGTM! Simplified condition is much clearer.Replacing
Animation.Value == firingFrame + cumulativeDelaywithframe == firingFramesignificantly improves readability. The pre-calculated variables make the intent immediately clear without requiring readers to mentally evaluate the expression.
131-131: LGTM! Correct variable usage after rename.The rearm threshold check correctly uses
fireUp(the base frame) instead of the calculatedfiringFrame. This maintains the original logic: checking whether the next shot's base frame would exceed the sequence frame count, which is the right behavior for determining when to start the rearm timer.
121-121: Verify that FiringAITemp::WeaponType is initialized before use.The updated API call passes
FiringAITemp::WeaponTypeas a parameter, but its initialization is not visible in the provided code. According to the AI summary, weapon type is looked up early in the FireUp path. Please ensure this value is properly initialized before this call to avoid undefined behavior.src/Ext/Techno/Body.cpp (1)
699-699: Verify frame parameter correctness across callers.The change from
pThis->Animation.Valueto theframeparameter aligns with the timer-based to frame-based refactor described in the AI summary.Run the following script to verify all callers pass appropriate frame values:
| bool TechnoExt::HandleDelayedFireWithPauseSequence(TechnoClass* pThis, WeaponTypeClass* pWeapon, int weaponIndex, int frame, int firingFrame) | ||
| { | ||
| auto const pExt = TechnoExt::ExtMap.Find(pThis); | ||
| auto& timer = pExt->DelayedFireTimer; | ||
| auto const pWeapon = pThis->GetWeapon(weaponIndex)->WeaponType; | ||
| auto const pWeaponExt = WeaponTypeExt::ExtMap.Find(pWeapon); | ||
|
|
||
| if (pExt->DelayedFireWeaponIndex >= 0 && pExt->DelayedFireWeaponIndex != weaponIndex) |
There was a problem hiding this comment.
Add null check for pWeapon parameter.
The pWeapon parameter is used at line 687 without validation. If a caller passes nullptr, this will cause a crash.
Apply this diff to add defensive null checking:
bool TechnoExt::HandleDelayedFireWithPauseSequence(TechnoClass* pThis, WeaponTypeClass* pWeapon, int weaponIndex, int frame, int firingFrame)
{
+ if (!pWeapon)
+ return false;
+
auto const pExt = TechnoExt::ExtMap.Find(pThis);Additionally, consider validating that pWeapon matches the weapon at weaponIndex to catch caller errors early:
// After the null check above
#ifdef DEBUG
auto* pExpectedWeapon = pThis->GetWeapon(weaponIndex)->WeaponType;
if (pWeapon != pExpectedWeapon)
Debug::Log("Warning: pWeapon mismatch with weaponIndex in HandleDelayedFireWithPauseSequence\n");
#endif🤖 Prompt for AI Agents
In src/Ext/Techno/Body.cpp around lines 683-689, the pWeapon parameter is
dereferenced without a null check which can crash if nullptr is passed; add a
defensive null check at the start of HandleDelayedFireWithPauseSequence (return
early or handle error) before any use of pWeapon, and in addition (under DEBUG)
retrieve the expected weapon via pThis->GetWeapon(weaponIndex)->WeaponType and
compare it to pWeapon, logging a warning if they differ to catch caller
mismatches early.
| DEFINE_HOOK(0x7363C9, UnitClass_AI_AnimationPaused, 0x6) | ||
| { | ||
| enum { SkipGameCode = 0x7363DE }; | ||
|
|
||
| GET(UnitClass*, pThis, ESI); | ||
|
|
||
| if (TechnoExt::ExtMap.Find(pThis)->DelayedFireSequencePaused) | ||
| return SkipGameCode; | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
Add null safety check for ExtMap lookup.
ExtMap.Find(pThis) may return nullptr if the extension hasn't been created or was destroyed, leading to a null pointer dereference when accessing DelayedFireSequencePaused.
Apply this diff to add a null check:
DEFINE_HOOK(0x7363C9, UnitClass_AI_AnimationPaused, 0x6)
{
enum { SkipGameCode = 0x7363DE };
GET(UnitClass*, pThis, ESI);
-
- if (TechnoExt::ExtMap.Find(pThis)->DelayedFireSequencePaused)
+
+ auto pExt = TechnoExt::ExtMap.Find(pThis);
+ if (pExt && pExt->DelayedFireSequencePaused)
return SkipGameCode;
return 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DEFINE_HOOK(0x7363C9, UnitClass_AI_AnimationPaused, 0x6) | |
| { | |
| enum { SkipGameCode = 0x7363DE }; | |
| GET(UnitClass*, pThis, ESI); | |
| if (TechnoExt::ExtMap.Find(pThis)->DelayedFireSequencePaused) | |
| return SkipGameCode; | |
| return 0; | |
| } | |
| DEFINE_HOOK(0x7363C9, UnitClass_AI_AnimationPaused, 0x6) | |
| { | |
| enum { SkipGameCode = 0x7363DE }; | |
| GET(UnitClass*, pThis, ESI); | |
| auto pExt = TechnoExt::ExtMap.Find(pThis); | |
| if (pExt && pExt->DelayedFireSequencePaused) | |
| return SkipGameCode; | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
In src/Ext/Techno/Hooks.cpp around lines 34 to 44, the call to
ExtMap.Find(pThis) can return nullptr and the code later dereferences the result
to access DelayedFireSequencePaused; add a null-safety check immediately after
the Find call and if the result is nullptr, bail out (return or skip the logic)
to avoid a null pointer dereference, otherwise proceed to read/write
DelayedFireSequencePaused.
| const int frame = (frames - pThis->CurrentFiringFrame) / 2; | ||
| const int firingFrame = fireUp + cumulativeDelay; | ||
|
|
||
| if (frame % 2 != 0) | ||
| if (TechnoExt::HandleDelayedFireWithPauseSequence(pThis, pWeapon, weaponIndex, frame, firingFrame)) | ||
| return SkipFiring; | ||
|
|
||
| if (frame / 2 != fireUp + cumulativeDelay) | ||
| if (frame != firingFrame) | ||
| { |
There was a problem hiding this comment.
Fix frame calculation when FiringFrames is zero
For SHP vehicles we routinely have FiringFrames == 0, which makes frames become -1. In that case (frames - CurrentFiringFrame) / 2 collapses to 0 while fireUp + cumulativeDelay is the (non‑zero) frame we actually wait for. The inequality never resolves, we keep returning SkipFiring, and the unit never gets to fire.
Make the computed frame fall back to the requested firing frame whenever frames < 0, e.g.:
- const int frame = (frames - pThis->CurrentFiringFrame) / 2;
- const int firingFrame = fireUp + cumulativeDelay;
+ const int firingFrame = fireUp + cumulativeDelay;
+ const int frame = (frames >= 0)
+ ? (frames - pThis->CurrentFiringFrame) / 2
+ : firingFrame;(Optionally guard the equality check with frames >= 0 if you want to avoid redundant comparisons.)
Without this adjustment every SHP vehicle relying on FireUp breaks.
🤖 Prompt for AI Agents
In src/Ext/Unit/Hooks.Firing.cpp around lines 68 to 75, the computation that
derives a candidate frame from FiringFrames can produce a negative value (e.g.
-1 when FiringFrames==0) which breaks the firing check; change the logic so that
if frames < 0 you fallback to the requested firing frame (fireUp +
cumulativeDelay) instead of using the negative-derived value, and only perform
the equality/inequality comparison against the computed value when frames >= 0
(or use the fallback value when frames < 0).
|
Nightly build for this pull request:
This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build. |
For compatibility with Delayed fire weapons.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor