docs: close C2, sharpen the column-memo prediction, reassess REVIEW_FINDINGS

Reading session -- no build available, so: verify things instead of writing code.

- Cross-checked the eight stat declarations against their definitions and use
  sites. Identical. The one compile risk I had only eyeballed is now retired.

- REVIEW_FINDINGS: the GetDensityWithParams and BuildChunkCache splits are now
  counter-productive rather than optional -- the op stack is replacing the
  first, and FRoomGraphSource deliberately CALLS the second so a restructure
  forks what was kept unforked. The two passage enums are not duplicates and
  are UMETA-serialised into authored assets; merging them rewrites content for
  tidiness. Judged: leave, and close the item.

- CODEX-TASK-002's acceptance said "20-30% of lookups", a guess wearing a
  number. Replaced with arithmetic from the real grid: 1225 columns/tile over
  35 Z planes, load factor 0.299, ~317 colliding columns, ~12000 vs 1225
  computations = ~9.8x. Plus the reading rule that matters -- the overhang and
  cliff mods re-query the same XY and add HITS only, so compare the ratio of
  the two hypotheses, not an absolute percentage.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-08-16 16:14:18 +02:00
parent b426cfcb0d
commit b5294b2d5b
3 changed files with 117 additions and 16 deletions
+29 -12
View File
@@ -87,21 +87,38 @@ around the existing `if (S.Key != ColumnKey || S.X != WorldX || S.Y != WorldY)`.
`Column->GetColumn(...)` (~line 864). They are legitimate traffic and must be counted, not `Column->GetColumn(...)` (~line 864). They are legitimate traffic and must be counted, not
excluded — they are part of why a miss is expensive. excluded — they are part of why a miss is expensive.
## Acceptance ## Acceptance — the prediction is now numeric (tightened 2026-08-16 from the real grid dimensions)
Fly a route through a **SurfaceWorld** strate that has `bUseOperatorStack` ticked, `stat VoxelForge` The hand-wavy "2030 %" band this section used to carry has been replaced by an arithmetic
on screen, and read the ratio. prediction, because the inputs are all statically knowable and were read out of the source:
- **`ColumnMemoMiss` ≈ one per distinct column per tile** (hit rate climbing toward ~97 % as Z - `CHUNK_SIZE = 32`, `CellsPerAxis = 32`, `GridDim = 33`, and the pre-sample loops run
planes accumulate) ⇒ **the hypothesis is WRONG**, the table is fine, and the perf cost is `g ∈ [-1, GridDim]` per axis ⇒ **35 × 35 = 1225 distinct columns per tile**, over **35 Z planes**
elsewhere. Record that and move to suspect 2 (per-voxel virtual dispatch). *A negative result here (the mesher's own buffer comment, "35³ floats", confirms the dimension).
is a real result and must be written down, not quietly dropped.* - 1225 keys in 4096 slots is a load factor of **0.299**. Expected slots holding exactly one key
- **`ColumnMemoMiss` in the neighbourhood of 2030 % of all lookups, and staying there** `= 4096 · np(1-p)^(n-1) ≈ 908`, so **~317 columns (25.9 %) share a slot with another** and evict
**confirmed**: the table is evicting on collision every Z plane. The fix is then obvious and cheap each other on every plane.
(direct-indexed box keyed like `GSurfColCache`, or a set-associative table), and gets its own - ⇒ op path ≈ `1225 + 34 × 317` ≈ **12 000** column computations per tile.
task with the before/after this run supplies. Original path (`GSurfColCache`, direct-indexed, `Computed[CI]` persists) = **1225**.
**≈ 9.8×.**
Report **both numbers**, not the ratio alone — a ratio cannot distinguish "few lookups" from "many". ### How to read the result
⚠️ **Compare the RATIO OF THE TWO HYPOTHESES, not an absolute percentage.** The overhang and cliff
modifiers call `GetColumn` again at the same XY (~line 864); every extra consumer adds **hits** and
no misses, so it inflates the denominator and drags the miss *rate* down without changing the
verdict. What does not move is the ~10× gap between the two outcomes.
| observation | verdict |
|---|---|
| misses ≈ **810×** the hit-path baseline (single-consumer: ~28 % of lookups) | **CONFIRMED** — the table evicts on collision every Z plane. The fix gets its own task, with this run as its "before". |
| misses ≈ **1 per distinct column** (single-consumer: ~3 %, hit rate ≥ 97 %) | **hypothesis WRONG.** The table behaves like the box, the ~9.8× does not exist, and the perf cost is suspect 2 (19 virtual calls per voxel). |
*A negative result here is a real result.* It retires the most-suspected cause and is worth the build
either way; it must be written into `OPSTACK-PROGRESS.md`, not quietly dropped.
Report **both raw numbers**, never the ratio alone — a ratio cannot distinguish "few lookups" from
"many", and the absolute miss count is what the fix would be reducing.
## Notes for the reviewer (Claude) ## Notes for the reviewer (Claude)
+56
View File
@@ -3497,3 +3497,59 @@ Three readings, and **the order matters**:
Also still unbuilt and riding along: `e002bd4` (VerticalShafts connector capsules) — Also still unbuilt and riding along: `e002bd4` (VerticalShafts connector capsules) —
`Box verdicts over 60 VerticalShafts tiles`, **0 is the number to beat**, `violations` must stay 0. `Box verdicts over 60 VerticalShafts tiles`, **0 is the number to beat**, `violations` must stay 0.
## 2026-08-16 (d) — while Jahni was away: one bug closed on paper, one prediction sharpened
No build, no measurement available, so this was a reading session. Three results.
### 1. The last compile risk in `eb317d9` is mechanically gone
The one spot I could not rule out by eye was an identifier mismatch between the eight declarations in
`VoxelStats.h` and the eight `DEFINE_STAT`s in `VoxelStats.cpp`. Diffed the extracted symbol lists:
**identical**, and all eight use sites resolve to declared names. That risk is retired, not estimated.
### 2. ⛔ `AUDIT §C2` IS FULLY CLOSED — I was one step from spec'ing a fix for a solved bug
The handoff, the audit and my own memory all listed the live-edit half (`OC_Chunk`, `BM_Chunk`,
`FChunkBiomeCache`) as open. It is not, and has not been for a while. Every per-chunk cache carries
the layout version — `CP_Version`, `OC_Version`, `BM_Version`, `TC_SeenVersion` — and
`FChunkBiomeCache::Invalidate()` exists precisely because the validity box says nothing about the
`FBiomeContext` its cells were classified against; all four `thread_local` instances call it on a
version change. The only two other instances in the tree are **function-local**, built per task, so
they cannot go stale.
**Seventh time a confident premise reversed on reading.** The difference is that this one was checked
*before* the work rather than after: it cost a doc edit instead of a Codex run and a build cycle.
Recorded at `AUDIT-2026-07.md §C2` and struck from the handoff's open list.
### 3. The column-memo prediction is now arithmetic, not a band
`CODEX-TASK-002`'s acceptance said "2030 % of lookups", which was a guess wearing a number. The
inputs are all statically knowable, so they were read instead: `CHUNK_SIZE = 32`, `CellsPerAxis = 32`,
`GridDim = 33`, pre-sample loops over `g ∈ [-1, GridDim]` ⇒ **1225 columns per tile over 35 Z
planes** (the mesher's own "35³ floats" buffer comment confirms the dimension).
1225 keys in 4096 slots = load factor **0.299**; expected single-occupancy slots `4096·np(1-p)^(n-1)`
≈ 908, so **~317 columns (25.9 %) collide** and evict each other every plane. Op path ≈
`1225 + 34×317` ≈ **12 000** column computations per tile against the box's **1225**: **≈ 9.8×**.
⚠️ And the reading instruction changed with it: **compare the ratio of the two hypotheses, not an
absolute percentage.** The overhang and cliff mods call `GetColumn` again at the same XY, which adds
**hits only** — it drags the miss *rate* down without touching the verdict. The ~10× gap between
"confirmed" and "wrong" is what survives that.
### 4. `REVIEW_FINDINGS.md` reassessed — two items are now counter-productive, not merely optional
- `GetDensityWithParams` and `BuildChunkCache` splits: **don't.** The operator stack is *replacing*
the first archetype by archetype, so the split gets deleted and churns the eight bit-for-bit
equivalence tests; and `FRoomGraphSource` deliberately **calls** `BuildChunkCache` rather than
transcribing it, so restructuring forks the thing kept unforked on purpose. Both also sit on the
`§8.10` invariants, where a "behaviour-preserving" change once silently deleted the overhang.
- `EVoxelPassageType` vs `EVoxelPassageStyle`: **judged, leave them.** Not duplicates — one is the
global inter-strate bore shape, the other per-strate descent styling on `FStratePassageConfig`,
with different owners and value sets. Both are `UMETA`-tagged and therefore **serialised into
Jahni's authored strate assets**, so merging them rewrites saved content for cosmetic tidiness.
Recommend closing the item rather than acting on it.
- `GetGenerationParams` is flagged as the safest of the six if any is ever wanted.
Nothing here needs a decision from Jahni; it all needs the same next build.
+32 -4
View File
@@ -92,6 +92,15 @@ Legend: ✅ verified against code · ◻️ checklist box.
`ResetGridBuildState(FDecoGrid&)`. *(2026-07-04)* `ResetGridBuildState(FDecoGrid&)`. *(2026-07-04)*
- [ ] **`EVoxelPassageType` vs `EVoxelPassageStyle`** — two overlapping passage-shape enums, both in - [ ] **`EVoxelPassageType` vs `EVoxelPassageStyle`** — two overlapping passage-shape enums, both in
active use (11 refs). Consider consolidating to one. *(judgment call, not dead)* active use (11 refs). Consider consolidating to one. *(judgment call, not dead)*
**JUDGED 2026-08-16 — LEAVE THEM. Recommendation: close this item rather than act on it.** They
read as duplicates from the index and are not: `EVoxelPassageType` (`VoxelStrateTypes.h` ~42) is
the **global inter-strate bore shape** the layout generator picks (`SlopedTunnel` / `VerticalShaft`
/ helix…); `EVoxelPassageStyle` (~1741) is **per-strate descent styling** on
`FStratePassageConfig` (`Straight` / `Worm` / `Spiral` / `Cascading`). Different owners, different
value sets, different lifetimes. And both are `UMETA`-tagged, i.e. **serialised into Jahni's
authored strate assets** — merging them silently rewrites saved content. That is a content-risk
change bought for cosmetic tidiness, which is the wrong trade at any time and especially before
the content lock.
## Dead code ## Dead code
- [x] **`UVoxelMarchingCubesMesher::GetDensity()`** — removed, along with `InterpolateEdge`, - [x] **`UVoxelMarchingCubesMesher::GetDensity()`** — removed, along with `InterpolateEdge`,
@@ -106,10 +115,29 @@ Legend: ✅ verified against code · ◻️ checklist box.
strate-aware vertical clamp; struck from the dead list. strate-aware vertical clamp; struck from the dead list.
## Over-complexity (behavior-preserving splits, optional) ## Over-complexity (behavior-preserving splits, optional)
- [ ] `GetDensityWithParams` (~600-1000 L) → `ApplyCaveMorphology`/`ApplySurfaceRoughness`/`ApplyTerrainOps`/`ApplyPostProcess`
- [ ] `BuildChunkCache` (~450 L) → `CollectRooms`/`BuildNeighborGraph`/`ResolveTunnels`/`BakeRoomFeatures` > ⛔ **REASSESSED 2026-08-16 — do not pick these up as filler work.** They were written before the
- [ ] `GenerateMesh` (~250 L) → `PrecalcDensityGrid`/`MarchCells`/`GenerateSkirts` > operator-stack refactor existed, and two of them are now actively counter-productive rather than
- [ ] `GetGenerationParams` (~180 L) → extract `ApplyBoundaryTransition(...)` > merely optional. Read the reason before ticking anything here.
- [ ] ~~`GetDensityWithParams` (~600-1000 L)~~ → **DON'T.** ⛔ Two independent reasons. (1) The
operator stack is *replacing* this function archetype by archetype — splitting it produces code
that gets deleted, and churns the eight equivalence tests that compare the stack against it **bit
for bit**. (2) It is the hottest path in the plugin and carries the `ARCHITECTURE §8.10`
invariants (`thread_local` box-valid caches, two-pass MC loop, SSE noise). The one time a
"behaviour-preserving" change was made here it silently deleted the overhang and only 1 sample in
20 000 crossed the isosurface — *a perf change can be a correctness change*. Revisit only once the
`switch` path is retired for good.
- [ ] ~~`BuildChunkCache` (~450 L)~~ → **DON'T, same reason.** `FRoomGraphSource` deliberately
**calls** `BuildChunkCache`/`EvaluateSDFCached` instead of transcribing them, precisely so there is
one definition. Restructuring it now forks the thing that was kept unforked on purpose.
- [ ] `GenerateMesh` (~250 L) → `PrecalcDensityGrid`/`MarchCells`/`GenerateSkirts`.
⚠️ Still genuinely optional, but the two-pass loop is an `§8.10` invariant — a split must not
merge the passes, and the Z-outermost pre-sample order is load-bearing for every column cache
downstream (see the column-memo work of 2026-08-16). Low value, non-zero risk.
- [ ] `GetGenerationParams` (~180 L) → extract `ApplyBoundaryTransition(...)`. **The safest of the
six** — pure params math, no caches, and `AUDIT §C2` already forced a close reading of both
Gradient arms. If any of these is ever worth doing, it is this one.
- [ ] `GeneratePassages` (~150 L) → `ComputePlacement`/`BuildControlChain`/`ComputeBounds` - [ ] `GeneratePassages` (~150 L) → `ComputePlacement`/`BuildControlChain`/`ComputeBounds`
- [ ] `BuildCellSpawns` (~150 L) → `FindSurfaceCrossings`/`PlaceDecorationsAtCrossings` - [ ] `BuildCellSpawns` (~150 L) → `FindSurfaceCrossings`/`PlaceDecorationsAtCrossings`