fix(opstack): two cell-sweep pads assumed MinRadius <= MaxRadius; a third already didn't

Second defect from the full 28-op EffectOverBox audit. Three ops roll a radius
as Lerp(MinRadius, MaxRadius, hash01), which lands in [min(A,B), max(A,B)] --
Lerp does not require A <= B. Their EffectOverBox sweeps lattice cells padded by
the largest radius a cell could hold, so a pad below the true maximum means the
cells are never examined and the op reports Identity for a box its own Eval will
fill or carve: no geometry, no collision.

  FGridColumnMod   ~1086  Max(MaxRadius, 0)                        exposed
  FShaftFieldSource ~1436 Max(ShaftMaxRadius, ConnectorRadius)     exposed
  FIslandBlobSource ~1803 Max(IslandMinRadius, IslandMaxRadius)    already correct

The third one is the argument: the concern was met and guarded once in this same
file, and the other two shipped without it. Fixed with FMath::Max3, a spelling
already used here (~2481) and in VoxelCaveMorphology.cpp.

Shipped defaults are ordered correctly (2/5, 2/7, 5/11), so this is a NO-OP at
defaults -- that is its acceptance signal, and any moved number means the diff
did more than intended. It needs a mis-ordered asset value, which nothing
prevents: ClampMin is a per-property floor and UE cannot express "<= that other
property". ColumnMinRadius is also settable per-room via UVoxelTerrainOpDefinition.

Deliberately NOT fixed by normalising the params: swapping the Lerp endpoints
maps the same hash to a different radius, changing geometry and breaking the
eight equivalence tests. Only the bound becomes conservative; Eval is byte-
identical -- verified, no Eval/Roll/GetCells line in the diff.

Also audited and found sound (recorded so they are not re-chased): FPassageCarveOp
and the passage bounding sphere, FOriginSpineOp, FBoundarySealOp, FShaftLedgeMod,
FCaveArchMod, FDomeMod.

Not built -- Jahni builds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-08-16 16:50:01 +02:00
parent 108982d135
commit eaa44bf0c0
3 changed files with 178 additions and 4 deletions
+101
View File
@@ -0,0 +1,101 @@
# Codex task 004 — two box verdicts assume `MinRadius ≤ MaxRadius`; a third one already doesn't
**Owner:** Codex (Model Luna, xHigh) · **Orchestrator:** Claude · **Branch:** `experimental`
**Status:** specified, not started
**Kind:** ⚠️ **correctness of a box verdict.** Same class as task 003. Small fix, closes a class.
---
## Why this exists
Three operators roll a primitive radius from a hash between two designer-set params:
```cpp
Out.R = FMath::Lerp(MinRadius, MaxRadius, hash01); // FGridColumnMod ~1129
Out.R = FMath::Lerp(P.ShaftMinRadius, P.ShaftMaxRadius, hash01); // FShaftFieldSource ~1560
Out.Rxy = FMath::Lerp(P.IslandMinRadius, P.IslandMaxRadius, hash01); // FIslandBlobSource ~1850
```
`FMath::Lerp(A, B, t)` with `t ∈ [0,1]` lands anywhere in `[min(A,B), max(A,B)]` — it does **not**
require `A ≤ B`.
Each op's `EffectOverBox` then sweeps a **range of lattice cells** around the query box, padded by
the largest radius a cell could hold, and tests each rolled primitive exactly. The pad decides which
cells are *looked at at all*, so a pad smaller than the true maximum radius means **cells are never
examined**, their primitives are never tested, and the op reports `Identity` for a box that its own
`Eval` will carve or fill.
| op | pad used for the cell sweep | correct? |
|---|---|---|
| `FGridColumnMod` ~1086 | `FMath::Max(MaxRadius, 0.0f) + ColBlend` | ⛔ **exposed** |
| `FShaftFieldSource` ~1436 | `FMath::Max(P.ShaftMaxRadius, P.ConnectorRadius) + ExtraReach` | ⛔ **exposed** |
| `FIslandBlobSource` ~1803 | `const float MaxR = FMath::Max(P.IslandMinRadius, P.IslandMaxRadius);` | ✅ **already correct** |
**The third one is the point.** Someone hit this exact concern while writing the island source and
guarded it. The other two shipped without the guard. This task makes the three consistent.
## How exploitable, stated honestly
The shipped defaults are correctly ordered (`2/5`, `2/7`, `5/11`), so **nothing is broken out of the
box.** It needs a mis-ordered asset value — `ColumnMinRadius = 8, ColumnMaxRadius = 4`.
Nothing prevents that. The `UPROPERTY` metas carry `ClampMin = "1.0"`, which is a per-property
floor; Unreal has no declarative way to say "must be ≤ that other property". And `ColumnMinRadius`
is *also* settable per-room through `UVoxelTerrainOpDefinition`, so it is not only the strate asset.
What makes it worth the five lines: when it does happen, the failure is **invisible and maddening**.
`Eval` still draws the fat column perfectly, so every tile that gets meshed looks correct; only the
tiles the classifier *skipped* are missing — no geometry, no collision, in a world that otherwise
looks right.
## The fix — five lines, and one thing you must NOT do
Make each pad use the true envelope:
```cpp
// FGridColumnMod ~1086
const float Reach = FMath::Max3(MinRadius, MaxRadius, 0.0f) + ColBlend;
// FShaftFieldSource ~1436
const float Pad = FMath::Max3(P.ShaftMinRadius, P.ShaftMaxRadius, P.ConnectorRadius) + ExtraReach;
```
(Use whatever spelling is idiomatic here — check that `FMath::Max3` is already used in this codebase
before reaching for it; nested `FMath::Max` is fine and matches `FIslandBlobSource`'s existing line.)
### ⛔ DO NOT "fix it properly" by normalising the params
The tempting larger fix — swap `Min`/`Max` at resolution time so `Min ≤ Max` always — is **wrong and
will break the build's tests.** `Eval` computes `Lerp(Min, Max, t)`; swapping the endpoints maps the
same hash `t` to a *different* radius for the same cell. That changes generated geometry and breaks
the eight bit-for-bit equivalence tests against the `switch` path.
**Only the BOUND may become conservative. `Eval` stays byte-identical.** This is the same rule as
task 003 and the same reason.
## ⚠️ Invariants
1. **No `Eval`, `RollColumn`, `RollShaft`, `GetCells`, or `GetCellsAt` body may change.** If your
diff touches one, you have the wrong site — stop and say so.
2. **Do not touch `FIslandBlobSource`.** It is already correct and is the reference for this fix.
3. The change direction is strictly conservative: a wider sweep examines *more* cells, so a verdict
can only move from `Identity` toward `CarveOnly`/`FillOnly`, never the reverse. Do not add any
compensating tightening.
4. Comments are French + English; match the surrounding file. Say **why** the envelope is
`max(Min, Max)` and not `Max` — the next reader must not "simplify" it back.
## Acceptance
- `git diff --stat` shows exactly one file: `Source/VoxelForge/Private/VoxelDensityOpStack.cpp`,
and a handful of lines.
- The three ops now agree on the pattern.
- After the build: **every box-verdict line and every equivalence test is unchanged**, because the
shipped defaults are correctly ordered and the envelope only differs when they are not. **A change
in any of those numbers means the diff did something it should not have.** That is this task's
whole acceptance signal — a *no-op at defaults* is the expected, correct result.
## Notes for the reviewer (Claude)
- Confirm both pads changed and `FIslandBlobSource` did not.
- Confirm no `Lerp` argument order was touched anywhere — that is the failure mode that would look
like a tidy-up and silently change the world.
+69
View File
@@ -3628,3 +3628,72 @@ The first attempt stopped without editing: my spec wrote `VF_PerlinAbsBound` in
exactly the behaviour the spec template asks for ("if the code contradicts the spec, stop and say so exactly the behaviour the spec template asks for ("if the code contradicts the spec, stop and say so
rather than guessing"), and it is worth recording that it works — the cost was one clarification rather than guessing"), and it is worth recording that it works — the cost was one clarification
instead of a plausible-looking wrong rename. instead of a plausible-looking wrong rename.
## 2026-08-16 (f) — the box-verdict audit, finished: 28 ops swept, a second class of bug found
Having found the `ExtraReach` bug by auditing one archetype, I swept **all 28 `EffectOverBox`
implementations** rather than stopping at the one that paid. Result: one more real defect, three
false alarms worth recording so nobody re-chases them, and a clean bill for the rest.
### ⛔ FOUND — `Min > Max` radius envelope (`CODEX-TASK-004`)
Three ops roll a primitive radius as `FMath::Lerp(MinRadius, MaxRadius, hash01)`. `Lerp` with
`t ∈ [0,1]` lands in `[min(A,B), max(A,B)]` — it does **not** require `A ≤ B`. Each op's
`EffectOverBox` then sweeps a range of lattice cells padded by the largest radius a cell could hold;
a pad below the true maximum means cells are **never examined**, so the op reports `Identity` for a
box its own `Eval` will fill or carve.
| op | pad | |
|---|---|---|
| `FGridColumnMod` ~1086 | `Max(MaxRadius, 0)` | ⛔ exposed |
| `FShaftFieldSource` ~1436 | `Max(ShaftMaxRadius, ConnectorRadius)` | ⛔ exposed |
| `FIslandBlobSource` ~1803 | `FMath::Max(IslandMinRadius, IslandMaxRadius)` | ✅ **already correct** |
**The third is the whole argument.** Someone hit this exact concern writing the island source and
guarded it; the other two shipped without. Fixed to `FMath::Max3(...)` — a spelling already used in
this file (~2481) and in `VoxelCaveMorphology.cpp`.
**Honest severity:** shipped defaults are correctly ordered (`2/5`, `2/7`, `5/11`), so nothing is
broken out of the box, and **this fix is a no-op at defaults — that is its acceptance signal.** It
needs a mis-ordered asset value, which nothing prevents: `ClampMin` is a per-property floor and
Unreal cannot express "≤ that other property". `ColumnMinRadius` is also settable per-room via
`UVoxelTerrainOpDefinition`. What makes five lines worth it is the failure *mode*: `Eval` still draws
the fat column perfectly, so every meshed tile looks right and only the **skipped** ones are missing
geometry and collision — invisible, and maddening to diagnose.
⚠️ Recorded in the spec because it is the tempting wrong fix: **do not normalise the params.**
Swapping the endpoints maps the same hash to a different radius, changing generated geometry and
breaking the eight equivalence tests. Only the *bound* may become conservative; `Eval` stays
byte-identical. Same rule as task 003.
### ✅ CHECKED AND SOUND — do not re-audit these
- **`FPassageCarveOp`** — its `Eval` reads `EvaluateModifierSDF` while its box guard asks
`AnyPassageNearBox`; the differing nouns look like a set mismatch and are not — **both iterate the
same `Passages` array**, "Modifier" is a legacy name. And the guard is the *same function* the
hand-written `ClassifyTile` uses, so there is one definition, not two.
- **The passage bounding sphere** — `BoundRadius = maxDistFromCentre + Radius + 4.0f`, and that `+4`
**is** `PASSAGE_BLEND_RADIUS`, so "the bound already includes the blend" is true rather than
assumed. Its per-point radii are `RadiusAt(t) = Lerp(Mouth, Mid, Sin(t·π))` with `Sin ∈ [0,1]`, so
they never exceed `Passage.Radius = max(Mouth, Mid)` — the one place the `Min > Max` class *could*
have bitten a shipped default, and it does not.
- **`FOriginSpineOp` / `FBoundarySealOp`** — Z-band plus XY-circle tests against named reaches; both
fail safe to `CarveOnly`/`FillOnly`.
- **`FShaftLedgeMod`** — returns `FillOnly` unconditionally whenever ledges are configured, and its
`Identity` case is exactly the early-out its `Eval` takes. Conservative by construction.
- **`FCaveArchMod` / `FDomeMod`** — they *also* roll `Lerp(Min, Max, hash)` radii, but their
`Identity` comes only from `VF_NoCaveOverBox` (cave-surface proximity) or the feature being off.
**No radius envelope in their verdict ⇒ not exposed.** Checked precisely because they matched the
pattern on a grep.
- `FConstantFieldSource`, `FSlabVoidSource`, `FSurfaceColumnSource` — never return `Identity` at all.
### The shape of both bugs, worth naming
Task 003 and task 004 are the same mistake twice: **a box verdict's bound was derived from the
parameter that reads like the maximum rather than from the actual supremum of what `Eval` produces.**
Once it was `sup|FBM|` (assumed 1.0, proved 1.5); once it was `max radius` (assumed `MaxRadius`,
actually `max(Min, Max)`). Both times a correct instance of the same reasoning existed **elsewhere in
the same file** — `VF_PerlinAbsBound` for the first, `FIslandBlobSource` for the second.
⇒ When adding a bound, the question is not "what is the max parameter" but **"what is the supremum
of the thing `Eval` can actually produce, and where in this file has that already been worked out?"**
@@ -1081,9 +1081,10 @@ namespace
{ {
if (ColDensity <= 0.0f || Spacing <= 0.0f) { return EVoxelOpEffect::Identity; } if (ColDensity <= 0.0f || Spacing <= 0.0f) { return EVoxelOpEffect::Identity; }
// Marge : le centre d'une colonne vit dans sa cellule, son influence porte au plus // Marge : l'enveloppe de `Lerp(MinRadius, MaxRadius, t)` est max(MinRadius, MaxRadius),
// MaxRadius + ColBlend. Sur-estimer coûte du CPU ; sous-estimer serait un trou. // pas `MaxRadius` seul si l'asset inverse les paramètres. The bound must cover both
const float Reach = FMath::Max(MaxRadius, 0.0f) + ColBlend; // endpoints; using `MaxRadius` alone would leave a hole when the asset reverses them.
const float Reach = FMath::Max3(MinRadius, MaxRadius, 0.0f) + ColBlend;
const int32 CX0 = FMath::FloorToInt(((float)VoxelBox.Min.X - Reach) / Spacing); const int32 CX0 = FMath::FloorToInt(((float)VoxelBox.Min.X - Reach) / Spacing);
const int32 CX1 = FMath::FloorToInt(((float)VoxelBox.Max.X + Reach) / Spacing); const int32 CX1 = FMath::FloorToInt(((float)VoxelBox.Max.X + Reach) / Spacing);
@@ -1433,7 +1434,10 @@ namespace
// La source répond pour la paire source+carve (SIMPLIFICATION DE PHASE 1) : `CarveOnly` // La source répond pour la paire source+carve (SIMPLIFICATION DE PHASE 1) : `CarveOnly`
// si une primitive atteint la boîte, `Identity` sinon. `ExtraReach` couvre la rugosité // si une primitive atteint la boîte, `Identity` sinon. `ExtraReach` couvre la rugosité
// et le blend en aval — le sous-estimer serait un TROU. // et le blend en aval — le sous-estimer serait un TROU.
const float Pad = FMath::Max(P.ShaftMaxRadius, P.ConnectorRadius) + ExtraReach; // L'enveloppe doit couvrir les deux bornes de `Lerp(ShaftMinRadius, ShaftMaxRadius, t)`,
// pas `ShaftMaxRadius` seul si l'asset inverse les paramètres. The bound must cover
// both radius endpoints before adding connector and downstream reach.
const float Pad = FMath::Max3(P.ShaftMinRadius, P.ShaftMaxRadius, P.ConnectorRadius) + ExtraReach;
const FBox Padded = VoxelBox.ExpandBy(Pad); const FBox Padded = VoxelBox.ExpandBy(Pad);
const float Spacing = FMath::Max(P.ShaftSpacing, 1.0f); const float Spacing = FMath::Max(P.ShaftSpacing, 1.0f);