Skip to content

Conversation

@Strogoo
Copy link
Contributor

@Strogoo Strogoo commented Jan 23, 2026

Engie will continue reclaiming even if both storages are full.

Additional context

requiers FAForever/FA-Binary-Patches#130

1.mp4

Summary by CodeRabbit

  • New Features
    • Added Force Reclaim functionality allowing players to toggle force reclaim behavior on selected units.
    • New keyboard shortcuts for enabling and disabling Force Reclaim with shift-key variants for alternative controls.
    • Status messages display the number of affected units when Force Reclaim is toggled.

✏️ Tip: You can customize this high-level summary in your review settings.

Engie will continue reclaiming even if both storages are full.
@coderabbitai
Copy link

coderabbitai bot commented Jan 23, 2026

📝 Walkthrough

Walkthrough

This PR introduces a force reclaim feature for game units. It adds a new callback handler that filters engineers from a selection and applies force reclaim state, complemented by key bindings and UI descriptions to enable/disable this functionality via keyboard shortcuts.

Changes

Cohort / File(s) Summary
Force Reclaim Callback
lua/SimCallbacks.lua
Introduces new Callbacks.ForceReclaim(data, selection) that validates selection, filters engineers, applies reclaim state with data.Enable, and optionally displays count messages.
Force Reclaim Key Bindings & Descriptions
lua/keymap/keyactions.lua, lua/keymap/keydescriptions.lua, lua/keymap/misckeyactions.lua
Adds four key action variants (enable/disable force reclaim with normal/shift modifiers) that invoke SetForceReclaim() helper function. Includes corresponding UI descriptions and a new global function that bridges key actions to the SimCallback. Note: SetForceReclaim appears declared twice in misckeyactions.lua.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A reclaim force springs to life,
Keys dance to engineer's strife,
Engineers now heed the call,
Force reclaim conquers all! 🔧✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is minimal and lacks most required template sections: no structured description of changes, no testing details, incomplete context, and missing changelog documentation. Add a complete 'Description of the proposed changes' section, document testing performed, include changelog snippet per guidelines, and ensure all checklist items are addressed.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'adding "Force reclaim" hotkey' directly and clearly summarizes the main change: introducing a new hotkey feature for force reclaiming.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@lua/keymap/misckeyactions.lua`:
- Around line 560-562: The SetForceReclaim function always forces showMsg to
true because it uses "showMsg or true"; change the defaulting so only
nil/undefined becomes true and explicit false is preserved—modify the call
around SimCallback in SetForceReclaim to pass ShowMsg = (showMsg == nil) and
true or showMsg (or equivalent nil-coalescing logic) so that passing false
results in ShowMsg = false while nil still yields true; ensure you update the
symbol SetForceReclaim and its use of SimCallback accordingly.

In `@lua/SimCallbacks.lua`:
- Around line 829-858: The loop in Callbacks.ForceReclaim calls
unit:ForceReclaim which can be nil on deployments without the engine patch;
update the function to guard before invoking the method by checking the method
exists on each unit (or on the prototype) and only calling it when present—e.g.,
verify unit.ForceReclaim (or check the first engineer) before the for k, unit in
engineers loop and skip units lacking the method (optionally log or count
skipped units) to avoid a hard Lua error when ForceReclaim is unavailable.

Comment on lines +560 to +562
function SetForceReclaim(enable, showMsg)
SimCallback({ Func = 'ForceReclaim', Args = { Enable = enable, ShowMsg = showMsg or true} }, true)
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

showMsg default logic always forces true.

Passing false is ignored because showMsg or true always evaluates to true. If silent mode is expected, this breaks it.

✅ Proposed fix
 function SetForceReclaim(enable, showMsg)
-    SimCallback({ Func = 'ForceReclaim', Args = { Enable = enable, ShowMsg = showMsg or true} }, true)
+    local show = (showMsg ~= false)
+    SimCallback({ Func = 'ForceReclaim', Args = { Enable = enable == true, ShowMsg = show } }, true)
 end
📝 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.

Suggested change
function SetForceReclaim(enable, showMsg)
SimCallback({ Func = 'ForceReclaim', Args = { Enable = enable, ShowMsg = showMsg or true} }, true)
end
function SetForceReclaim(enable, showMsg)
local show = (showMsg ~= false)
SimCallback({ Func = 'ForceReclaim', Args = { Enable = enable == true, ShowMsg = show } }, true)
end
🤖 Prompt for AI Agents
In `@lua/keymap/misckeyactions.lua` around lines 560 - 562, The SetForceReclaim
function always forces showMsg to true because it uses "showMsg or true"; change
the defaulting so only nil/undefined becomes true and explicit false is
preserved—modify the call around SimCallback in SetForceReclaim to pass ShowMsg
= (showMsg == nil) and true or showMsg (or equivalent nil-coalescing logic) so
that passing false results in ShowMsg = false while nil still yields true;
ensure you update the symbol SetForceReclaim and its use of SimCallback
accordingly.

Comment on lines +829 to +858
do

---@param data { Enable: boolean, ShowMsg: boolean }
---@param selection Unit[]
Callbacks.ForceReclaim = function(data, selection)
-- verify selection
selection = SecureUnits(selection)
if (not selection) or TableEmpty(selection) then
return
end

-- verify we have engineers
local engineers = EntityCategoryFilterDown(categories.ENGINEER, selection)
if TableEmpty(engineers) then
return
end

for k, unit in engineers do
unit:ForceReclaim(data.Enable)
end

if data.ShowMsg then
if data.Enable == true then
print(string.format("Force reclaim ENABLED for %d engineers", table.getn(engineers)))
else
print(string.format("Force reclaim DISABLED for %d engineers", table.getn(engineers)))
end
end
end
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for ForceReclaim references in the codebase
rg -i "forcereclaim" --type lua -B 2 -A 2

Repository: FAForever/fa

Length of output: 2057


🏁 Script executed:

#!/bin/bash
# Check for similar patterns where methods are called on units in SimCallbacks
rg "unit:" lua/SimCallbacks.lua -A 1 | head -50

Repository: FAForever/fa

Length of output: 582


🏁 Script executed:

#!/bin/bash
# Look for defensive checks or error handling patterns in SimCallbacks
rg "and.*\." lua/SimCallbacks.lua | head -20

Repository: FAForever/fa

Length of output: 1653


🌐 Web query:

FAForever FA-Binary-Patches PR 130 ForceReclaim

💡 Result:

FAForever FA-Binary-Patches PR #130 is titled “sim Unit:ForceReclaim(bool)” and is currently Open (opened Jan 23, 2026) by Strogoo. [1]

What it does:

  • Adds a new Lua-exposed unit API: Unit:ForceReclaim(bool).
  • Purpose: when enabled, engineers continue reclaiming even if storage is full. [2]

Implementation notes (high level):

  • Introduces new assembly hooks and C++ support files to store/check a per-unit “force reclaim” flag and expose it to Lua. [2]

CI / review status:

  • CodeRabbit shows 1 passed check (title), and 2 warnings (missing template sections in description; docstring coverage below threshold). [2]

Sources: PR list page [1], PR #130 conversation/details [2].


🏁 Script executed:

#!/bin/bash
# Check if there are other guards for patch-dependent features in similar callbacks
rg "WARN|error|if not.*then.*return" lua/SimCallbacks.lua -B 2 -A 2 | head -40

Repository: FAForever/fa

Length of output: 1403


🏁 Script executed:

#!/bin/bash
# Search for any documentation or comments about patch dependencies in the codebase
rg -i "patch|binary|binary-patch|forcereclaim" lua/ --type lua | grep -i "comment\|--\|doc"

Repository: FAForever/fa

Length of output: 6192


Guard against missing ForceReclaim method to prevent sim crash.

unit:ForceReclaim() will cause a hard Lua error if the engine patch isn't present, since the method will be nil. This is a real risk given that FA-Binary-Patches PR #130 (which adds this method) is currently open and not yet deployed. Add a defensive check before the loop to prevent crash on mismatched deployments.

Suggested fix
+        if not (engineers[1] and engineers[1].ForceReclaim) then
+            WARN("Force reclaim is unavailable. Verify the FA-Binary-Patches update is deployed.")
+            return
+        end
+
         for k, unit in engineers do
             unit:ForceReclaim(data.Enable)
         end
🤖 Prompt for AI Agents
In `@lua/SimCallbacks.lua` around lines 829 - 858, The loop in
Callbacks.ForceReclaim calls unit:ForceReclaim which can be nil on deployments
without the engine patch; update the function to guard before invoking the
method by checking the method exists on each unit (or on the prototype) and only
calling it when present—e.g., verify unit.ForceReclaim (or check the first
engineer) before the for k, unit in engineers loop and skip units lacking the
method (optionally log or count skipped units) to avoid a hard Lua error when
ForceReclaim is unavailable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant