diff --git a/CODEX-TASK-002-column-memo-thrash.md b/CODEX-TASK-002-column-memo-thrash.md index 877de0c..f0a484f 100644 --- a/CODEX-TASK-002-column-memo-thrash.md +++ b/CODEX-TASK-002-column-memo-thrash.md @@ -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 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` -on screen, and read the ratio. +The hand-wavy "20–30 %" band this section used to carry has been replaced by an arithmetic +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 - planes accumulate) ⇒ **the hypothesis is WRONG**, the table is fine, and the perf cost is - elsewhere. Record that and move to suspect 2 (per-voxel virtual dispatch). *A negative result here - is a real result and must be written down, not quietly dropped.* -- **`ColumnMemoMiss` in the neighbourhood of 20–30 % of all lookups, and staying there** ⇒ - **confirmed**: the table is evicting on collision every Z plane. The fix is then obvious and cheap - (direct-indexed box keyed like `GSurfColCache`, or a set-associative table), and gets its own - task with the before/after this run supplies. +- `CHUNK_SIZE = 32`, `CellsPerAxis = 32`, `GridDim = 33`, and the pre-sample loops run + `g ∈ [-1, GridDim]` per axis ⇒ **35 × 35 = 1225 distinct columns per tile**, over **35 Z planes** + (the mesher's own buffer comment, "35³ floats", confirms the dimension). +- 1225 keys in 4096 slots is a load factor of **0.299**. Expected slots holding exactly one key + `= 4096 · np(1-p)^(n-1) ≈ 908`, so **~317 columns (25.9 %) share a slot with another** and evict + each other on every plane. +- ⇒ op path ≈ `1225 + 34 × 317` ≈ **12 000** column computations per tile. + 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 ≈ **8–10×** 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) diff --git a/OPSTACK-PROGRESS.md b/OPSTACK-PROGRESS.md index 718dccc..7c2480e 100644 --- a/OPSTACK-PROGRESS.md +++ b/OPSTACK-PROGRESS.md @@ -3497,3 +3497,59 @@ Three readings, and **the order matters**: 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. + +## 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 "20–30 % 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. diff --git a/REVIEW_FINDINGS.md b/REVIEW_FINDINGS.md index 61bb825..75da76c 100644 --- a/REVIEW_FINDINGS.md +++ b/REVIEW_FINDINGS.md @@ -92,6 +92,15 @@ Legend: ✅ verified against code · ◻️ checklist box. `ResetGridBuildState(FDecoGrid&)`. *(2026-07-04)* - [ ] **`EVoxelPassageType` vs `EVoxelPassageStyle`** — two overlapping passage-shape enums, both in 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 - [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. ## Over-complexity (behavior-preserving splits, optional) -- [ ] `GetDensityWithParams` (~600-1000 L) → `ApplyCaveMorphology`/`ApplySurfaceRoughness`/`ApplyTerrainOps`/`ApplyPostProcess` -- [ ] `BuildChunkCache` (~450 L) → `CollectRooms`/`BuildNeighborGraph`/`ResolveTunnels`/`BakeRoomFeatures` -- [ ] `GenerateMesh` (~250 L) → `PrecalcDensityGrid`/`MarchCells`/`GenerateSkirts` -- [ ] `GetGenerationParams` (~180 L) → extract `ApplyBoundaryTransition(...)` + +> ⛔ **REASSESSED 2026-08-16 — do not pick these up as filler work.** They were written before the +> operator-stack refactor existed, and two of them are now actively counter-productive rather than +> 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` - [ ] `BuildCellSpawns` (~150 L) → `FindSurfaceCrossings`/`PlaceDecorationsAtCrossings`