Conversation
|
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:
📝 WalkthroughWalkthroughRemoved the old RangeRings implementation and its asm hooks; added a new AlliedRangeRings module with intel-aware range logic and an asm trampoline; fixed pointer precedence and a public typedef in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Renderer
participant ASMWrapper as asm__ShouldAddUnit
participant Decision as ShouldAddUnit
participant Blueprint
participant Map
participant AllyCheck as Army_IsAlly
Renderer->>ASMWrapper: invoke range-check
ASMWrapper->>Decision: preserve registers & call ShouldAddUnit
Decision->>Blueprint: query intel ranges (omni/radar/sonar)
Decision->>Map: call map_has_unit (unit presence)
Decision->>AllyCheck: check ally status
AllyCheck-->>Decision: ally result
Map-->>Decision: presence result
Blueprint-->>Decision: intel ranges
Decision-->>ASMWrapper: return boolean
ASMWrapper-->>Renderer: branch/return based on result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
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/AlliedRangeRings.cpp`:
- Around line 38-56: Add a short explanatory comment above the selectionSize <
67 check in ShouldAddUnit that documents why 67 was chosen (e.g., engine
selection limit, practical performance cutoff, or observed behavior) so future
maintainers understand the constraint; reference the selectionSize variable and
the selection-size optimization branch that avoids double-rendering (the check
using selectionSize < 67 and the subsequent map_has_unit(session + 0x4A0,
userUnit) call) and state the source of the value (engine limit, empirical
threshold, or link to design docs) and any implications if it changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
hooks/AlliedRangeRings.cpphooks/RangeRings.cppinclude/PatchBase.hinclude/moho.hsection/AlliedRangeRings.cppsection/RangeRings.cpp
💤 Files with no reviewable changes (2)
- hooks/RangeRings.cpp
- section/RangeRings.cpp
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 `@hooks/AlliedRangeRings.hook`:
- Around line 5-6: The patch writes the address of
Moho__UserUnit__GetIntelRanges into 0x00E4D984 but the review warns the original
slot may expect a different ABI; inspect all call sites that read 0x00E4D984
(including any vtable usage or direct jumps) and confirm they invoke a
__thiscall member-style function with the exact parameter/return signature used
in the extern "C" declaration; if they do not, either change the declaration for
Moho__UserUnit__GetIntelRanges to the ABI the callers expect or add a small
thunk wrapper that adapts calling conventions/this-pointer semantics and store
the thunk address at 0x00E4D984 instead, then re-run a build/test to validate no
calling-ABI mismatches remain.
|
This would be amazing! Would it be possible to include/exclude mobile units somehow via a toggle (console command)? For example, frigates have a lot of range rings. This may clutter the screen. And personally, I use range rings as an informal indicator that these (mobile) units are mine haha. I may want to disable it for mobile units because of that. But it's not blocking in my point of view. |
|
Everything is possible, after all, we are C++ developers |
|
To me, just these two features would be useful:
In Lua I think we should default it to the new behavior, but with mobile units turned off. That way you get 95% of the benefit, without all the clutter in larger and longer teamgames. Maybe it is separate, maybe it is not: being able to see SMD/TMD range of allies. But I'd rather first merge this in and that you work on that in a separate pull request. As the value of just this change to be able to share intel is huge. |
|
I added |
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/AlliedRangeRings.cpp`:
- Line 73: Fix the typo in the inline comment that reads "// MoutionType != None
-> let's assume this is not a building" by changing "MoutionType" to
"MotionType"; search for any other occurrences of "MoutionType" in the same
source (e.g., near the AlliedRangeRings logic) and correct them to "MotionType"
so comments consistently reference the correct symbol.
|
I tihnk checking for unit motion is not enough. For example it will exclude t3 sonars from units that don't move. |
|
Yes, from a human perspective, it should be added. But I havent found an easy way to define the T3 sonar. Hardcoding the blueprint is a bad idea, imagine a new fifth faction? :D |
To me as a first version this would suffice. We can always expand and/or refine it further later down the line. An alternative check that may be trivial is instead of checking the motion type, you take the largest intel of any type (radar/sonar/omni). These fields always exist on the blueprint, even when they are 0. Compare that to some integer. If you can make that configurable with a console command (just an integer as input) then we could introduce a slider in the game options menu. You can then exclude allied units with an intel range of 90 or lower, for example. |
|
This can be done, but lets leave everything as is for now. We are approaching the 'customization hell' phase from zero seconds. Imho, considering how update changelogs are presented (usually just text when you first enter the lobby), not many people will even learn about a noticeable changes and customization options. It would be awesome to show images Honestly, I would set "all allied units" by default, but I will listen to the team leadership experience of Jip, I dont have community surveys to disagree .exe in zulip |
|
@RutreD I've made the Lua side here: FAForever/fa#7054 |
|
@4z0t is the pull request technically sound? Then we can merge this in and make it available on FAF Develop. |
|
Not really. I highly discourage to put asm and cpp code into same file. C++ code must be in |
|
One issue I noticed is allied acu has attack ring or something? |
|
Good find, looks like about 5 more hooks will be... |
|
I think we should just use option 0 and 1 for now cuz it just breaks like this 2026-03-01_00-38-55.mp4 |
That's Omni of the ACU. |
| .int Moho__UserUnit__GetIntelRanges | ||
|
|
||
| 0x007EF12B: | ||
| push esp |
There was a problem hiding this comment.
Why do you push stack pointer? Why not use lea to get proper pointer to structure on stack?
|
@4z0t , there is no need to nitpick that much. If we take this approach, then this patch is just a half-measure. You need to understand that the game is not designed for ally ranges. Although, if you're that inspired, to move forward, you would need to completely rewrite the class RangeExtractor (.rdata:00E3F918; class Moho::RangeExtractor: (#classinformer)). However, the work for this patch is enough to rewrite 00E3F980 (RadarExtractor), 00E3F998 (Sonar), 00E3F978 (Omni), and then you'll be able to remove Moho__UserUnit__GetIntelRanges. Ideally, you'd completely rewrite 007EEA00 ; void __userpurge RangeRenderer__Render(int wldSession@, int camera@, int rangeRenderer, int device, float deltaFrame) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
section/AlliedRangeRings.cxx (2)
84-111: MotionType heuristic has known limitations for mobile intel units.Per PR discussion, using
MotionType != 0(line 104) to identify mobile units is a simple heuristic that may not correctly handle all cases (e.g., T3 sonars that are stationary but classified differently). This is acknowledged in the PR discussion as an acceptable initial approach, with refinements deferred to later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@section/AlliedRangeRings.cxx` around lines 84 - 111, The MotionType heuristic in ShouldAddUnit (the check using *reinterpret_cast<uint32_t*>(blueprint + 0x290) != 0) can misclassify units; replace the inline comment with a clear TODO and justification: explicitly document that this is a best-effort heuristic for distinguishing buildings vs mobile units, mention known edge-cases (e.g., T3 sonars), and add a TODO to refine the check later (suggested approaches: use blueprint category flags or an explicit MotionType enum) so future readers see the limitation; keep the logic unchanged for now but update the comment near the blueprint+0x290 check and reference intelRangeBehavior and ShouldAddUnit in the TODO.
21-31: Consider limiting initial modes per reviewer feedback.Per the PR discussion, 4z0t recommended limiting options to modes 0 and 1 initially because other modes cause breakage. Mode 2 (
kOwnUnitsAndAlliedUnits) is currently available but may exhibit visual bugs with allied mobile units displaying non-intel rings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@section/AlliedRangeRings.cxx` around lines 21 - 31, Remove the problematic third mode by deleting the kOwnUnitsAndAlliedUnits enum entry from IntelRangeBehavior, keep only kOwnUnits and kOwnUnitsAndAlliedBuildings, set the default intelRangeBehavior to kOwnUnitsAndAlliedBuildings, and update the ConDescReg string (conIntelRangeBehavior) to list only modes 0 and 1; also search for and remove or guard any usage of kOwnUnitsAndAlliedUnits elsewhere in the codebase to avoid references to the removed value.
🤖 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/AlliedRangeRings.cxx`:
- Around line 99-100: Army_IsAlly is missing a declaration causing the build
failure; declare it (matching the project's pattern in global.h) with the
correct signature and calling convention and bind it to the executable address
using asm(), then include that declaration before AlliedRangeRings.cxx uses it;
reference the function name Army_IsAlly and the variables focusArmyIndex and
unitArmy so you add a prototype like the other engine functions in global.h
(e.g., bool __cdecl Army_IsAlly(int armyIndex1, int armyIndex2) asm("0x...");)
and ensure the asm address matches the engine symbol.
---
Nitpick comments:
In `@section/AlliedRangeRings.cxx`:
- Around line 84-111: The MotionType heuristic in ShouldAddUnit (the check using
*reinterpret_cast<uint32_t*>(blueprint + 0x290) != 0) can misclassify units;
replace the inline comment with a clear TODO and justification: explicitly
document that this is a best-effort heuristic for distinguishing buildings vs
mobile units, mention known edge-cases (e.g., T3 sonars), and add a TODO to
refine the check later (suggested approaches: use blueprint category flags or an
explicit MotionType enum) so future readers see the limitation; keep the logic
unchanged for now but update the comment near the blueprint+0x290 check and
reference intelRangeBehavior and ShouldAddUnit in the TODO.
- Around line 21-31: Remove the problematic third mode by deleting the
kOwnUnitsAndAlliedUnits enum entry from IntelRangeBehavior, keep only kOwnUnits
and kOwnUnitsAndAlliedBuildings, set the default intelRangeBehavior to
kOwnUnitsAndAlliedBuildings, and update the ConDescReg string
(conIntelRangeBehavior) to list only modes 0 and 1; also search for and remove
or guard any usage of kOwnUnitsAndAlliedUnits elsewhere in the codebase to avoid
references to the removed value.
|
I'm not being nitpick. I want to know why you did certain things for it to work. I don't have that part reversed either that's why I'm asking. |
|
Okay, if you want, you can share the file of your IDA base .i64 in zulip. I think I could help supplement some of these parts |
|
Your |
|
You are right. It is fixed. But I dont know how to fix this for allies (and same thing with radar toggle) |
|
Hmm, is it hard to tweak the |
|
@4z0t please check |
|
Love this, this will be great for replays too, not having to bounce between all players to figure out team intel. |
M3RT1N99
left a comment
M3RT1N99
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround on the previous review — the encapsulation of EntityAttributes (private fields + GetIntelRadius/SetIntelRadius member helpers that mirror the engine's bit-packing semantics) is a notably better solution than the call-site masking I had originally suggested. Making the helpers part of the type means future hooks cannot accidentally bypass the 0x80000000 enable bit. Adding the VALIDATE_OFFSETs for Entity, UserEntity, Unit, ReconBlip, etc. closes the offset-trust gap as well, and the live-attribute read in ShouldAddUnit addresses the dynamic intel range concern.
With those in, the only substantive remaining issue I'd like to flag is the < 67 selection-size threshold (inline below) — IDA verification of the actual ring-rendering pipeline shows it is both unnecessary and logically inverted. The other two inline notes are minor: a documentation request for the esp + 0x30 stack offset, and a question about the intentional sync scope of the SyncVisionRange hook.
Marking this as a comment review rather than request-changes since the previously-blocking high-bit issue is resolved. Verified against ForgedAlliance.exe md5 d27b16b5d9b1c0480dd92316b125448e.
|
Thats too many changes for 1 PR tbh. I won't review all of that. |
M3RT1N99
left a comment
M3RT1N99
left a comment
There was a problem hiding this comment.
Both new commits address the remaining concerns cleanly:
36c9140fremoves the< 67threshold entirely and moves the double-draw prevention intoRenderRange— which is a better placement than my suggestion of keeping it inShouldAddUnit, since it preserves the unit inmArmyUnitsInFrustumfor other consumers while only skipping the redundant ring draw.5c6fca43scopes the Radar/Sonar/Omni sync to allied units only, keeping own-unit recon blips untouched.
All correctness concerns from the IDA verification are resolved. LGTM.
6 participants
This PR adds the ability to view the intel range(radar, sonar, omni) of allied units. It introduces console command ren_IntelRangeBehavior. Lua patch that adds an option in the game settings fa#7054
Modes:
ren_IntelRangeBehavior 0: The default behavior, as before the patch. Only your own intel rings are visible.ren_IntelRangeBehavior 1: Displays your own rings and those provided by allied structures (e.g., radar and sonar), excluding T3 sonar, as it is a mobile unitren_IntelRangeBehavior 2: Displays both your own Intel rings and those of allied units.Screen
ren_IntelRangeBehavior 2Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Chores