beyond-all-reason / beyond-all-reason/RecoilEngine
CGroundMoveType::HandleUnitCollisions mixes squares and elmos when sizing its collision search radius
- Dominant language
- C++
- Stars
- 683
- Forks
- 293
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 40
Description
(The bug identification and the issue description are AI generated)
# `CGroundMoveType::HandleUnitCollisions` mixes squares and elmos when sizing its collision search radius
## Summary
The search radius used to gather collision candidates in `CGroundMoveType::HandleUnitCollisions`
adds a value counted in heightmap **squares** to a value counted in **elmos**. The term that is
supposed to widen the search for oversized collidees is therefore a factor of `SQUARE_SIZE` (8)
too small, so collisions and separation nudges against large units can be missed.
Two smaller gaps sit in the same expression (details below): the term only ranges over MoveDefs,
so static collidees' footprints are never accounted for, and only the collider's half of
`separationDistance` is included.
## The code
`rts/Sim/MoveTypes/GroundMoveType.cpp:2807`
```cpp
// Account for units that are larger than one's self.
const float maxCollisionRadius = colliderParams.y + moveDefHandler.GetLargestFootPrintSizeH();
const float searchRadius = colliderParams.x + maxCollisionRadius + colliderSeparationDist;
// ...
quadField.GetUnitsExact(qfQuery, collider->pos, searchRadius);
```
`colliderParams.y` is set by the caller at `GroundMoveType.cpp:2534`:
```cpp
const float colliderFootPrintRadius = colliderMD->CalcFootPrintMaxInteriorRadius();
```
and that helper is in elmos — note the `* SQUARE_SIZE` (`MoveDefHandler.cpp:737`):
```cpp
float MoveDef::CalcFootPrintMaxInteriorRadius(float scale) const {
return ((std::max(xsize, zsize) * 0.5f * SQUARE_SIZE) * scale);
}
```
`GetLargestFootPrintSizeH()` returns `largestSizeH` (`MoveDefHandler.h:235`), which is a max over
`MoveDef::xsizeh` / `zsizeh` (`MoveDefHandler.cpp:152-153`):
```cpp
largestSizeH = std::max(largestSizeH, moveDefs[mdCounter].xsizeh);
largestSizeH = std::max(largestSizeH, moveDefs[mdCounter].zsizeh);
```
and those are plain square counts (`MoveDefHandler.cpp:319-320`):
```cpp
// precalculated data for MoveMath
xsizeh = xsize >> 1;
zsizeh = zsize >> 1;
```
So `maxCollisionRadius` = ` + `. For a largest MoveDef footprint of 12 squares
the term contributes **6 elmos** where it should contribute **48**.
## Why this looks like a slip rather than a deliberate heuristic
Every other consumer of `GetLargestFootPrintSizeH()` treats it as squares correctly:
```cpp
// Game/GameHelper.cpp:922
const int bufferSize = (moveDefHandler.GetLargestFootPrintSizeH() + 1) * SQUARE_SIZE;
// Game/GameHelper.cpp:1337
const int largestMoveTypSizeH = moveDefHandler.GetLargestFootPrintSizeH() + 1; // used as square indices
// Sim/MoveTypes/Systems/UnitTrapCheckSystem.cpp:47
const int largestMoveTypSizeH = moveDefHandler.GetLargestFootPrintSizeH() + 1; // used as square indices
```
The sibling function `HandleFeatureCollisions` (`GroundMoveType.cpp:3022`) also stays in elmos
and does not touch `largestSizeH` at all:
```cpp
quadField.GetFeaturesExact(qfQuery, collider->pos, colliderParams.x + (colliderParams.y * 2.0f));
```
## Impact
`GetUnitsExact` accepts a candidate when (`QuadField.cpp`, `GetUnitsExact`):
```cpp
const float totRad = radius + u->radius;
if (pos.SqDistance(u->pos) >= totRad * totRad)
continue;
```
so the query already extends its reach by each candidate's **model** radius. That masks the bug
for most content: a big unit usually has a big model radius, and the missing footprint allowance
never gets noticed.
The bug becomes visible for any unit whose **MoveDef footprint radius exceeds its model radius**
— which is not exotic, since MoveDef footprints are often inflated relative to the model for
pathing clearance. For those, `HandleUnitCollisions` can fail to enumerate the collidee at all,
and the symptoms would be units clipping into or overlapping large collidees, and
`separationDistance` silently not being honoured against them.
I have not reproduced this in a running game — the report is from reading the code, so the
practical severity depends on real content and is worth confirming before acting. A cheap check
is to log, per unit at load, `footprintRadius - modelRadius` and see whether anything is
positive.
## Two related gaps in the same expression
**1. Static collidees are not covered.** `largestSizeH` ranges only over MoveDefs, but the loop
sizes non-mobile collidees with the object's own footprint (`GroundMoveType.cpp:2852`):
```cpp
const float collDist = (collideeMobile) ? collideeMD->CalcFootPrintMaxInteriorRadius()
: collidee->CalcFootPrintMaxInteriorRadius();
```
so buildings contribute nothing to the search radius even though they are tested against it.
**2. Only the collider's separation distance is included.** The search radius uses
`colliderSeparationDist`, but the separation test inside the loop uses the max of both sides
(`GroundMoveType.cpp:2864`):
```cpp
separationDist = std::max(colliderSeparationDist, collideeUD->separationDistance);
const float separation = colliderParams.y + collideeParams.y + separationDist;
```
A collidee with a larger `separationDistance` than the collider can therefore fall outside the
query.
## On fixing it
The obvious fix — multiplying the term by `SQUARE_SIZE` — is correct but pays for a lot it does
not need to, because `GetUnitsExact` already contributes each candidate's model radius. In a
standalone reconstruction of the query (10k units, 4k movers, mixed 2–10 square footprints) it
roughly doubled the candidate list and cost ~2.2x the current query time.
A cheaper formulation is to add only the amount by which a footprint can reach *past* the
collidee's own model radius, i.e. a single engine-wide
```
max over units of ( footprintRadius + separationDistance - modelRadius ), floored at 0
```
which also closes both gaps above, and is zero — hence cheaper than the current buggy code — if
every unit's model radius already covers its footprint. Measured ~1.4x the current query cost on
the same synthetic mix, but that number is entirely content-dependent.
Note that any fix here changes synced behaviour: collisions that were previously missed will
start firing, so unit pushing and crushing will differ and pre-existing demos will diverge.
## Version
Observed on `master` (source archive), `rts/Sim/MoveTypes/GroundMoveType.cpp`.
Contributor guide
Research direction
Start in rts/Sim/MoveTypes/GroundMoveType.cpp at HandleUnitCollisions around lines 2534 and 2807, then trace CalcFootPrintMaxInteriorRadius in MoveDefHandler.cpp and candidate filtering in QuadField.cpp. Confirm the radius units and evaluate the static-collidee and separation-distance cases; done means an agreed, measured fix is validated without unintended synced collision changes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- game-dev
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100