Fix adjacency error caused by dummy unit from drones/external factories - #6863
Fix adjacency error caused by dummy unit from drones/external factories#6863lL1l1 wants to merge 13 commits into
Conversation
lL1l1
added
type: bug
area: sim
|
Great catch on the missing call to I do not fully understand the context about UEF construction drones (from kennels), these generate calls to |
|
It is, but we have a "guard dummy" unit (zxa0003) which uses the The dummy is used to save assist orders onto the drone after a drone docks. |
| -- make sure we're both finished building | ||
| if self:IsBeingBuilt() or adjacentUnit:IsBeingBuilt() then | ||
| if self:IsBeingBuilt() or adjacentUnit:IsBeingBuilt() | ||
| or adjacentUnit.Blueprint.CategoriesHash["DUMMYUNIT"] |
There was a problem hiding this comment.
By the suggested logic the dummy unit should not have adjacency.
Whats the reason to put this check into Structure?
Why not to add OnAdjacentTo OnNotAdjacentTo to the Dummy class that would return?
There was a problem hiding this comment.
^Second this suggestion. Keeping things modular means that any future changes/removals don't leave lingering logic in the base class. Additionally, as the original implementer of zxa0003 I'll say that I knew enough to get it working, but not enough to know all the details exhaustively. Somebody with more experience in the unit blueprints could remove the structure-ness of it and address the problem at the source (add a movement type, maybe?).
There was a problem hiding this comment.
Why not to add OnAdjacentTo OnNotAdjacentTo to the Dummy class that would return?
The error pops up in the script of the structure that is adjacent. Returning in the dummy class does nothing to prevent the adjacent structure unit script from running.
don't leave lingering logic in the base class
I agree, I'll try to find a different solution.
remove the structure-ness of it and address the problem at the source (add a movement type, maybe?).
I tried other motion types but the engine removes the assist order once the drone is attached to the SACU (and the dummy unit is finally attached indirectly to the SACU).
Fortunately, upon some investigation, it seems related to the "Attached" state of the unit, so we can do self:SetUnitState("Attached", false) in OnAttachedToTransport and then remove all the guard re-assignment behavior, since assist orders no longer get removed.
I haven't yet figured out how to deal with fatboy detaching its external factory when you give the dummy unit a motion type.
|
Now I understand the context more, the guard unit is introduced with: #6176 And it only applies to the construction pods of the (S)ACUs. The drones from Kennels do not use this class. They are 'regular' construction units. |
fe1b3c7 to
fa75f69
Compare
📝 WalkthroughWalkthroughThe change documents motion and attachment states, adds diagnostics for adjacency and active unit states, and updates UEF construction pod transport attachment handling. It also adds a changelog entry for the ChangesMotion and adjacency behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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
🧹 Nitpick comments (1)
lua/sim/units/StructureUnit.lua (1)
778-778: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGate and contextualize the adjacency diagnostics.
OnAdjacentToandOnNotAdjacentTocallLOGwithout a debug guard. Construction-heavy games can write one message for every skipped adjacency. The messages also omit both entity IDs and theIsBeingBuilt()result. Use the existing debug logging path, or an equivalent guard, and include the skipped units and condition.Proposed logging change
- LOG('OnAdjacentTo return') + self:DebugLog( + 'OnAdjacentTo return; adjacent=' .. tostring(adjacentUnit.EntityId) + .. '; selfBeingBuilt=' .. tostring(self:IsBeingBuilt()) + .. '; adjacentBeingBuilt=' .. tostring(adjacentUnit:IsBeingBuilt()) + ) ... - LOG('OnNotAdjacentTo return') + self:DebugLog( + 'OnNotAdjacentTo return; adjacent=' .. tostring(adjacentUnit.EntityId) + .. '; selfBeingBuilt=' .. tostring(self:IsBeingBuilt()) + .. '; adjacentBeingBuilt=' .. tostring(adjacentUnit:IsBeingBuilt()) + )Also applies to: 821-821
🤖 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/StructureUnit.lua` at line 778, Update the diagnostic LOG calls in OnAdjacentTo and OnNotAdjacentTo to use the existing debug-logging guard/path, preventing them from running during normal construction. When enabled, include both involved entity IDs and each unit’s IsBeingBuilt() result in the adjacency messages.
🤖 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/Enums.lua`:
- Line 56: Resolve the `RULEUMT_Special` entry in the alias by either
documenting its intended behavior with a definitive comment and corresponding
repository support, or removing it from the public alias if it is unused and not
part of the contract; do not leave the unresolved “Identical to None?” note.
---
Nitpick comments:
In `@lua/sim/units/StructureUnit.lua`:
- Line 778: Update the diagnostic LOG calls in OnAdjacentTo and OnNotAdjacentTo
to use the existing debug-logging guard/path, preventing them from running
during normal construction. When enabled, include both involved entity IDs and
each unit’s IsBeingBuilt() result in the adjacency messages.
🪄 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: d328a2ad-85f8-4c44-97e7-06d2d2d83909
📒 Files selected for processing (8)
changelog/snippets/fix.6863mdengine/Core/Blueprints/UnitBlueprint.luaengine/Core/Categories.luaengine/Enums.lualua/sim/Unit.lualua/sim/units/StructureUnit.lualua/sim/units/components/DebugUnitComponent.lualua/sim/units/uef/TConstructionPodUnit.lua
| ---| "RULEUMT_Hover" | ||
| ---| "RULEUMT_AmphibiousFloating" | ||
| ---| "RULEUMT_Special" | ||
| ---| "RULEUMT_Special" # Identical to "None"? |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'RULEUMT_(Special|None)' .Repository: FAForever/fa
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Engine files likely containing RULEUMT handling =="
rg -n -C 3 'case[[:space:]]+UNIT_MOTION_TYPE|UnitMotionType|MotionType|RULEUMT_Special|UNIT_MOTION_TYPE_SPECIAL|UNIT_MOTION_TYPE_NONE' engine lua src include --glob '*.cpp' --glob '*.h' --glob '*.lua' --max-count 80
echo "== Enums.lua surrounding aliases =="
sed -n '40,60p' engine/Enums.luaRepository: FAForever/fa
Length of output: 17395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '(^|/)([^/]+\.(cpp|h|hpp|cc|c|inc|lua)|[^/]*_unit\.bp)$' | {
rg -n 'UNIT_MOTION_TYPE|RULEUMT_Special|RULEUMT_None|UnitMotionType|MotionType' || true
} | head -n 200
echo "== Lua direct comparisons =="
python3 - <<'PY'
import subprocess, pathlib, re
files=list(pathlib.Path('.').rglob('*.lua'))
for p in files:
txt=p.read_text(errors='ignore')
if re.search(r"['\"]RULEUMT_(Special|None)['\"]", txt):
for i,line in enumerate(txt.splitlines(),1):
if re.search(r"['\"]RULEUMT_(Special|None)['\"]", line):
print(f"{p}:{i}:{line.strip()}")
PY
echo "== Blueprint direct comparisons =="
python3 - <<'PY'
import pathlib, re
for p in pathlib.Path('.').rglob('*_unit.bp'):
txt=p.read_text(errors='ignore')
if re.search(r"['\"]RULEUMT_(Special|None)['\"]", txt):
for i,line in enumerate(txt.splitlines(),1):
if re.search(r"['\"]RULEUMT_(Special|None)['\"]", line):
print(f"{p}:{i}:{line.strip()}")
PYRepository: FAForever/fa
Length of output: 26696
🌐 Web query:
FAForever RULEUMT_Special MotionType RULEUMT_None
💡 Result:
In the context of Forged Alliance Forever (FAF) and the underlying SupCom engine, RULEUMT refers to Unit Motion Types, which define how a unit moves within the game world [1][2]. While there is no standard MotionType named RULEUMT_Special or RULEUMT_None in the base engine, these terms often appear in custom modding contexts or specific blueprint configurations where developers are either defining custom behaviors or attempting to disable movement logic [1][3]. 1. MotionTypes (RULEUMT): The engine uses various constants prefixed with RULEUMT to govern physics movement, such as RULEUMT_Hover, RULEUMT_AmphibiousFloating, RULEUMT_Land, and RULEUMT_Air [2]. 2. RULEUMT_Special: This is not a native engine constant. If encountered, it is likely a custom identifier used by a specific mod to define a unique movement logic handled by custom Lua scripts. 3. RULEUMT_None: Similarly, this is not a native constant. In cases where developers want to indicate a unit has no movement capability (static structures), they typically ensure the MotionType is omitted or set to a type that the physics engine ignores, or they define a custom constant named RULEUMT_None in their local environment to explicitly force a 'no movement' state [1][3]. If you are encountering these terms in a specific mod or file, they are almost certainly custom definitions added by the mod author to override or extend the standard behavior [3]. You can verify this by checking the local Enums.lua or unit blueprint files within that specific mod repository [1][4].
Citations:
- 1: https://github.com/FAForever/fa/blob/develop/engine/Core/Blueprints/UnitBlueprint.lua
- 2: https://github.com/FAForever/fa/blob/develop/lua/ScenarioFramework.lua
- 3: https://github.com/FAForever/etfreeman-db/blob/main/README.md
- 4: https://github.com/FAForever/fa/tree/deploy/fafdevelop/engine
Document RULEUMT_Special before exposing it.
RULEUMT_Special is listed in this alias, but no repository blueprint or Lua comparison uses it, and the current comment reads as unresolved. Either describe its intended behavior or remove the alias if it is not part of the public contract.
🤖 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/Enums.lua` at line 56, Resolve the `RULEUMT_Special` entry in the
alias by either documenting its intended behavior with a definitive comment and
corresponding repository support, or removing it from the public alias if it is
unused and not part of the contract; do not leave the unresolved “Identical to
None?” note.
5 participants
Description of the proposed changes
Fixes #6425.
Testing done on the proposed changes
The t2 mass fab is placed here so that it triggers the adjacency for the dummy unit of the fatboy. Using debug logging it shows that the dummy unit is ignored correctly.
You can also trigger the adjacency by having the UEF engineer SACU build any structures.
Checklist
Summary by CodeRabbit
Bug Fixes
OnAdjacentToerrors involving dummy units attached to external factories and UEF engineering drones.Documentation
Diagnostics