From eb80c80eb68f9095ac8c6361871d3915f632a531 Mon Sep 17 00:00:00 2001 From: Erik Date: Thu, 3 Sep 2026 08:58:43 +0200 Subject: [PATCH] =?UTF-8?q?docs(render):=20S3=20chunk=203=20fix=20round=20?= =?UTF-8?q?1=20=E2=80=94=20slot=20key,=20cross-block=20deferred=20batching?= =?UTF-8?q?,=20per-frame=20diagnostic,=20LandCell=20pins?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three-lens review of 671eb3ad: the terrain slot map is keyed by the DAT id (0xXXYYFFFF) while the walk passes 0xXXYY0000 (no terrain drawn); the adjacent-same-landblock batching can never merge because retail's own DrawSortCell interposes a particle turn after every land cell; the timing diagnostic moved inside the per-batch loop. Round 1 of two. Co-Authored-By: Claude Fable 5.1 --- .../s3-walk-ownership-map.md | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/docs/research/2026-09-01-overhaul/s3-walk-ownership-map.md b/docs/research/2026-09-01-overhaul/s3-walk-ownership-map.md index 7c4ea113..74015de8 100644 --- a/docs/research/2026-09-01-overhaul/s3-walk-ownership-map.md +++ b/docs/research/2026-09-01-overhaul/s3-walk-ownership-map.md @@ -518,6 +518,89 @@ against the chunk-2 run: the ONLY expected delta is the punch region at Holtburg (fragments gone); dense-outdoor p50/p99 recorded before/after with the existing frame-time diagnostics (no new counters). +### 9.6 Chunk 3 fix round 1 (lead, 2026-09-03, after the three-lens review of `671eb3ad`) + +Verdicts: build/test FAIL (blocking: the per-batch timing diagnostic), retail +FAIL (blocking: the terrain slot key), driver FAIL (blocking: batching is +inert). Lead-verified against the source; every item below is real. Fix in the +same worktree on top of `671eb3ad`; this is round 1 of the two the plan allows. + +- **F1 — slot key (blocking).** `TerrainModernRenderer._idToSlot` is keyed by + the DAT landblock id `0xXXYYFFFF` (`LandblockRenderPublisher.LandblockId => + Build.Landblock.LandblockId`; the old walk filter compared + `data.LandblockId & 0xFFFF0000`), while the walk hands `0xXXYY0000` + (`WalkLandBlock.LandblockId = bx<<24 | by<<16`). `DrawLandCellRuns` therefore + misses every lookup and the walk path draws NO terrain. Normalize at the + terrain entry: look up `(landblockId & 0xFFFF0000u) | 0xFFFFu`; add a unit + test that `AddLandblock(0xA9B4FFFF, …)` is found by a `0xA9B40000` lookup and + that an unknown block is a no-op. +- **F2 — batching across landblocks, deferred across no-op turns (blocking).** + Retail's `DrawSortCell` follows every `DrawLandCell`, so `HandleLandscapeCellTurn` + always appends a `StaticParticles` event (and, for cells with content, a + `StreamMark`) between two `LandCell` events — the "adjacent, same landblock" + rule never merges. The terrain index buffer is ONE buffer with + `FirstIndex = slot*384 + start`, and the deleted whole-stage draw already + issued one `MultiDrawIndexedIndirect` spanning every visible slot, so a + batch may span LANDBLOCKS. New rule, still order-preserving by construction: + Replay keeps a PENDING terrain batch (list of `(landblockId, side, + cellIndex)`, cleared at frame start); a `LandCell` event appends to it; before + any leaf call that WILL submit GPU work the pending batch is flushed + (`DrawLandCells` once); a particle turn whose cell has no renderable emitter + submits nothing and does NOT flush — add + `ParticleSystem.HasRenderableEmittersInCell(pass, cellId)` (a lookup in the + existing per-pass cell→handles index, no allocation) and ask it in Replay's + `StaticParticles`/`CellParticles` arms BEFORE flushing; every other event kind + (`StreamMark`, `AlphaBarrier`, `LandscapeFlush`, `ClearInteriorDepth`, + `ExitSeals`, `PunchFan`, `CellShell`, `Sky`) flushes first, and the end of + Replay flushes the remainder. The GPU submission order is then IDENTICAL to + the unbatched order (a skipped particle turn submits nothing either way). + `TerrainModernRenderer.DrawLandCellRuns` becomes `DrawLandCells(viewProjection, + IReadOnlyList<(uint LandblockId, int SideCellCount, int CellIndex)>)`: + resolve each entry's slot (F1), skip unknown slots, build the run commands in + list order, ONE indirect draw. Expected at terrace-edge frame 2: tens of + submissions, not 578; report the count from a driver test and in perfNote. +- **F3 — the terrain diagnostic (blocking).** One sample per FRAME, not per + batch: the leaf accumulates each batch's elapsed `Stopwatch.GetTimestamp()` + delta (no allocation) and the executor/renderer pushes ONE sample at the + end of the walk replay (where the old whole-stage `Complete()` semantics + lived); `TerrainRenderDiagnosticFacts.VisibleSlots` = distinct slots + submitted this frame (a per-frame bit set/scratch cleared at + `BeginFrame`), `draws` = batches submitted. The `[TERRAIN-DIAG]` line keeps + its meaning (cpu_us per frame) so §9.5's before/after compare is valid. +- **F4 — driver pins for the LandCell position (major).** The deleted + `TERRAIN:0` pins are REPLACED, not dropped: the fake leaf logs + `LANDCELL:::[,…]` per batch; add (a) an outdoor-root + `RunFrame` test with a synthetic in-view block whose sequence is + `SKY, LANDCELL…` with the block's cell turns after their terrain, (b) an + interior-root test with one exit view and one block: `SKY, LANDCELL…, + LFLUSH, SEALS, SHELL…`, (c) the batching pin from T4 re-expressed for the + new rule: two land cells of DIFFERENT landblocks with only an empty particle + turn between them merge into one `LANDCELL` batch; a `StreamMark` (a cell + with statics) or a building barrier between them splits; the fake leaf's + particle turn reports "has emitters" from a test-set so both arms are + covered. +- **F5 — interior-root terrain extent (major, answered, no code change).** + Retail's `DrawLandCell` fires only for in-view cells (`DrawBlock` @0x005a197d; + capture: `f418` 16 LC of 64) — the per-cell admission IS retail; the FW4 + under-paint came from the GPU clip of terrain to the portal polygon, which + no longer exists. Keep the in-view gate; the cathedral self-gate poses are + the check (recorded in the ledger either way). +- **F6 — minor/notes.** Frustum cull: the walk's `CheckBlocks` admission is the + authority (retail has no separate terrain frustum test beyond + `landcell_check`); document that in `DrawLandCells`' comment. The cathedral + order-trace token prints `:LC/:`. Fix the inverted comment in + `HandleLandscapeTurn` (a FARTHER building's punch survives because NEARER + terrain was drawn before it; the interleave draws that terrain after). Fix + the "directional-shadow receivers" claim (the only non-walk caller is the + flat-terrain path). Replace T2's vacuous `TERRAIN` assertion with the + driver-level pins of F4. Retire the stale "Confirmed OH5 defect" row in + `oh1-construction-landscape-contract.md` (line ≈310) with a one-line + "FIXED by S3 chunk 3 (``)". +- Gates unchanged (§9.5); the implementer additionally reports, from the + F4(c)-style driver test over a synthetic terrace-like frame, the batch + count for the terrace-edge cell sequence if it can be driven cheaply, else + the reasoning. + ## 10. Chunk 4 contract — deletions (lead draft, 2026-09-03; re-locate every owner at chunk time, after chunks 2 and 3 land) ### 10.1 What retail clips, and what it does not (verified)