Disable air unit intel on death instead of on impact except for scout vision - #7190
Disable air unit intel on death instead of on impact except for scout vision#7190lL1l1 wants to merge 28 commits into
Conversation
if the vision marker needs a mesh or attached FX, the author should make a new subclass with altered UpdatePosition No existing uses of the vision marker rely on the y-position
lL1l1
added
area: sim
📝 WalkthroughWalkthroughAir units can retain vision while falling after death through ChangesFalling Air-Unit Vision
Blueprint Reload Guard
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engine/Sim/Entity.lua`:
- Around line 268-274: Update the radius annotations for Entity:InitIntel and
the corresponding SetIntelRadius API to match their callers: use number if
fractional radii are supported, including AirUnit’s 0.5-scaled vision values, or
consistently normalize or reject non-integer inputs before they reach these
methods. Keep the documented optional-radius behavior aligned across both APIs.
In `@lua/sim/units/AirUnit.lua`:
- Around line 222-225: Correct the misspelled type annotation on the
CreateProjectileAtBone result assigned to self.colliderProj, changing
ShiledCollider to ShieldCollider to match the referenced projectile type and
provide accurate Lua tooling information.
In `@lua/sim/VizMarker.lua`:
- Around line 136-140: Update the lifetime guard in the method containing
UpdateDuration so the optional lifetime is validated for presence before
comparing it to zero. Use the existing UpdateDuration path only when lifetime is
non-nil and non-negative, leaving duration unchanged when lifetime is omitted.
In `@lua/system/Blueprints.lua`:
- Around line 440-445: Guard the dependent-reload block around the load-order
length and dependent-loop start with the same orderNumber check used by the
precursor loop. Update the logic near TableGetn(loadOrderTable) so it only
calculates and reloads dependent entries when orderNumber exists, while safely
skipping that pass when no load-order index is available.
In `@units/ZXA0004/ZXA0004_script.lua`:
- Around line 13-16: Update the self annotation on ZXA0004.OnCreate from ZXA0003
to ZXA0004, leaving the method implementation unchanged.
- Around line 32-46: Update CopyAllIntelFrom and its local CopyIntel helper so
each intel type copied from unit is initialized and enabled on self before
applying the source radius. Preserve the existing IsIntelEnabled filtering and
radius propagation for all listed intel types.
🪄 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 Plus
Run ID: 8f01962b-5212-4cef-9dbf-7a3543761062
📒 Files selected for processing (9)
changelog/snippets/features.7190.mdengine/Core/Blueprints/UnitBlueprint.luaengine/Core/Categories.luaengine/Sim/Entity.lualua/sim/VizMarker.lualua/sim/units/AirUnit.lualua/system/Blueprints.luaunits/ZXA0004/ZXA0004_script.luaunits/ZXA0004/ZXA0004_unit.bp
| --- Initializes the entity's intel to provide intelligence of a partiuclar type for an army. | ||
| --- This lets the other intel methods work with this entity for that type of intel. | ||
| ---@param army Army | ||
| ---@param type IntelType | ||
| ---@param radius? number | ||
| ---@param type IntelType # Logs a warning if using `"None"`, but still inits intel manager. | ||
| ---@param radius? integer | ||
| function Entity:InitIntel(army, type, radius) | ||
| end |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find relevant files:"
fd -a 'Entity.lua|AirUnit.lua|VizMarker.lua' . | sed 's#^\./##'
echo
echo "Relevant sections:"
for f in engine/Sim/Entity.lua lua/sim/units/AirUnit.lua lua/sim/VizMarker.lua; do
if [ -f "$f" ]; then
echo "===== $f ====="
wc -l "$f"
sed -n '240,385p' "$f"
fi
done
echo
echo "Search InitIntel/SetIntelRadius usages:"
rg -n "InitIntel|SetIntelRadius|IntelRadius" -S . --glob '!vendor/**' --glob '!build/**' | head -200Repository: FAForever/fa
Length of output: 12605
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Entity.lua intel implementations:"
sed -n '120,380p' engine/Sim/Entity.lua
echo
echo "lua/sim/VizMarker.lua relevant sections:"
sed -n '1,180p' lua/sim/VizMarker.lua
echo
echo "lua/sim/units/AirUnit.lua relevant sections:"
sed -n '1,90p' lua/sim/units/AirUnit.lua
echo
echo "Search blueprint intel field definitions/usage:"
rg -n "VisionRadius|WaterVisionRadius|OmniRadius|JammerRadius|SonarRadius|Intel" -S engine lua --glob '!build/**' | head -250Repository: FAForever/fa
Length of output: 42483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Luau/FAF callbacks around entity intel implementations:"
git ls-files | rg '(^|/)SimCallbacks\.lua$|/moho.*Entity|engine/Sim/CollisionBeamEntity.lua' | head -50
sed -n '850,970p' lua/SimCallbacks.lua
rg -n "InitIntel|SetIntelRadius|GetIntelRadius|IntelRadius" -S lua/SimCallbacks.lua lua/AI lua/system lua/sim engine/Sim --glob '!**/generated/**' | head -80
echo
echo "AirUnit blueprint classes:"
rg -n "AirUnit.*Blueprint|Intel =|WaterVisionRadius|VisionRadius" lua/system lua/blueprints units --glob '!**/generated/**' | head -200
echo
echo "Read-only Lua static scanner for direct integer annotation vs non-integer arg patterns:"
python3 - <<'PY'
from pathlib import Path
for p in ["engine/Sim/Entity.lua","lua/sim/VizMarker.lua","lua/sim/units/AirUnit.lua"]:
text=Path(p).read_text()
print(f"===== {p} =====")
for i,line in enumerate(text.splitlines(),1):
if "Entity:InitIntel" in line or "Entity:SetIntelRadius" in line or "IntelRadius" in line or "`@field` Radius" in line or "`@param` radius" in line or ".Intel." in line:
print(f"{i}: {line}")
PYRepository: FAForever/fa
Length of output: 19286
Align intel-radius parameter types with the callers.
Entity.InitIntel/SetIntelRadius are annotated as integer, but lua/sim/VizMarker.lua forwards radius: number values and lua/sim/units/AirUnit.lua passes 0.5 * blueprintIntel.VisionRadius/WaterVisionRadius. Either document these APIs as number when fractional radii are valid, or normalize/reject non-integer values consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@engine/Sim/Entity.lua` around lines 268 - 274, Update the radius annotations
for Entity:InitIntel and the corresponding SetIntelRadius API to match their
callers: use number if fractional radii are supported, including AirUnit’s
0.5-scaled vision values, or consistently normalize or reject non-integer inputs
before they reach these methods. Keep the documented optional-radius behavior
aligned across both APIs.
|
I would entertain the idea of actually removing this feature, as it doesnt matter for most of the air units. And is kinda OP for scouts where they get intel on things after getting show down. That would maybe even require them to not be so fragile as they are right now, and could lead to a different balance. And for this change, if the problem with that 1 tick delay is causing by unit dying - intel entity getting created. How about creating this entity when the unit is constructed and then just enabling the intel on death? |
0.45ms sim time just with an acu fully zoomed out on 20km map (since vfx cause sim lag) test with 1500 ASF: before change: 5.3-5.9ms idle, 9.5-10.5 on initial patrol order, 8.7-9.3 on second after change: 7.0-7.7ms idle, 11-12.9ms on first patrol order, 9.9-10.7ms second patrol order, 10.9-11.5ms on third patrol order
This reverts commit 6fde48b.
|
I did some testing and got quite a big performance hit from spawning 2x the units for all air units, so I don't like the simple answer of just giving every air unit an intel dummy. Written in the commit description:
The average patrol is +17% sim time taken up. On idle its +31%. With how conservatively air units are played, they don't die for quite a long time and build up, so keeping the intel dummies with these performance hits is quite bad. |
|
I see that you requested my review, but I'm moving on from FAForever. You don't have to request it in the future. More on-topic: adding a dummy unit to every air unit is a mistake. You also came to this conclusion. Not only is that a ~2ms increase, which will be much higher for fewer units on weaker hardware. You also increase memory consumption (modded games run into this issue quite quickly). You increase complexity of the code base with this solution. And you increase the likely hood for bugs, like how existing scripts can accidentally get a handle to this dummy unit when trying to find units in an area.
Just because something is commonly used does not make it a good feature. The only correct fix in my humble opinion is to stop intel when a unit dies, for any type of unit. The case you describe also applies to ships that sink I would assume. Now the behavior is consistent and aligns with what the engine is trying. Then you adjust the balance so that scouts don't die instantly. This can be by giving them more health, or by increasing their altitude so that it takes longer (and is more difficult) for projectiles to hit. edit: another solution would be an engine patch, where units even after being killed still render their intel rings until they are destroyed. Then you double down on the behavior, which is fine too. |
|
Ok I understand. good luck in your future endeavors. Wish you a less stressful environment. Yeah the performance impact is quite large, I forgot to mention that I have a powerful 7800x3d so 1500 units wasn't even an unrealistic number of units relative to my capabilities. |
by default scout category units have it enabled
|
I updated the PR to disable all intel for most air units on death instead of on impact, with the exception of vision intel for scouts. I think this is a good compromise for the balance, immersion, performance, and player-pov understandability. |
lL1l1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lua/sim/units/AirUnit.lua`:
- Around line 224-235: The falling-death vision setup in the AirUnit vision
block ignores the blueprint’s death-specific radius and duration. Update the
VisionMarkerOpti configuration around EnableVisionWhileFalling to use
self.Blueprint.Intel.VisionRadiusOnDeath and apply the configured
IntelDurationOnDeath via the marker’s duration API; if this vision is
intentionally permanent until sinking or destruction instead, remove or document
those stale death-specific blueprint fields.
🪄 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 Plus
Run ID: 2c0e2661-e7d2-40eb-8492-936c88e545aa
📒 Files selected for processing (13)
changelog/snippets/balance.7190.mdchangelog/snippets/features.7190.mdengine/Core/Blueprints/UnitBlueprint.lualua/sim/units/AirUnit.lualua/system/blueprints-units.luaunits/UAA0101/UAA0101_unit.bpunits/UAA0302/UAA0302_unit.bpunits/UEA0101/UEA0101_unit.bpunits/UEA0302/UEA0302_unit.bpunits/URA0101/URA0101_unit.bpunits/URA0302/URA0302_unit.bpunits/XSA0101/XSA0101_unit.bpunits/XSA0302/XSA0302_unit.bp
🚧 Files skipped from review as they are similar to previous changes (1)
- changelog/snippets/features.7190.md
| if not self.Blueprint.Air.EnableVisionWhileFalling then | ||
| self:DisableIntel('Vision') -- Disable vision seperately, it's not handled in DisableUnitIntel | ||
| else | ||
| -- Create a vision entity so vision shading is visible | ||
|
|
||
| local x, y, z = self:GetPositionXYZ(0) | ||
| ---@type VisionMarkerOpti | ||
| local vizEnt = self.Trash:Add(VisionMarkerOpti({ Army = army })) | ||
| vizEnt:UpdateIntel(army, self:GetIntelRadius("Vision"), "Vision", true) | ||
| vizEnt:UpdatePosition(x, z) | ||
| vizEnt:AttachTo(self, 0) | ||
| end |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'IntelDurationOnDeath|VisionRadiusOnDeath|EnableVisionWhileFalling|UpdateDuration|GetIntelRadius' \
engine lua unitsRepository: FAForever/fa
Length of output: 22047
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## AirUnit.lua context"
sed -n '180,245p' lua/sim/units/AirUnit.lua
echo
echo "## VizMarker lifecycle context"
sed -n '1,220p' lua/sim/VizMarker.lua
echo
echo "## Blueprint parser/deadlife context"
sed -n '600,660p' lua/system/blueprints-units.lua
sed -n '1040,1095p' engine/Core/Blueprints/UnitBlueprint.lua
echo
echo "## Vision field occurrences and assignments in lua/engine data"
rg -n -C 3 'EnableVisionWhileFalling|VisionRadiusOnDeath|IntelDurationOnDeath|OnKilled|DisableUnitIntel|DisableUnitIntel|KillUnit|death|Killed|Die' lua/sim/units/XSB*.lua lua/sim/units/UA*.lua lua/sim/units/UE*.lua lua/sim/units/UR*.lua lua/sim/AirUnit.lua lua/system/blueprints-units.lua engine/Core/Blueprints/UnitBlueprint.lua | sed -n '1,260p'Repository: FAForever/fa
Length of output: 22034
Honor the death-specific vision settings or remove their stale contract.
EnableVisionWhileFalling creates VisionMarkerOpti, but line 232 uses self:GetIntelRadius("Vision") instead of self.Blueprint.Intel.VisionRadiusOnDeath, and it leaves UpdateDuration unset while IntelDurationOnDeath = 20 exists for this lifecycle. If death vision is meant to remain until sink or destruction, document/remove those death fields; otherwise pass the death radius and duration to the marker.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/sim/units/AirUnit.lua` around lines 224 - 235, The falling-death vision
setup in the AirUnit vision block ignores the blueprint’s death-specific radius
and duration. Update the VisionMarkerOpti configuration around
EnableVisionWhileFalling to use self.Blueprint.Intel.VisionRadiusOnDeath and
apply the configured IntelDurationOnDeath via the marker’s duration API; if this
vision is intentionally permanent until sinking or destruction instead, remove
or document those stale death-specific blueprint fields.
4 participants
Description of the proposed changes
Intel for air units is not disabled upon death, instead it goes away when the unit is Destroyed. This is especially relevant for air units since they fly fast and take a long time falling to the ground after they die and before they are Destroyed.
This behavior of air units is commonly used on purpose so removing it with a fix is not permissible in my opinion. Instead, I made air units keep their intel range rings and vision shading using a dummy intel unit. Adding this intel dummy makes the mechanic people have been using more understandable for all players.
Testing done on the proposed changes
Make sure the intel dummy gives identical intel to the air unit when impacting on both land and water (water causes sinking instead of destruction).
Here is how it looks:
air.vision.dummy.mp4
One issue is that the overlay appears after 1 tick. I tried a workaround by putting the intel dummy creation in a
Kill()hook, but that didn't work. I guess the only way to do it would be to pre-allocate intel dummies and move them into place when necessary and disable them when not. It's probably doable in a maintainable way although it is a bit of complex work so I haven't gotten around to it.A potential issue is that this is enabled for all units and could be unusual with things like the czar:
air.vision.dummy.czar.mp4
People could get used to it or we could disable it for non-scouts. I don't like disabling it for non-scouts because there are situations where you use fighters for immediate intel instead of waiting for scouts, so I'd rather people get used to it.
Additional context
Other units use
DisableUnitIntel('Killed')in OnKilled, which disables everything except vision and water vision.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation