Document engine-side of Weapon:CanFire#7045
Conversation
📝 WalkthroughWalkthroughThis PR adds documentation for weapon firing logic across three files. A changelog entry documents engine-side behavior of UnitWeapon:CanFire(), inline documentation for the NeedToComputeBombDrop field is clarified, and an extended comment block detailing CanFire() logic and conditions is added to the implementation. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/Core/Blueprints/WeaponBlueprint.lua`:
- Around line 215-217: The comment for the optional field NeedToComputeBombDrop?
is ambiguous and reads like a fragment; update its docstring to explicitly state
that this boolean controls whether the unit is marked as "busy" during the
weapon's charge/reload/fire cycle (true = unit is set busy and cannot perform
other actions; false = weapon activity does not mark the unit busy). Locate the
doc comment above the NeedToComputeBombDrop? field in WeaponBlueprint.lua and
replace the current wording with this clear, self-contained explanation
referencing the charge/reload/fire phases.
In `@engine/Sim/UnitWeapon.lua`:
- Around line 26-40: The pseudo-code in the UnitWeapon:CanFire comment has a
typo and an inconsistent call style; fix "reutrn" to "return" and make method
calls consistent (use colon form for instance methods like unit:IsStunned(),
unit:IsUnitState("Busy"), unit:IsUnitState("Immobile"),
unit:IsUnitState("MakingAttackRun") and keep unit:_GetSpeed() if it’s private;
ensure weapon calls use consistent naming like weapon:_HasTarget() and
weapon:_UnitIsInBombDropZone() and return weapon._AimOnTarget where indicated;
also clarify the Winged check to read "if not unit.Blueprint.Air.Winged then
return weapon._AimOnTarget end" and keep comments about BombDropThreshold and
PredictAheadForBombDrop unchanged.
| ---@field NeedToComputeBombDrop? boolean | ||
| --- if the unit is set as "busy" while the weapon charges | ||
| --- If the unit is set as "busy" while the weapon charges/reloads/fires. Set to `true` to allow | ||
| --- other weapons to fire/unit actions to occur while this weapon is working. |
There was a problem hiding this comment.
Clarify the “busy” wording to avoid ambiguity.
The current phrasing reads like a sentence fragment and can be interpreted as contradictory. Consider making it explicit that this field controls whether the unit is set as “busy” during charge/reload/fire.
✏️ Suggested wording tweak
---- If the unit is set as "busy" while the weapon charges/reloads/fires. Set to `true` to allow
---- other weapons to fire/unit actions to occur while this weapon is working.
+--- Controls whether the unit is set as "busy" while the weapon charges/reloads/fires. Set to `true` to allow
+--- other weapons to fire/unit actions to occur while this weapon is working.📝 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.
| ---@field NeedToComputeBombDrop? boolean | |
| --- if the unit is set as "busy" while the weapon charges | |
| --- If the unit is set as "busy" while the weapon charges/reloads/fires. Set to `true` to allow | |
| --- other weapons to fire/unit actions to occur while this weapon is working. | |
| ---@field NeedToComputeBombDrop? boolean | |
| --- Controls whether the unit is set as "busy" while the weapon charges/reloads/fires. Set to `true` to allow | |
| --- other weapons to fire/unit actions to occur while this weapon is working. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/Core/Blueprints/WeaponBlueprint.lua` around lines 215 - 217, The
comment for the optional field NeedToComputeBombDrop? is ambiguous and reads
like a fragment; update its docstring to explicitly state that this boolean
controls whether the unit is marked as "busy" during the weapon's
charge/reload/fire cycle (true = unit is set busy and cannot perform other
actions; false = weapon activity does not mark the unit busy). Locate the doc
comment above the NeedToComputeBombDrop? field in WeaponBlueprint.lua and
replace the current wording with this clear, self-contained explanation
referencing the charge/reload/fire phases.
| --- `UnitWeapon:CanFire` calls this engine `CanFire` function that is used in the firing cycle, alongside | ||
| --- the self-explanatory `HasTarget`, `HasSiloAmmo`, and `TargetSolutionStatusGun() == TRS_Available` calls. | ||
| --- | ||
| --- Here is the list of conditions for the engine `CanFire`, written in pseudo-lua with underscores ("_") for engine fields. | ||
| --- It can return early in the list, as indicated by the return statements. If no return is reached, it returns false. | ||
| --- !unit:IsStunned() | ||
| --- !unit:IsUnitState("Busy") | ||
| --- !unit.Blueprint.Air.CanFly or unit.Layer == "Air" | ||
| --- !unit.Blueprint.AI.NeedUnpack or unit:IsUnitState("Immobile") | ||
| --- Check if the weapon's below/above water firing restrictions. | ||
| --- if !unit.Blueprint.Air.Winged then return weapon._AimOnTarget end -- aim on target in terms of firing tolerance | ||
| --- !weapon.Blueprint.AutoInitiateAttackCommand or unit:_GetSpeed() >= 0.25 * unit.Blueprint.MaxAirSpeed * unit._SpeedMult | ||
| --- if !weapon.Blueprint.NeedToComputeBombDrop or !weapon:_HasTarget() then return weapon._AimOnTarget end | ||
| --- unit.IsUnitState("MakingAttackRun") | ||
| --- if weapon:_UnitIsInBombDropZone() then reutrn weapon._AimOnTarget end --Involves weapon BombDropThreshold and unit PredictAheadForBombDrop |
There was a problem hiding this comment.
Fix typos/inconsistent call style in pseudo-code.
There’s a misspelling and one method call that deviates from the rest of the block, which can confuse readers or lead to copy/paste errors.
✏️ Suggested doc fixes
---- Check if the weapon's below/above water firing restrictions.
+--- Check if the weapon meets below/above water firing restrictions.
---- unit.IsUnitState("MakingAttackRun")
+--- unit:IsUnitState("MakingAttackRun")
---- if weapon:_UnitIsInBombDropZone() then reutrn weapon._AimOnTarget end --Involves weapon BombDropThreshold and unit PredictAheadForBombDrop
+--- if weapon:_UnitIsInBombDropZone() then return weapon._AimOnTarget end -- Involves weapon BombDropThreshold and unit PredictAheadForBombDrop🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/Sim/UnitWeapon.lua` around lines 26 - 40, The pseudo-code in the
UnitWeapon:CanFire comment has a typo and an inconsistent call style; fix
"reutrn" to "return" and make method calls consistent (use colon form for
instance methods like unit:IsStunned(), unit:IsUnitState("Busy"),
unit:IsUnitState("Immobile"), unit:IsUnitState("MakingAttackRun") and keep
unit:_GetSpeed() if it’s private; ensure weapon calls use consistent naming like
weapon:_HasTarget() and weapon:_UnitIsInBombDropZone() and return
weapon._AimOnTarget where indicated; also clarify the Winged check to read "if
not unit.Blueprint.Air.Winged then return weapon._AimOnTarget end" and keep
comments about BombDropThreshold and PredictAheadForBombDrop unchanged.
Description of the proposed changes
Documents the engine-side details of Weapon:CanFire. This is relevant when working with air units and understanding how the busy state interacts with weapons.
Testing done on the proposed changes
UnitWeapon::CanFireat.text:006D4C80.Additional Context
This stems from testing with restorer and looking in the default projectile weapon script to see why its AA missiles can't fire, and then seeing that NotExclusive was poorly documented.
Checklist
Summary by CodeRabbit