From 110fb691a8dca907b571c712285c7bc9f5e929fe Mon Sep 17 00:00:00 2001 From: Erik Date: Mon, 11 May 2026 00:09:57 +0200 Subject: [PATCH] =?UTF-8?q?ship(post-A.5=20#53):=20Tier=201=20entity-class?= =?UTF-8?q?ification=20cache=20=E2=80=94=20closes=20ISSUE=20#53?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EntityClassificationCache keyed by (entityId, landblockHint) tuple lands per spec docs/superpowers/specs/2026-05-10-issue-53-tier1-cache-design.md + plan docs/superpowers/plans/2026-05-10-issue-53-tier1-cache.md. Perf result (horizon-safe preset + High quality, AMD Radeon RX 9070 XT @ 1440p): entity dispatcher cpu_us median ~1200 us, p95 ~1500 us. Down from ~3500m / ~4000p95 pre-Tier-1. ~66% / ~63% reduction. Well under the A.5 spec budget (median <= 2.0 ms, p95 <= 2.5 ms). No BUDGET_OVER flag across 30s+ standstill captures. Visual gate cleared after 4 bug-fix iterations: - 71d0edc: namespace stab Ids globally (cross-LB id collision) - 95ebbf3: key cache by (entityId, landblockHint) tuple (defensive) - c55acdc: skip cache populate when classification is incomplete - f928e66: incomplete-entity flag must persist across same-entity tuples User-confirmed visually via +Acdream test character: NPCs animate, multi-part static buildings render fully (no airborne geometry, no Z-fighting, no missing parts, no wrong textures), Nullified Statue of a Drudge on top of the Foundry renders all parts, trees outside Holtburg render with branches present. Closes the post-A.5 polish phase. Issues #52, #54, #53 all closed. Tests: 1711 passing, 8 pre-existing physics/input failures unchanged. N.5b sentinel: 112/112 throughout. Memory: ~/.claude/projects/.../memory/project_tier1_cache.md + feedback_cache_per_tuple_pattern.md capture the audit-gap and per-tuple- vs-per-entity recurring trap for future cache work. Co-Authored-By: Claude Opus 4.7 (1M context) --- CLAUDE.md | 34 ++++++++++++++++------ docs/ISSUES.md | 77 +++++++++++++++++++++++++++++++++----------------- 2 files changed, 77 insertions(+), 34 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4e0b00b..6203821 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -525,14 +525,32 @@ acdream's plan lives in two files committed to the repo: acceptance criteria. Do not drift from the spec without explicit user approval. -**Currently in flight: Post-A.5 polish — Tier 1 retry (only remaining priority).** -Open issues: #53 (Tier 1 entity cache redo with animation-mutation audit). -ISSUES #52 (lifestone missing) and #54 (JobKind plumbing) closed 2026-05-10. #52 by -commit `e40159f` — three real bugs in the WB rendering migration's translucent pass -(alpha-test discard, missing cull state, missing `uDrawIDOffset` uniform). #54 by -commit `bf31e59` — `LandblockStreamJobKind` plumbed through `BuildLandblockForStreaming`, -far-tier worker now does heightmap-only load (no `LandBlockInfo`, no `SceneryGenerator`). -After #53 closes, the next planned phase is N.6 (perf polish) — see roadmap for scope. +**Currently in flight: NONE — Post-A.5 polish phase COMPLETE 2026-05-11.** +All three post-A.5 issues closed: #52 (lifestone, `e40159f`), #54 (JobKind, `bf31e59`), +#53 (Tier 1 entity cache, `f928e66`). Phase A.5 + post-A.5 polish together comprise +the streaming + rendering perf foundation for the project. + +**Tier 1 entity-classification cache (#53) shipped 2026-05-11.** New +`EntityClassificationCache` keyed by `(entityId, landblockHint)` tuple at +`src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs`; the dispatcher's static- +entity slow path populates flat `CachedBatch[]` (one entry per (partIdx, batchIdx) +with part-relative `RestPose` + resolved `BindlessTextureHandle`), and the cache-hit +fast path skips classification entirely on subsequent frames. Animated entities +(`_animatedEntities`) bypass the cache. Invalidation fires on live-entity despawn +(`RemoveLiveEntityByServerGuid`) and LB demote/unload (`RemoveEntitiesFromLandblock`). +Entity dispatcher cpu_us **median ~1200 µs / p95 ~1500 µs** at horizon-safe preset +on AMD Radeon RX 9070 XT @ 1440p — down from ~3500m / ~4000p95 pre-Tier-1 +(~66% / ~63% reduction). Well under the A.5 spec budget (median ≤ 2.0 ms, p95 ≤ 2.5 ms). +The implementation required 4 bug-fix iterations after the spec landed (stab Id +namespacing → cache tuple-key → drudge incomplete-classification → mid-list null- +renderData); see `docs/ISSUES.md` #53 closure entry for the lessons. + +**Next planned phase: N.6 (perf polish) — see `docs/plans/2026-04-11-roadmap.md`.** +Alternative escalation path: Tier 2 (static/dynamic split with persistent groups, +~2 weeks) or Tier 3 (GPU compute culling, ~1 month) per +`docs/plans/2026-05-10-perf-tiers-2-3-roadmap.md`. With the Tier 1 dispatcher at +~1.2 ms, the project comfortably hits 200-400 FPS at radius=12 standstill; +escalation makes sense only if user wants 500+ FPS sustained. **Phase A.5 (Two-tier Streaming + Horizon LOD) shipped 2026-05-10.** N₁=4 near-tier (81 LBs, full detail) + N₂=12 far-tier (544 LBs, terrain only); fog diff --git a/docs/ISSUES.md b/docs/ISSUES.md index a8da715..3565c30 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -46,32 +46,6 @@ Copy this block when adding a new issue: # Active issues -## #53 — A.5/tier1-redo: entity-classification cache broke animation (reverted) - -**Status:** OPEN -**Severity:** MEDIUM (perf gap; the classification cache would save ~1-2ms/frame but cannot land until animation-mutation audit is done) -**Filed:** 2026-05-10 -**Component:** rendering / WbDrawDispatcher / AnimationSequencer - -**Description:** Tier 1 entity-classification cache (commit `3639a6f`) was reverted at `9b49009` due to an animation regression. The cache stored `meshRef.PartTransform` at first-classify time. For static entities this is stable. For animated entities, `AnimationSequencer` mutates `meshRef.PartTransform` every frame to apply the current skeletal pose. The cache froze the pose, causing NPCs and some animated entities to stop animating (some buildings also showed at wrong positions, likely entities incorrectly flagged as animated). - -**Root cause:** the "trust MeshRefs as the source of truth" comment in the dispatcher gave false confidence — MeshRefs IS the source of truth, but it is mutated EVERY frame for animated entities. - -**Next attempt needs:** - -1. Audit `AnimationSequencer` + `AnimationHookRouter` to identify ALL per-frame mutations of `MeshRef` state (not just `PartTransform` — are any other fields mutated?). -2. Redesign cache to: (a) bypass animated entities entirely (classify them each frame, cache only static entities), OR (b) cache only the animation-invariant subset of the classification key (group key, texture handle, blend mode) while reading the per-frame pose from the live `MeshRef`. -3. Test specifically with a moving animated NPC visible on screen before shipping. - -**Estimated:** 1 week including audit + redesign + retest. - -**Files:** -- `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs` — dispatcher classification logic -- `src/AcDream.Core/Animation/AnimationSequencer.cs` — mutation source -- `src/AcDream.Core/Animation/AnimationHookRouter.cs` — secondary mutation source - ---- - ## #50 — Road-edge tree at 0xA9B1 visible in acdream but not retail **Status:** OPEN @@ -1703,6 +1677,57 @@ Unverified. The likely culprits, ranked by suspected probability: # Recently closed +## #53 — [DONE 2026-05-11 · f928e66] A.5/tier1-redo: entity-classification cache retry + +**Closed:** 2026-05-11 +**Commit chain (newest first):** +- `f928e66` — incomplete-entity flag must persist across same-entity tuples (mid-list null-renderData) +- `c55acdc` — skip cache populate when classification is incomplete (drudge fix) +- `95ebbf3` — key cache by `(entityId, landblockHint)` tuple to defeat ID collision +- `71d0edc` — namespace stab Ids globally (`0xC0LLBB01..`) for Tier 1 cache safety +- `4df1914` — clarify `DebugCrossCheck`'s wiring status +- `f16604b` — DEBUG cross-check + tripwire + 2 tests +- `489174f` — wire `InvalidateLandblock` callback at LB demote/unload +- `1d1afcd` — wire `InvalidateEntity` at live-entity despawn +- `f7e38c2` — cache-hit fast path must fire per-entity, not per-tuple +- `0cbef3c` — cache-hit fast path + dispatcher integration tests +- `00fa8ae` — cache `Populate` must flush at entity boundary, not per-MeshRef tuple +- `2f489a8` — cache-miss populate on first frame for static entities +- `28513ea` — optional `CachedBatch` collector + `restPose` param on `ClassifyBatches` +- `a65a241` — inject `EntityClassificationCache` into `WbDrawDispatcher` +- `60fbfce` — plumb `landblockId` through `_walkScratch` +- `a171e70`, `aea4460`, `694815c`, `773e970` — cache `InvalidateLandblock` / `InvalidateEntity` / `Populate` / skeleton+first test +- `c02405c` — extract `GroupKey` to namespace-scope `internal` +- `2f8a574` — implementation plan +- `4abb838` — mutation audit + cache design spec + +**Component:** rendering / `WbDrawDispatcher` / `EntityClassificationCache` / `LandblockLoader` + +**Resolution.** New `EntityClassificationCache` keyed by `(entityId, landblockHint)` tuple in `src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs`. The dispatcher routes static entities (NOT in `_animatedEntities`) through the cache — first-frame slow-path populates flat `CachedBatch[]` (one entry per (partIdx, batchIdx) with the part-relative `RestPose` and resolved `BindlessTextureHandle`); subsequent-frame cache hits skip classification entirely and append `cached.RestPose * entityWorld` to each matching group. Animated entities bypass. Invalidation fires from `RemoveLiveEntityByServerGuid` (per-entity, `0xF747`/`0xF625`) and `RemoveEntitiesFromLandblock` (per-LB, Near→Far demote + unload). + +**Perf result.** Entity dispatcher cpu_us **median ~1200 µs, p95 ~1500 µs** at horizon-safe + High preset on AMD Radeon RX 9070 XT @ 1440p. Pre-Tier-1 baseline was ~3500m / ~4000p95. ~66% reduction in median, ~63% in p95. Well under the A.5 spec budget (median ≤ 2.0 ms, p95 ≤ 2.5 ms). No `BUDGET_OVER` flag observed. + +**Verification.** Build green; full suite 1711 passed / 8 pre-existing physics/input failures unchanged; N.5b sentinel 112/112; visual gate confirmed via `+Acdream` test character (NPCs animate, lifestone renders, multi-part buildings + scenery + Nullified Statue of a Drudge on top of the Foundry all render fully — no airborne geometry, no Z-fighting, no missing parts, no wrong textures). + +**Lessons surfaced during implementation (4 bug-fix iterations):** + +1. **Audit must verify ID uniqueness for cache keys.** The original mutation audit verified `Position`/`Rotation`/`MeshRefs` stability post-spawn but didn't verify `entity.Id` was globally unique. Stabs from `LandblockLoader.BuildEntitiesFromInfo` restarted at `nextId = 1` per landblock → cross-LB collisions. Scenery (`0x80LLBB00 + localIndex`) and interior (`0x40LLBB00 + localCounter`) overflow at >256 items/LB. Cache key collision produced "buildings up in the air with wrong textures." Fixed by namespacing stab Ids (`71d0edc`) then by changing cache key to `(entityId, landblockHint)` tuple (`95ebbf3`) — defensive against ALL future hydration paths. + +2. **Per-tuple iteration with per-entity cache state is a recurring trap.** Three separate bugs caught by code review or visual gate hit this same root cause: + - Populate fired per-tuple → multi-MeshRef entities lost all but the last MeshRef's batches (`00fa8ae`). + - Cache hit fired per-tuple → multi-MeshRef entities drew N× copies, severe Z-fighting (`f7e38c2`). + - Incomplete-flag reset fired per-tuple → mid-list null-MeshRef trees populated partial cache, branches never rendered (`f928e66`). + + The fix pattern in all three: track previous entity Id (`prevTupleEntityId` / `lastHitEntityId`); execute per-entity logic only on actual entity-change detected against that tracker, not unconditionally per tuple. + +3. **Async mesh loading interacts with cache populate.** WB's `ObjectMeshManager.PrepareMeshDataAsync` decodes meshes off the main thread. If a MeshRef's GfxObj is still decoding at first-frame visibility, `TryGetRenderData` returns null and the slow path skips it. Without the drudge fix (`c55acdc`), the cache populated a partial classification and cache hits served it forever — even after the missing mesh loaded. With the fix, the dispatcher tracks `currentEntityIncomplete` per entity and drops the populate scratch when any MeshRef returned null; the slow path retries every frame until all meshes load. + +4. **A/B diagnostic env-var paid for itself.** `ACDREAM_DISABLE_TIER1_CACHE=1` forces every static entity through the slow path. Used twice during debugging to instantly differentiate "bug is in the cache" vs "bug is elsewhere entirely." Kept in tree (read once in `WbDrawDispatcher` ctor) for future cache investigations. + +**Memory.** See `~/.claude/projects/C--Users-erikn-source-repos-acdream/memory/project_tier1_cache.md` for the audit-gap and per-tuple-vs-per-entity pattern documented for future cache work. + +--- + ## #54 — [DONE 2026-05-10 · bf31e59] A.5/jobkind-plumbing: far-tier worker loads full entity layer then strips **Closed:** 2026-05-10