Engineers start building at max build range#148
Engineers start building at max build range#148M3RT1N99 wants to merge 8 commits intoFAForever:masterfrom
Conversation
Engineers now begin construction as soon as they enter MaxBuildDistance of the build site, instead of walking all the way to the center. Two-hook system: - Hook 1 (TaskTick): Tags the navigator with build center + range after NewMoveTask, using unused SNavGoal.mPos2 fields - Hook 2 (UpdateCurrentPosition): Circular distance check signals early arrival when unit enters build range (dx²+dz² ≤ range²) Pathfinding is unaffected — the unit follows the normal optimal path and simply stops earlier. Works for all unit types (ACU, T1/T2/T3 engineers) with range scaling proportional to MaxBuildDistance.
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds assembly hook stubs and C++ handlers that tag path navigators with engineer build-range data at move-task creation and perform a circular-arrival check to force navigator arrival when within the tagged radius. Changes
Sequence Diagram(s)sequenceDiagram
participant Unit as Engineer Unit
participant HookBuild as Hook (0x5F7754)
participant NewMove as NewMoveTask
participant Tagger as TagNavigatorForBuild
participant Nav as PathNavigator
Unit->>HookBuild: prepare move / call path set
HookBuild->>NewMove: call NewMoveTask()
HookBuild->>Tagger: invoke TagNavigatorForBuild(task)
Tagger->>Nav: validate pointers, read blueprint MaxBuildDistance
Tagger->>Tagger: compute rangeCells
alt distance > (range + 3)
Tagger->>Nav: write tagged goal X/Z and range
end
Tagger->>HookBuild: return
HookBuild->>HookBuild: resume execution (continue)
sequenceDiagram
participant Nav as PathNavigator
participant HookCirc as Hook (0x5AE485)
participant Checker as CheckCircularArrival
participant State as Navigator State
Nav->>HookCirc: arrival-check point
HookCirc->>Checker: call CheckCircularArrival(pathNav)
Checker->>Nav: read tagged range and positions
Checker->>Checker: compute dx² + dz²
alt distSq ≤ range²
Checker->>Nav: clear tag
Checker->>State: zero/adjust navigator fields (mark arrived)
Checker->>HookCirc: return 1 (arrived)
else
Checker->>HookCirc: return 0 (not arrived)
HookCirc->>Nav: continue original flow
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
section/EngineerBuildRange.cpp (1)
40-43: ReuseOFF_UNIT_NAVIGATORhere instead of another0x54Cliteral.This file already depends on
MovementConfig.h, so keeping a second copy of the navigator offset just creates another place to miss the next disassembly correction.♻️ Tiny cleanup
- uint32_t navLand = *(uint32_t*)(unitAddr + 0x54C); + uint32_t navLand = *(uint32_t*)(unitAddr + OFF_UNIT_NAVIGATOR);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@section/EngineerBuildRange.cpp` around lines 40 - 43, Replace the hardcoded navigator offset 0x54C with the existing constant OFF_UNIT_NAVIGATOR (from MovementConfig.h) when computing navLand (i.e., use *(uint32_t*)(unitAddr + OFF_UNIT_NAVIGATOR)); keep the validity check (IsValidPtr(navLand)) and subsequent use of OFF_NAVLAND_PATHNAV unchanged, and update the comment to reference OFF_UNIT_NAVIGATOR instead of the literal to avoid duplicate offset definitions.include/MovementConfig.h (1)
17-23: Centralize theCAiPathNavigatoroffsets here too.
section/EngineerBuildRange.cppandsection/NavigatorCircularArrival.cppare sharing the samemCurrentPos/mGoal.mPos2contract through separate literals today. Moving those offsets into this header would make future offset corrections a one-file change instead of a two-hook sync problem.♻️ Suggested shape
// Blueprint economy `#define` OFF_BP_MAXBUILDDIST 0x564 // float MaxBuildDistance + +// CAiPathNavigator +#define OFF_PATHNAV_CURPOS 0x24 // mCurrentPos (packed HPathCell) +#define OFF_PATHNAV_GOAL_X 0x30 // mGoal.mPos1.x0 +#define OFF_PATHNAV_GOAL_Z 0x34 // mGoal.mPos1.z0 +#define OFF_PATHNAV_TAG_X 0x40 // mGoal.mPos2.x0 +#define OFF_PATHNAV_TAG_Z 0x44 // mGoal.mPos2.z0 +#define OFF_PATHNAV_TAG_RANGE 0x48 // mGoal.mPos2.x1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/MovementConfig.h` around lines 17 - 23, Centralize the CAiPathNavigator member offsets by adding named constants/macros in MovementConfig.h for the fields currently referenced by magic literals (e.g., CAiPathNavigator::mCurrentPos and CAiPathNavigator::mGoal.mPos2), then update section/EngineerBuildRange.cpp and section/NavigatorCircularArrival.cpp to include MovementConfig.h and replace the hard-coded offset literals with the new names; ensure the new symbols (e.g., OFF_CAiPathNavigator_mCurrentPos, OFF_CAiPathNavigator_mGoal_mPos2) match the pointer arithmetic/size semantics used in the callers and maintain the same integer type and alignment checks already used by IsValidPtr.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@include/MovementConfig.h`:
- Around line 17-23: Centralize the CAiPathNavigator member offsets by adding
named constants/macros in MovementConfig.h for the fields currently referenced
by magic literals (e.g., CAiPathNavigator::mCurrentPos and
CAiPathNavigator::mGoal.mPos2), then update section/EngineerBuildRange.cpp and
section/NavigatorCircularArrival.cpp to include MovementConfig.h and replace the
hard-coded offset literals with the new names; ensure the new symbols (e.g.,
OFF_CAiPathNavigator_mCurrentPos, OFF_CAiPathNavigator_mGoal_mPos2) match the
pointer arithmetic/size semantics used in the callers and maintain the same
integer type and alignment checks already used by IsValidPtr.
In `@section/EngineerBuildRange.cpp`:
- Around line 40-43: Replace the hardcoded navigator offset 0x54C with the
existing constant OFF_UNIT_NAVIGATOR (from MovementConfig.h) when computing
navLand (i.e., use *(uint32_t*)(unitAddr + OFF_UNIT_NAVIGATOR)); keep the
validity check (IsValidPtr(navLand)) and subsequent use of OFF_NAVLAND_PATHNAV
unchanged, and update the comment to reference OFF_UNIT_NAVIGATOR instead of the
literal to avoid duplicate offset definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9dedeafe-50b1-4190-bc00-88f85358f693
📒 Files selected for processing (5)
hooks/HEngineerBuildRange.cpphooks/HNavigatorCircularArrival.cppinclude/MovementConfig.hsection/EngineerBuildRange.cppsection/NavigatorCircularArrival.cpp
Move hardcoded navigator member offsets (mCurrentPos, mGoal.mPos2, mNext, mTargetPos, v26) from section files into named constants in MovementConfig.h. Replace hardcoded 0x54C with OFF_UNIT_NAVIGATOR.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@section/NavigatorCircularArrival.cpp`:
- Around line 17-36: The code trusts any positive OFF_PATHNAV_TAG_RANGE as live
but can be stale; read the tag's stored build center (OFF_PATHNAV_TAG_X and
OFF_PATHNAV_TAG_Z) from pathNav and compare them to the current goal position
(mGoal.mPos1 x/z) to ensure the tag matches the active goal; if they differ,
clear OFF_PATHNAV_TAG_RANGE (set to 0) and return 0 to reject the stale tag,
otherwise proceed with the existing distance check and one‑shot clear/arrival
behavior. Use the same casting used for curPos/tag fields when comparing
coordinates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6853ddf-0f8a-41e0-a5a6-7db23eb21c29
📒 Files selected for processing (3)
include/MovementConfig.hsection/EngineerBuildRange.cppsection/NavigatorCircularArrival.cpp
✅ Files skipped from review due to trivial changes (1)
- include/MovementConfig.h
- Add navigator offsets and function addresses to Info.txt - Add changelog entry under Gameplay section
Compare tagged build center (mPos2.x0/z0) against current goal (mPos1.x0/z0). If they differ, the tag is stale from a previous build order — clear it and skip the circular check.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@section/NavigatorCircularArrival.cpp`:
- Around line 23-30: TagNavigatorForBuild can return early from multiple paths
without clearing the shared tag field, which lets stale tags persist (e.g.,
short moves that return without clearing OFF_PATHNAV_TAG_RANGE); ensure the tag
is cleared on every non-tagging return by either writing *(int*)(pathNav +
OFF_PATHNAV_TAG_RANGE) = 0 before each early return in TagNavigatorForBuild or
perform a single unconditional clear of OFF_PATHNAV_TAG_RANGE at the start of
TagNavigatorForBuild (before the validations that cause early returns);
reference TagNavigatorForBuild, pathNav, OFF_PATHNAV_TAG_RANGE and the goal
fields OFF_PATHNAV_GOAL_X / OFF_PATHNAV_GOAL_Z when applying the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 422c1749-ae04-49d8-a589-deb51ec4e698
📒 Files selected for processing (1)
section/NavigatorCircularArrival.cpp
| // Validate tag matches the active goal. If the goal changed (e.g. new order | ||
| // interrupted the build move without SetGoal zeroing mPos2), clear stale tag. | ||
| int goalX = *(int*)(pathNav + OFF_PATHNAV_GOAL_X); | ||
| int goalZ = *(int*)(pathNav + OFF_PATHNAV_GOAL_Z); | ||
| if (buildX != goalX || buildZ != goalZ) { | ||
| *(int*)(pathNav + OFF_PATHNAV_TAG_RANGE) = 0; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -a 'EngineerBuildRange.cpp$' | head -n1)"
if [ -z "$file" ]; then
echo "EngineerBuildRange.cpp not found" >&2
exit 1
fi
echo "== TagNavigatorForBuild definition =="
rg -nP -C60 '\bTagNavigatorForBuild\s*\(' "$file"
echo
echo "== OFF_PATHNAV_TAG_RANGE writes and nearby returns =="
rg -n -C3 'OFF_PATHNAV_TAG_RANGE|return\s*;' "$file"Repository: FAForever/FA-Binary-Patches
Length of output: 108
🏁 Script executed:
# First, check if section/ directory exists and list its contents
echo "=== Files in section/ directory ==="
fd -a . section/ --max-depth 1 2>/dev/null | head -20 || echo "section/ not found or empty"
echo
echo "=== Search for any files matching '*EngineerBuild*' ==="
fd -a 'EngineerBuild' . 2>/dev/null || echo "No matches"
echo
echo "=== Search for NavigatorCircularArrival.cpp ==="
fd -a 'NavigatorCircularArrival' . 2>/dev/null || echo "No matches"
echo
echo "=== Repository root structure ==="
ls -la . | head -20Repository: FAForever/FA-Binary-Patches
Length of output: 2307
🏁 Script executed:
# Search for TagNavigatorForBuild in EngineerBuildRange.cpp
echo "=== Searching for TagNavigatorForBuild ==="
rg 'TagNavigatorForBuild' section/EngineerBuildRange.cpp -C 60
echo
echo "=== File size check ==="
wc -l section/EngineerBuildRange.cppRepository: FAForever/FA-Binary-Patches
Length of output: 5147
Clear OFF_PATHNAV_TAG_RANGE on every non-tagging return in TagNavigatorForBuild().
The function has 7 early returns without clearing the tag field. Most critically, line 81 returns early for short moves without clearing OFF_PATHNAV_TAG_RANGE. Combined with the goal-coordinate check in NavigatorCircularArrival.cpp (which only clears on goal change), this allows a short move to the same goal to inherit a stale tag from a previous longer move. Every early return must clear the tag before returning, or use an unconditional clear at the start before any validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@section/NavigatorCircularArrival.cpp` around lines 23 - 30,
TagNavigatorForBuild can return early from multiple paths without clearing the
shared tag field, which lets stale tags persist (e.g., short moves that return
without clearing OFF_PATHNAV_TAG_RANGE); ensure the tag is cleared on every
non-tagging return by either writing *(int*)(pathNav + OFF_PATHNAV_TAG_RANGE) =
0 before each early return in TagNavigatorForBuild or perform a single
unconditional clear of OFF_PATHNAV_TAG_RANGE at the start of
TagNavigatorForBuild (before the validations that cause early returns);
reference TagNavigatorForBuild, pathNav, OFF_PATHNAV_TAG_RANGE and the goal
fields OFF_PATHNAV_GOAL_X / OFF_PATHNAV_GOAL_Z when applying the fix.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
section/EngineerBuildRange.cpp (1)
44-46: Consider removing developer name from inline comment.The comment
"Martin"appears to be a leftover note. Consider cleaning it up to maintain professional code style.Suggested cleanup
- int rangeCells = (int)(maxBD * 0.9f); //"Martin" 0.9 works fine at max range + int rangeCells = (int)(maxBD * 0.9f); // 0.9 factor balances range vs diagonal safety🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@section/EngineerBuildRange.cpp` around lines 44 - 46, Remove the developer name from the inline comment next to the rangeCells calculation: clean up the comment after the expression int rangeCells = (int)(maxBD * 0.9f); by deleting the `"Martin"` note and leaving a concise, professional comment (or no comment) explaining why 0.9f is used if necessary; update any nearby comment text so only relevant rationale remains and ensure the code around rangeCells and maxBD stays unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@run_test.bat`:
- Around line 1-6: The batch file uses Unix LF endings and unquoted SET values;
convert the file to Windows CRLF line endings and update the SET lines (SRC,
DST, LOG) to quote their values (e.g., SET SRC="...") so paths with spaces are
handled correctly; remove the unused INIT variable (or quote and use it if
intended) to avoid dead code and save the file with CRLF (e.g., via editor
settings or unix2dos) to prevent cmd.exe parsing issues.
---
Nitpick comments:
In `@section/EngineerBuildRange.cpp`:
- Around line 44-46: Remove the developer name from the inline comment next to
the rangeCells calculation: clean up the comment after the expression int
rangeCells = (int)(maxBD * 0.9f); by deleting the `"Martin"` note and leaving a
concise, professional comment (or no comment) explaining why 0.9f is used if
necessary; update any nearby comment text so only relevant rationale remains and
ensure the code around rangeCells and maxBD stays unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb93c387-e137-4164-b437-c822d49a9daa
⛔ Files ignored due to path filters (1)
debug.logis excluded by!**/*.log
📒 Files selected for processing (2)
run_test.batsection/EngineerBuildRange.cpp
| @echo off | ||
| set SRC=%~dp0ForgedAlliance_exxt.exe | ||
| set DST=C:\ProgramData\FAForever\bin\ForgedAlliance_exxt.exe | ||
| set INIT=init_faf.lua | ||
| set LOG=%~dp0debug.log | ||
|
|
There was a problem hiding this comment.
Convert to Windows line endings (CRLF) and quote SET values.
The batch file uses Unix line endings (LF), which can cause parsing failures and script malfunction in Windows cmd.exe due to label handling bugs near 512-byte boundaries. Additionally, SET values should be quoted for safety with paths containing spaces. The INIT variable on line 4 is also unused.
Proposed fix
-set SRC=%~dp0ForgedAlliance_exxt.exe
-set DST=C:\ProgramData\FAForever\bin\ForgedAlliance_exxt.exe
-set INIT=init_faf.lua
-set LOG=%~dp0debug.log
+set "SRC=%~dp0ForgedAlliance_exxt.exe"
+set "DST=C:\ProgramData\FAForever\bin\ForgedAlliance_exxt.exe"
+set "LOG=%~dp0debug.log"Also ensure the file is saved with CRLF line endings (e.g., via unix2dos run_test.bat or editor settings).
📝 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.
| @echo off | |
| set SRC=%~dp0ForgedAlliance_exxt.exe | |
| set DST=C:\ProgramData\FAForever\bin\ForgedAlliance_exxt.exe | |
| set INIT=init_faf.lua | |
| set LOG=%~dp0debug.log | |
| `@echo` off | |
| set "SRC=%~dp0ForgedAlliance_exxt.exe" | |
| set "DST=C:\ProgramData\FAForever\bin\ForgedAlliance_exxt.exe" | |
| set "LOG=%~dp0debug.log" | |
🧰 Tools
🪛 Blinter (1.0.112)
[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 16 LF sequences found
(E018)
[error] 2-2: Unsafe SET command usage. Explanation: SET commands without proper validation or quoting can cause security issues. Recommendation: Always quote SET values and validate input: SET "var=safe value". Context: SET command value should be quoted for safety
(SEC002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@run_test.bat` around lines 1 - 6, The batch file uses Unix LF endings and
unquoted SET values; convert the file to Windows CRLF line endings and update
the SET lines (SRC, DST, LOG) to quote their values (e.g., SET SRC="...") so
paths with spaces are handled correctly; remove the unused INIT variable (or
quote and use it if intended) to avoid dead code and save the file with CRLF
(e.g., via editor settings or unix2dos) to prevent cmd.exe parsing issues.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Remove debug.log please. |
|
@4z0t log is gone now |
Replace inline asm .cpp wrappers with cleaner .hook files matching the project convention for pure jump trampolines.
When building a row of mass extractors diagonally, the first mex in the queue was being skipped because the rangeCells of 0.9*maxBD was too aggressive at the diagonal corner case. 0.89 keeps the straight-line range improvement while preventing the skip on the first build of a queued diagonal sequence.
2 participants
Engineers now begin construction as soon as they enter MaxBuildDistance of the build site, instead of walking all the way to the center.
Two-hook system:
Pathfinding is unaffected — the unit follows the normal optimal path and simply stops earlier. Works for all unit types (ACU, T1/T2/T3 engineers) with range scaling proportional to MaxBuildDistance.
The memo
A clear and concise description (or visuals) of what the changes imply. If it closes an issue, make sure to mention that with closing keywords.
Guide how to create an engine patch.
Checklist
Summary by CodeRabbit
New Features
Chores
Documentation