beyond-all-reason / beyond-all-reason/RecoilEngine

CGroundMoveType::HandleUnitCollisions mixes squares and elmos when sizing its collision search radius

Open
#3,303 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.