diff --git a/docs/research/2026-05-10-tier1-mutation-audit.md b/docs/research/2026-05-10-tier1-mutation-audit.md new file mode 100644 index 0000000..f206bf4 --- /dev/null +++ b/docs/research/2026-05-10-tier1-mutation-audit.md @@ -0,0 +1,246 @@ +# Tier 1 entity-classification cache — mutation audit + +**Created:** 2026-05-10, opening move of the ISSUE #53 retry session. +**Purpose:** enumerate every code path that writes to `WorldEntity.MeshRefs` (the dispatcher's load-bearing per-entity input) and every adjacent state read by `WbDrawDispatcher.ClassifyBatches` / model-matrix composition, classify each as STATIC or DYNAMIC, and design the cache invalidation surface BEFORE touching renderer code. + +This audit is the load-bearing prerequisite the prior Tier 1 attempt (commit `3639a6f`, reverted at `9b49009`) skipped. Cache design follows from the audit, not the other way around. + +--- + +## TL;DR — the invariant + +> **An entity's `MeshRefs` reference, `Position`, `Rotation`, `PaletteOverride`, `HiddenPartsMask`, `ParentCellId`, and `Scale` are stable from spawn to despawn IF AND ONLY IF the entity is NOT in `GameWindow._animatedEntities`.** + +That is the invariant the cache rides on. Animated entities (player + remote NPCs/players + animated dat scenery like the lifestone crystal) get a fresh `MeshRefs` list every frame from `TickAnimations` plus per-frame `Position`/`Rotation` writes from physics/dead-reckoning. Everything else — stabs, scenery, cell-mesh entities, interior static objects — touches none of those fields after construction. + +The cache should hold per-entity classification ONLY for entities whose `Id` is not in `_animatedEntities` at lookup time. Animated entities go through today's per-frame classification path unchanged. + +--- + +## §1. `entity.MeshRefs = ...` write sites (the core question) + +`WorldEntity.MeshRefs` is `IReadOnlyList` with a `set` accessor (see [src/AcDream.Core/World/WorldEntity.cs:28](../../src/AcDream.Core/World/WorldEntity.cs#L28)). `MeshRef` itself is a `readonly record struct` ([src/AcDream.Core/World/MeshRef.cs:15](../../src/AcDream.Core/World/MeshRef.cs#L15)) — its fields cannot be mutated in place. So every "MeshRefs change" is a whole-list replacement, not a per-element edit. + +Six write sites total in `src/`. Five STATIC, one DYNAMIC. + +### Site 1 — `OnLiveEntitySpawnedLocked` (server-spawned entity hydration) + +**[src/AcDream.App/Rendering/GameWindow.cs:2578](../../src/AcDream.App/Rendering/GameWindow.cs#L2578)** — `MeshRefs = meshRefs` in the `WorldEntity { … }` constructor. + +**Classification:** **STATIC** at first spawn. + +**Trigger:** server's `0xF745 CreateObject` for any entity (NPC, monster, player, item, statue, lifestone). Also re-runs from `OnLiveAppearanceUpdated` (server's `0xF625 ObjDescEvent`) → spawn dedup at top of `OnLiveEntitySpawnedLocked` invokes `RemoveLiveEntityByServerGuid`, then re-spawns. Each invocation gets a NEW local `entity.Id` from `_liveEntityIdCounter++` (line 2573). + +**Implication for cache:** ObjDescEvent isn't a "mutate existing entity" event — it's a despawn+respawn pair. The despawn path (next subsection) clears the cache for the old Id; the respawn populates fresh under the new Id. The cache never sees a stale entry for a still-active Id from this path. + +**Pre-construction `parts[…]` mutations** at lines 2333 and 2365 (AnimPartChanges + DIDDegrade resolver) edit the *local* `parts` list before it becomes the `meshRefs` argument; they're not separate write sites. + +### Site 2 — `BuildLandblockForStreaming` (stab hydration) + +**[src/AcDream.App/Rendering/GameWindow.cs:4748](../../src/AcDream.App/Rendering/GameWindow.cs#L4748)** — `MeshRefs = meshRefs` constructing dat-stab entities. + +**Classification:** **STATIC** at hydration. Worker-thread only. + +**Trigger:** streaming worker's near-tier load path (`LandblockStreamJobKind.LoadNear` or `PromoteToNear`). Single-GfxObj stabs use `Matrix4x4.Identity`; multi-part Setups go through `SetupMesh.Flatten` to produce per-part MeshRefs. + +**Lifetime:** lives until the entity's owning landblock is demoted (Near→Far) or unloaded — see Site invalidation §3.2. + +### Site 3 — `BuildSceneryEntitiesForStreaming` (procedural scenery) + +**[src/AcDream.App/Rendering/GameWindow.cs:4951](../../src/AcDream.App/Rendering/GameWindow.cs#L4951)** — `MeshRefs = meshRefs` for trees / rocks / bushes / fences. + +**Classification:** **STATIC** at hydration. Worker-thread only. + +**Lifetime:** identical to Site 2. + +### Site 4 — Interior cell-mesh entity + +**[src/AcDream.App/Rendering/GameWindow.cs:5023](../../src/AcDream.App/Rendering/GameWindow.cs#L5023)** — `MeshRefs = new[] { cellMeshRef }` for the EnvCell's own room geometry as a renderable entity. + +**Classification:** **STATIC** at hydration. + +### Site 5 — Interior static-object entity + +**[src/AcDream.App/Rendering/GameWindow.cs:5083](../../src/AcDream.App/Rendering/GameWindow.cs#L5083)** — `MeshRefs = meshRefs` for static objects placed inside an EnvCell (furniture, fixtures). + +**Classification:** **STATIC** at hydration. + +### Site 6 — `TickAnimations` per-frame rebuild + +**[src/AcDream.App/Rendering/GameWindow.cs:7580](../../src/AcDream.App/Rendering/GameWindow.cs#L7580)** — `ae.Entity.MeshRefs = newMeshRefs;` after constructing a fresh `List(partCount)` at line 7501 from `sequencer.Advance(dt)` output. + +**Classification:** **DYNAMIC** every frame. + +**Trigger:** per-frame iteration over `_animatedEntities.Values` inside `TickAnimations`. If `entity.Id ∈ _animatedEntities`, this loop runs for that entity every frame (subject to motion-table presence). If `entity.Id ∉ _animatedEntities`, this loop never runs for it. + +**Consequence:** any cache that captures `entity.MeshRefs[i].PartTransform` for an entity in `_animatedEntities` will freeze the pose. **This is exactly what the prior Tier 1 attempt did.** + +--- + +## §2. `_animatedEntities` membership transitions + +`_animatedEntities` at [GameWindow.cs:160](../../src/AcDream.App/Rendering/GameWindow.cs#L160) is the gating dict. The cache's "static" predicate is `! _animatedEntities.ContainsKey(entity.Id)`. + +### Population + +- **[GameWindow.cs:2724](../../src/AcDream.App/Rendering/GameWindow.cs#L2724)** — `_animatedEntities[entity.Id] = new AnimatedEntity { … }` at server-spawn for entities with a non-empty motion table + a resolvable idle cycle. +- **[GameWindow.cs:7685](../../src/AcDream.App/Rendering/GameWindow.cs#L7685)** — `_animatedEntities[pe.Id] = ae;` in `UpdatePlayerAnimation` to *re-add* the local player entity if a prior `UpdateMotion` removed it (the "Phase 6.8 stationary remove" pattern). This is the only path that can flip an entity from STATIC to ANIMATED mid-life. + +### Removal + +- **[GameWindow.cs:2935](../../src/AcDream.App/Rendering/GameWindow.cs#L2935)** — `_animatedEntities.Remove(existingEntity.Id)` inside `RemoveLiveEntityByServerGuid`. Fires for `0xF747 DeleteObject` and as the dedup leg of `OnLiveAppearanceUpdated`. + +### Cache implication + +Membership IS NOT cached by the dispatcher. The cache lookup checks `_animatedEntities.ContainsKey(entity.Id)` at lookup time. If the player flips STATIC→ANIMATED mid-session (Site 7685 above), a stale cache entry would still exist for the player Id but never be read; the next despawn (Site 2935) clears it. No special-casing needed. + +The reverse flip (ANIMATED→STATIC, e.g. a ground-state demote) leaves no cache entry; the dispatcher takes the cache-miss path on the first frame and populates fresh. Also no special-casing needed. + +--- + +## §3. Position / Rotation write sites (matters for the cached model matrix) + +The dispatcher composes `model = meshRef.PartTransform * entityWorld` for non-Setup entities, and `model = restPose * meshRef.PartTransform * entityWorld` for Setup multi-parts (with `entityWorld = Rotation × Translation`). If `Position` or `Rotation` changes for a STATIC entity, a cached model matrix would be stale. + +Audit shows: **every Position/Rotation write site in `GameWindow.cs` operates on entities that are in `_animatedEntities`.** Static entities never have these fields touched after construction. + +| Line | Context | Animated? | +|---|---|---| +| 3992-3993 | `entity.SetPosition(worldPos); entity.Rotation = rot;` (player physics snap-on-arrival) | YES — `entity` is the local player | +| 4116 | `entity.SetPosition(rmState.Body.Position);` (remote dead-reckon snap branch) | YES — remote NPC/player | +| 4230 | same context, near-enqueue branch | YES | +| 4362-4363 | remote dead-reckon physics tick body sync | YES | +| 4407-4408 | local player position snap (teleport / GoHome) | YES | +| 7045-7046 | `ae.Entity.SetPosition(rm.Body.Position); ae.Entity.Rotation = rm.Body.Orientation;` (TickAnimations body sync) | YES — `ae.Entity` is in `_animatedEntities` by definition | +| 7373-7374 | same body-sync context, fall-through path | YES | + +No Position/Rotation writes happen on entities that are NOT in `_animatedEntities`. Confirmed via grep. + +--- + +## §4. Other entity fields read by the dispatcher + +`WbDrawDispatcher.Draw` and `ClassifyBatches` read these `WorldEntity` fields beyond `MeshRefs`, `Position`, `Rotation`: + +| Field | Mutability | Cache impact | +|---|---|---| +| `PaletteOverride` | `init`-only ([WorldEntity.cs:37](../../src/AcDream.Core/World/WorldEntity.cs#L37)) | Stable post-spawn → safe to fold into cache key / texHandle resolution | +| `HiddenPartsMask` | `init`-only ([WorldEntity.cs:73](../../src/AcDream.Core/World/WorldEntity.cs#L73)) | Stable; doesn't apply to dispatcher anyway (animation tick handles part-hide via `s_hidePartIndex` debug global, animated path only) | +| `ParentCellId` | `init`-only ([WorldEntity.cs:45](../../src/AcDream.Core/World/WorldEntity.cs#L45)) | Stable; visibility filter input | +| `AabbMin/AabbMax/AabbDirty` | Mutated lazily by `RefreshAabb` ([WorldEntity.cs:79-91](../../src/AcDream.Core/World/WorldEntity.cs#L79)) on `AabbDirty` flag, set by `SetPosition` | Read by `WalkEntitiesInto`, NOT used by classification. AABB stays static for static entities (Position never changes → never marked dirty after first refresh) | +| `MeshRefs[i].SurfaceOverrides` | `init`-only on the MeshRef record struct | Stable for the lifetime of the MeshRef list (Sites 1-5) | +| `MeshRefs[i].GfxObjId` | Stable (`readonly record struct`) | Forms part of the cache key | +| `MeshRefs[i].PartTransform` | Stable for STATIC entities (the list is replaced atomically in Site 6 only for ANIMATED entities) | Cacheable for STATIC entities | + +No hidden mutability surface. The cache is safe for entities outside `_animatedEntities`. + +--- + +## §5. Cache invalidation events (the wire-up) + +The cache is keyed by `entity.Id`. Only TWO event sources can invalidate a cached entry: + +### §5.1 Per-entity despawn (live server entities) + +**[GameWindow.cs:2933-2935](../../src/AcDream.App/Rendering/GameWindow.cs#L2933)** — `_worldState.RemoveEntityByServerGuid(serverGuid); _worldGameState.RemoveById(...); _animatedEntities.Remove(...);` + +This block fires for: +- `0xF747 DeleteObject` (server explicitly says entity is gone). +- `0xF625 ObjDescEvent` (dedup leg before respawn). + +**Hook:** add `_wbDrawDispatcher.InvalidateEntity(existingEntity.Id)` to this block. + +### §5.2 Landblock demote / unload (static dat entities) + +**[src/AcDream.App/Streaming/GpuWorldState.cs:373](../../src/AcDream.App/Streaming/GpuWorldState.cs#L373)** — `RemoveEntitiesFromLandblock(landblockId)` clears the entity list for a landblock. Called from `StreamingController.Tick` at [StreamingController.cs:116](../../src/AcDream.App/Streaming/StreamingController.cs#L116) for `ToDemote` (Near→Far) and via `_enqueueUnload` for `ToUnload`. + +**Hook:** add `_wbDrawDispatcher.InvalidateLandblock(landblockId)` adjacent to the `RemoveEntitiesFromLandblock` call. Walk the LB's pre-removal entity list; invalidate each Id. + +Implementation note: `RemoveEntitiesFromLandblock` already has the entity list in scope before zeroing it — adding the invalidation walk inside the method (or via a callback) is cheap. Alternative: `StreamingController` walks the LB's entries before invoking `RemoveEntitiesFromLandblock`. Either works; brainstorming will pick. + +### §5.3 No other invalidation paths needed + +Confirmed: +- `MarkPersistent` ([GameWindow.cs:2024](../../src/AcDream.App/Rendering/GameWindow.cs#L2024)) — keeps player Id pinned across LB unloads. No MeshRefs change. +- `DrainRescued` ([GameWindow.cs:5885](../../src/AcDream.App/Rendering/GameWindow.cs#L5885)) — re-attaches rescued persistent entities. No MeshRefs change. +- `RelocateEntity` ([GameWindow.cs:6026](../../src/AcDream.App/Rendering/GameWindow.cs#L6026)) — moves entity between landblocks. Doesn't change MeshRefs/Position/Rotation. Safe. +- `AddEntitiesToExistingLandblock` ([GpuWorldState.cs:401](../../src/AcDream.App/Streaming/GpuWorldState.cs#L401)) — Far→Near promotion adds entities. New entries get cache-miss naturally. + +`AnimationSequencer` ([src/AcDream.Core/Physics/AnimationSequencer.cs](../../src/AcDream.Core/Physics/AnimationSequencer.cs)) does NOT write to `entity.MeshRefs` or `entity.Position`/`entity.Rotation` directly. It produces `PartTransform[]` frames consumed by `TickAnimations`. Confirmed via grep — only docstring mention of `MeshRef`. Sequencer is safe to ignore for cache design. + +`Core` library has zero `entity.MeshRefs = ...` writes. All writes are in the App layer, all in `GameWindow.cs`. Confirmed via grep. + +--- + +## §6. Recommended cache shape (for brainstorming, not yet committed) + +Pre-spec recommendation; final design picks settle in the brainstorming session. + +```csharp +// Per-(entity, partIdx, batchIdx) classification result. +private readonly record struct CachedBatch( + GroupKey Key, // bucket identity + ulong BindlessTextureHandle, // resolved texture (via palette + override) + Matrix4x4 RestPose); // meshRef.PartTransform (or restPose * meshRef.PartTransform for Setup) + +// Per-entity cache value. +private sealed class EntityCache +{ + public List Batches = new(); // ordered: (part, batch) flat + public uint LandblockHint; // for InvalidateLandblock +} + +// Cache state. +private readonly Dictionary _entityCache = new(); + +// Hot path: +// if (_animatedEntities.ContainsKey(entity.Id)) → today's path (full ClassifyBatches) +// else if (_entityCache.TryGetValue(entity.Id, out var cached)) → +// for each batch: append (cached.RestPose * entityWorld) to its group's matrices +// else → ClassifyBatches once, populate cache, then same fast path next frame. +``` + +**Per-frame static cost:** dictionary lookup + per-batch matrix multiply + matrices.Add. No texture resolution, no group-key construction, no metaTable lookup. + +**Worst case:** if every entity is animated (e.g. a city full of NPCs), the cache adds one `ContainsKey` lookup per visible entity vs today's path. Negligible overhead. In practice ~10K entities total at radius=12 with ~50 animated → 99.5% cache hit rate on the static path. + +**Risk surface:** the cache invariant rests on TWO claims, both verified in the audit above: +1. STATIC entity Position / Rotation never mutate post-spawn. Verified §3. +2. STATIC entity MeshRefs reference never changes post-spawn. Verified §1 (only Site 6 writes, only for animated entities). + +If either claim breaks in a future change (e.g. someone adds an "earthquake" effect that mutates static-tree positions), the cache will quietly serve stale matrices. Defense: +- **DEBUG-only assertion** in the cache hit path: `Debug.Assert(!_animatedEntities.ContainsKey(entity.Id))`. +- **DEBUG-only cross-check**: in DEBUG builds, in the cache-hit path, also recompute the live model matrix and compare against `cached.RestPose * entityWorld`. Log a warning if they differ. Catches the "someone added a new mutation site" failure mode without paying the cost in Release. + +(Belt-and-suspenders option (c) from the original handoff. Recommended for the first retry given the prior bug.) + +--- + +## §7. What does NOT need to be in the cache design + +- **Texture invalidation on bindless handle change.** Bindless handles are issued on first texture upload and remain valid for the texture's lifetime. `TextureCache` doesn't evict entries during normal play (only on shutdown). Static-entity texture handles never change. +- **GfxObj re-decode.** `WbMeshAdapter.TryGetRenderData` returns the same `ObjectRenderData` instance for a given `gfxObjId` for the session. Static-entity batches never change. +- **`SurfaceOverrides` reactivation.** Init-only on `MeshRef`, set at Site 1's hydration time, stable for the MeshRef's lifetime. +- **Per-frame `Time` / `dt` inputs.** The dispatcher doesn't read time. Texture animation (e.g. animated UV scrolls) happens in the shader from `gl_Time`-equivalent uniforms, not from cached state. + +--- + +## §8. Open questions for brainstorming + +These need a user decision before I write the spec: + +1. **Where do `InvalidateEntity` / `InvalidateLandblock` live?** On `WbDrawDispatcher` (cache lives there)? On a new `EntityClassificationCache` class injected into the dispatcher (separation of concerns; testable in isolation)? My lean: separate class, dispatcher gets it via ctor. +2. **Static-only (option a) vs static-only + DEBUG cross-check (option c)?** Cross-check costs nothing in Release and catches the exact bug class that bit us last time. My lean: option (c). +3. **Cache the full model matrix or the rest pose?** Full matrix saves a per-frame multiply but bakes Position/Rotation into the cache (theoretically violatable). Rest pose is safer + costs ~one mat4 mult per batch. My lean: rest pose. +4. **Setup multi-part flattening: cache the per-part `setupPart.PartTransform * meshRef.PartTransform` product?** Today's `Draw` walks `renderData.SetupParts` per-frame even though that list is per-GfxObj-immutable. The cache could pre-flatten into the batch list. My lean: yes — that's where the visible CPU win is. +5. **Test plan: where do new tests live?** `tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs`? Pure-CPU tests on the cache class without GL state? My lean: yes, separate test file in the existing Wb test directory. + +--- + +## §9. Sentinel + baseline (verified at audit start, 2026-05-10) + +- `dotnet build`: green (after `git submodule update --init` for the WorldBuilder ref tree, which was missing in this fresh worktree). +- `dotnet test --no-build`: 1688 passing, 8 pre-existing failures in `AcDream.Core.Tests`. Matches the post-#52/#54 baseline in the handoff. +- N.5b sentinel filter (`TerrainSlot|TerrainModernConformance|Wb|MatrixComposition|TextureCacheBindless|SplitFormulaDivergence`): 94/94 passing. Matches the post-#52/#54 baseline. + +These are the floors the Tier 1 retry must keep clean throughout. diff --git a/docs/superpowers/specs/2026-05-10-issue-53-tier1-cache-design.md b/docs/superpowers/specs/2026-05-10-issue-53-tier1-cache-design.md new file mode 100644 index 0000000..dfe7a84 --- /dev/null +++ b/docs/superpowers/specs/2026-05-10-issue-53-tier1-cache-design.md @@ -0,0 +1,451 @@ +# ISSUE #53 — Tier 1 entity-classification cache (design) + +**Created:** 2026-05-10. +**Status:** approved design, ready for implementation plan. +**Audit foundation:** [docs/research/2026-05-10-tier1-mutation-audit.md](../../research/2026-05-10-tier1-mutation-audit.md). +**Originating issue:** [docs/ISSUES.md](../../ISSUES.md) §#53. +**Phase context:** Phase Post-A.5 polish, Priority 3 (only remaining priority after #52 + #54 closed). + +--- + +## §1. Problem + +`WbDrawDispatcher.Draw` runs full per-frame entity classification at radius=12: walk every visible entity → resolve textures (palette + override) → bucket into groups by `(IBO, FirstIndex, BaseVertex, IndexCount, textureHandle, layer, translucency)`. At ~10K visible entities × ~3 batches average = ~30K classification ops/frame, this dominates the dispatcher's CPU at ~3.5 ms median (post-#52/#54 baseline) — 75% over the Phase A.5 spec's 2.0 ms entity dispatcher budget. + +For ~99.5% of entities (stabs, scenery, cell-mesh, interior fixtures, lifestone), the classification result is *identical* every frame from spawn to despawn. The classification work for those entities is pure waste. + +A first attempt to cache this state — commit `3639a6f`, reverted at `9b49009` — froze NPC animation by caching `meshRef.PartTransform`, which is mutated every frame for entities in `_animatedEntities`. ([memory entry on the failure mode](../../../../../../.claude/projects/C--Users-erikn-source-repos-acdream/memory/project_phase_a5_state.md)) + +This spec is the audit-driven retry. + +--- + +## §2. Goals and non-goals + +### Goals + +1. Drop entity dispatcher CPU median from ~3.5 ms to ≤ 2.0 ms (matches A.5 spec budget) at the horizon-safe preset (radius=4/12). +2. Hold p95 at ≤ 2.5 ms. +3. Hold animation correctness — NPCs animate, the lifestone crystal animates, the player animates, no frozen poses. +4. Hold N.5b conformance sentinel: 94/94 passing (`TerrainSlot|TerrainModernConformance|Wb|MatrixComposition|TextureCacheBindless|SplitFormulaDivergence`) throughout. +5. Hold full test baseline: 1688 passing, 8 pre-existing physics/input failures unchanged. +6. Surface a defensive guard against the prior bug class so the next regression of "static entity gets per-frame mutation snuck in" fails fast instead of silently freezing visuals. + +### Non-goals + +- Tier 2 (static/dynamic split with persistent groups) — separate multi-week phase per [docs/plans/2026-05-10-perf-tiers-2-3-roadmap.md](../../plans/2026-05-10-perf-tiers-2-3-roadmap.md). DO NOT bundle. +- Tier 3 (GPU compute culling) — same roadmap; depends on Tier 2 first. +- Caching for animated entities. Animated entities use today's per-frame classification path, unchanged. +- Persistent-mapped indirect buffer or any other rendering perf work outside the entity classification path. + +--- + +## §3. Design decisions (from brainstorming, 2026-05-10) + +| # | Decision | Rationale | +|---|---|---| +| Q1 | **Static-only cache + DEBUG cross-check** (option `c`) | The prior failure mode was "we silently cached mutable state." DEBUG cross-check converts that class of regression from "user notices a frozen NPC" to "Debug.Assert fires in any dev/test run." Zero Release cost. | +| Q2 | **Separate `EntityClassificationCache` class** (option `B`) at `src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs`, injected into `WbDrawDispatcher` via ctor | Pure-CPU testable in isolation. The single invariant ("static entity = `entity.Id ∉ _animatedEntities`") lives at the top of one ~200-line file rather than scattered through the 940-line dispatcher. | +| Q3 | **Cache the rest pose, not the full model matrix** (option `P`) | Full-matrix would save ~50 µs/frame of mat4 mults at the cost of baking `Position`/`Rotation` into the cache. With rest pose, `Position`/`Rotation` are read live every frame; if a future regression introduces a static-entity Position write, Release builds still produce correct visuals (just with unused cache entries). DEBUG cross-check catches the regression either way. Marginal perf delta dominated by safety. | +| Q4 | **Pre-flatten Setup multi-parts at populate time** (option `F`) | The bulk of the visible CPU win lives here. Today the dispatcher walks `renderData.SetupParts` per frame even though that list is per-GfxObj-immutable. Pre-flattening makes the per-frame hot path branchless: walk one flat list per entity regardless of Setup-vs-non-Setup. Populate cost: one extra mat4 mult per subPart, run once per entity per session. | +| Q5 | **Thorough test coverage** (option `T`): ~10 tests in a new `EntityClassificationCacheTests.cs`, +2 integration tests in `WbDrawDispatcherBucketingTests.cs` | The prior bug would have been caught by the DEBUG cross-check test. The "ObjDescEvent treated as despawn-respawn" test pins a contract from the audit so it can't quietly change. Setup pre-flattening test verifies the per-batch product math without the GL stack. ~150-200 lines of test code. | + +--- + +## §4. The invariant + +The cache rests on this single rule, verified in the audit: + +> **An entity's `MeshRefs` reference, `Position`, `Rotation`, `PaletteOverride`, `HiddenPartsMask`, `ParentCellId`, and `Scale` are stable from spawn to despawn IF AND ONLY IF the entity is NOT in `GameWindow._animatedEntities`.** + +Six write sites in `src/`, five static (one-shot at hydration), one dynamic (per-frame in `TickAnimations`, only for entities in `_animatedEntities`). All seven `Position`/`Rotation` write sites operate on entities in `_animatedEntities`. `PaletteOverride`, `HiddenPartsMask`, `ParentCellId`, `Scale` are `init`-only on `WorldEntity`. `MeshRef` is a `readonly record struct` — no in-place mutation possible. See [audit §1, §3, §4](../../research/2026-05-10-tier1-mutation-audit.md#1-entitymeshrefs---write-sites-the-core-question). + +The DEBUG cross-check (§6.5) is the safety net for any future regression that violates this rule. + +--- + +## §5. Architecture + +``` + ┌─────────────────────────────────┐ + │ GameWindow │ + │ └─ _animatedEntities (dict) │ ← gating predicate + │ └─ _classificationCache (NEW) ─┼──┐ + │ └─ _wbDrawDispatcher │ │ + └──────────────────┬──────────────┘ │ + │ │ + ▼ │ + ┌─────────────────────────────────┐ │ + │ WbDrawDispatcher (MODIFIED) │ │ + │ └─ Draw(...) │ │ + │ └─ per (entity, partIdx): │ │ + │ ├─ animated? → slow path │ │ + │ ├─ cache hit? → fast path┼──┤ + │ └─ cache miss? → slow │ │ + │ path + populate ──────┼──┘ + └─────────────────────────────────┘ + ▲ + │ ctor injection + │ + ┌─────────────────────────────────┐ + │ EntityClassificationCache (NEW)│ + │ └─ Dictionary │ + │ └─ TryGet(id, out CachedBatch[])│ + │ └─ Populate(id, partIdx, ...) │ + │ └─ InvalidateEntity(id) │ + │ └─ InvalidateLandblock(lbId) │ + │ └─ [DEBUG] CrossCheck(...) │ + └─────────────────────────────────┘ + ▲ + │ invalidation calls + │ + ┌─────────────────────────────────┐ + │ GameWindow.RemoveLiveEntity… ──┘ + │ GpuWorldState.RemoveEntities… │ (or wired via callback) + └─────────────────────────────────┘ +``` + +### §5.1 Cache shape + +```csharp +namespace AcDream.App.Rendering.Wb; + +/// +/// Per-(entity, partIdx, batchIdx) classification result. Stored flat in +/// EntityCacheEntry.Batches — one entry per (logical-part, batch), where +/// for a Setup MeshRef each subPart contributes its own entries. +/// +public readonly record struct CachedBatch( + GroupKey Key, // bucket identity (matches the dispatcher's private GroupKey) + ulong BindlessTextureHandle, // resolved texture (post-palette + override) + Matrix4x4 RestPose); // meshRef.PartTransform (or subPart.PartTransform * meshRef.PartTransform for Setup) + +internal sealed class EntityCacheEntry +{ + public required uint EntityId; + public required uint LandblockHint; // for InvalidateLandblock sweep + public required CachedBatch[] Batches; // flat across (partIdx, batchIdx); ordered as classification produced them +} + +public sealed class EntityClassificationCache +{ + private readonly Dictionary _entries = new(); + + public bool TryGet(uint entityId, out EntityCacheEntry entry); + public void Populate(uint entityId, uint landblockHint, CachedBatch[] batches); + public void InvalidateEntity(uint entityId); + public void InvalidateLandblock(uint landblockId); + public int Count => _entries.Count; // diag + +#if DEBUG + public void DebugCrossCheck( + uint entityId, + Matrix4x4 entityWorld, + IReadOnlyList liveMeshRefs, + // …enough live state to recompute model matrices and assert match + ); +#endif +} +``` + +`GroupKey` is defined privately inside `WbDrawDispatcher` today (lines 923-930); promote to internal or pass an opaque payload through. Implementation detail; settle in writing-plans. + +### §5.2 Dispatcher integration (the per-entity branch) + +```csharp +// Inside WbDrawDispatcher.Draw, replacing today's per-(entity, partIdx) body +// at lines 367-423. + +foreach (var (entity, partIdx) in _walkScratch) +{ + if (diag) _entitiesSeen++; + + var entityWorld = + Matrix4x4.CreateFromQuaternion(entity.Rotation) * + Matrix4x4.CreateTranslation(entity.Position); + + bool isAnimated = animatedEntityIds?.Contains(entity.Id) == true; + if (!isAnimated && _cache.TryGet(entity.Id, out var entry)) + { + // Fast path: cache hit on a static entity. + foreach (var cached in entry.Batches) + { + if (!_groups.TryGetValue(cached.Key, out var grp)) + { + grp = new InstanceGroup { /* …materialize from key… */ }; + _groups[cached.Key] = grp; + } + grp.Matrices.Add(cached.RestPose * entityWorld); + } + +#if DEBUG + _cache.DebugCrossCheck(entity.Id, entityWorld, entity.MeshRefs, /*…*/); +#endif + + if (diag) _entitiesDrawn++; + continue; + } + + // Slow path: animated entity, OR cache miss. + // Run today's classification, optionally collecting into a populate buffer + // when !isAnimated. + var collector = isAnimated ? null : _populateScratch; + collector?.Clear(); + + // …today's TryGetRenderData / SetupParts walk / ClassifyBatches … + // ClassifyBatches now also writes (key, texHandle, restPose) into + // `collector` when collector is non-null. + + if (collector is not null && collector.Count > 0) + { + _cache.Populate(entity.Id, /*landblockHint*/ ResolveLandblockHint(entity), + collector.ToArray()); + } +} +``` + +`ClassifyBatches` is extended to optionally append into a caller-supplied `List`. When the collector is null (animated path), behavior is unchanged from today. When non-null (cache-miss path on static entities), each emitted batch also produces a `CachedBatch` record. + +### §5.3 Invalidation wiring + +Two invalidation events: + +1. **Per-entity despawn** at [GameWindow.cs:2933-2935](../../../src/AcDream.App/Rendering/GameWindow.cs#L2933) — add `_classificationCache.InvalidateEntity(existingEntity.Id);` next to `_animatedEntities.Remove(...)`. +2. **Landblock demote / unload** — `GpuWorldState.RemoveEntitiesFromLandblock` is the choke point. Wire one of: + - **(W1)** Add an `Action?` callback parameter; `GameWindow` wires it to `_classificationCache.InvalidateEntity`. Cleaner separation. + - **(W2)** Pass the cache directly into `GpuWorldState`. Less ceremony. + - **(W3)** Call `_classificationCache.InvalidateLandblock(landblockId)` from `StreamingController.Tick` before invoking `RemoveEntitiesFromLandblock`. Requires the cache to maintain `LandblockHint` correctly per entry. + + Implementation plan picks one. My lean: **(W3)** — the cache already needs `LandblockHint` for the sweep, and `StreamingController` is the natural lifecycle owner. + +### §5.4 Failure modes and recovery + +| Failure mode | Detection | Recovery | +|---|---|---| +| Future regression adds `MeshRefs` write site for static entity | DEBUG cross-check `Debug.Assert` fires in dev runs | Audit + fix source. Cross-check stays as guard. | +| Future regression adds `Position`/`Rotation` write site for static entity | DEBUG cross-check (compares `RestPose * liveEntityWorld` against live `meshRef.PartTransform * liveEntityWorld`) | Same. | +| Despawn fires but invalidation not wired | Despawn test asserts `cache.TryGet(id, …) == false` post-call | TDD test catches in CI. | +| Landblock unload misses cache invalidation | `RemoveEntitiesFromLandblock` test asserts every entry with matching `LandblockHint` is gone | TDD test catches in CI. | +| Animated→static membership flip leaves stale entry | No-op (membership predicate skips cache for animated entries; if entity later flips static, cache miss → populate fresh) | None needed. | +| Static→animated membership flip leaves stale entry | No-op (predicate now skips cache; entry sits unused until despawn) | None needed. | +| Cache memory growth | At radius=12: ~10K static entities × ~3-10 batches × ~64 bytes = ~2-6 MB total | None needed. | +| Cache hit on a `_meshAdapter.TryGetRenderData` mesh that subsequently becomes unavailable (theoretical — adapter is session-stable) | N/A — adapter doesn't evict during play | N/A | + +--- + +## §6. Components and their contracts + +### §6.1 `EntityClassificationCache` (NEW) + +**File:** `src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs` + +**Public surface:** + +```csharp +public sealed class EntityClassificationCache +{ + public bool TryGet(uint entityId, out EntityCacheEntry entry); + public void Populate(uint entityId, uint landblockHint, CachedBatch[] batches); + public void InvalidateEntity(uint entityId); + public void InvalidateLandblock(uint landblockId); + public int Count { get; } // for diag +} +``` + +**Invariants:** + +- `Populate` overwrites any existing entry for `entityId` (defensive: handles a populate that races with a partial despawn). +- `InvalidateEntity` is idempotent (no-throw on missing id). +- `InvalidateLandblock` walks all entries; entries whose `LandblockHint == landblockId` are removed. +- `TryGet` is read-only; never mutates. + +**Threading:** dispatcher runs on the render thread. All cache operations are render-thread only. No locking needed. + +### §6.2 `WbDrawDispatcher` (MODIFIED) + +**File:** `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs` + +**Constructor change:** add `EntityClassificationCache classificationCache` parameter; assign to a private `readonly` field. + +**`Draw` change:** the per-entity body at lines ~367-423 is restructured per §5.2. The `WalkEntitiesInto` walk and the GL state setup phases (sort, upload, two `glMultiDrawElementsIndirect` calls) are unchanged. + +**`ClassifyBatches` change:** add optional `List? collector` parameter. When non-null, every classified `(key, texHandle, restPose)` triple is also appended to the collector. Today's behavior preserved for animated entities (collector is null). + +**`ResolveLandblockHint(entity)`:** small helper that returns the landblock id the cache should associate with the entity, for `InvalidateLandblock` sweeps. For dat-loaded entities, this is the landblock the entity was hydrated into. For live-spawned entities, it's the entity's current `Position`-implied landblock at spawn time (or `0` if landblock-invalidation isn't expected to fire — live entities are invalidated by `InvalidateEntity` on despawn). + +### §6.3 `GameWindow` (MODIFIED) + +**File:** `src/AcDream.App/Rendering/GameWindow.cs` + +**Construction:** instantiate `EntityClassificationCache`, pass to dispatcher ctor. + +**Despawn hook:** at line 2935 (inside `RemoveLiveEntityByServerGuid`), add `_classificationCache.InvalidateEntity(existingEntity.Id);` adjacent to `_animatedEntities.Remove(...)`. + +### §6.4 `GpuWorldState` and/or `StreamingController` (MODIFIED, exact split per W1/W2/W3) + +Implementation plan picks one of W1/W2/W3 from §5.3. The wiring lands invalidation calls at the LB demote / unload boundary. + +### §6.5 DEBUG cross-check + +```csharp +#if DEBUG +public void DebugCrossCheck( + uint entityId, + Matrix4x4 entityWorld, + IReadOnlyList liveMeshRefs, + Func tryGetRenderData, + AcSurfaceMetadataTable metaTable, + Func resolveTexture, + WorldEntity entity, + ulong palHash) +{ + if (!_entries.TryGetValue(entityId, out var entry)) return; + + // Re-classify from live state and compare against cached batches one-by-one. + int idx = 0; + foreach (var meshRef in liveMeshRefs) + { + var renderData = tryGetRenderData(meshRef.GfxObjId); + if (renderData is null) continue; + var setupParts = renderData.IsSetup ? renderData.SetupParts : OnePart(meshRef); + foreach (var (subGfxId, subTransform) in setupParts) + { + var subData = tryGetRenderData(subGfxId); + if (subData is null) continue; + var liveRestPose = renderData.IsSetup + ? subTransform * meshRef.PartTransform + : meshRef.PartTransform; + for (int b = 0; b < subData.Batches.Count; b++) + { + var batch = subData.Batches[b]; + var liveTex = resolveTexture(entity, meshRef, batch, palHash); + Debug.Assert(idx < entry.Batches.Length, + $"cache size mismatch for entity {entityId}"); + var cached = entry.Batches[idx]; + Debug.Assert(MatrixApproxEqual(cached.RestPose, liveRestPose, 1e-5f), + $"RestPose drift for entity {entityId} batch {idx}"); + Debug.Assert(cached.BindlessTextureHandle == liveTex, + $"texture drift for entity {entityId} batch {idx}"); + idx++; + } + } + } + Debug.Assert(idx == entry.Batches.Length, + $"cache batch count mismatch for entity {entityId}"); +} +#endif +``` + +The cross-check duplicates the slow-path classification against live state and compares to cached. If any drift is detected, the assert fires in dev runs with an actionable message. Zero cost in Release. + +--- + +## §7. Test plan + +### §7.1 New tests — `EntityClassificationCacheTests.cs` + +**File:** `tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs` + +| # | Test | What it verifies | +|---|---|---| +| 1 | `TryGet_EmptyCache_ReturnsFalse` | Baseline. | +| 2 | `Populate_ThenTryGet_ReturnsBatchesInOrder` | Round-trip. | +| 3 | `Populate_OverridesExistingEntry` | Defensive overwrite. | +| 4 | `InvalidateEntity_RemovesEntry` | Entity despawn invalidation. | +| 5 | `InvalidateEntity_OnMissingId_NoThrow` | Idempotent. | +| 6 | `InvalidateLandblock_RemovesAllMatchingEntries` | LB demote invalidation, single LB. | +| 7 | `InvalidateLandblock_LeavesNonMatchingEntries` | LB sweep is precise. | +| 8 | `InvalidateLandblock_OnMissingLb_NoThrow` | Idempotent. | +| 9 | `Count_TracksLiveEntries` | Diag accuracy. | +| 10 | `Populate_WithEmptyBatches_StoresEmptyEntry` | Edge case (entity with zero classifiable batches). | + +### §7.2 Extended tests — `WbDrawDispatcherBucketingTests.cs` + +| # | Test | What it verifies | +|---|---|---| +| 11 | `Draw_StaticEntity_RoutesThroughCache` | Spawn one static entity; first frame populates the cache; second frame's draw call doesn't invoke `ClassifyBatches` (verify via spy / counter on a mock `WbMeshAdapter`). | +| 12 | `Draw_AnimatedEntity_BypassesCache` | Spawn one entity in `animatedEntityIds`; verify cache is never populated for it; `ClassifyBatches` runs every frame. | + +### §7.3 (DEBUG-only) Cross-check test + +| # | Test | What it verifies | +|---|---|---| +| 13 | `DebugCrossCheck_DetectsMutatedRestPose` | Populate with synthetic data, mutate the live `MeshRef` list, invoke `DebugCrossCheck`, assert fires. Wrapped in `#if DEBUG`. | + +### §7.4 Setup pre-flatten lock-in + +| # | Test | What it verifies | +|---|---|---| +| 14 | `Populate_SetupMultiPart_StoresFlatBatchPerSubPart` | Synthetic Setup with N subParts × M batches each → cache stores N × M entries with the expected `RestPose` products. | + +### §7.5 Lifecycle integration + +| # | Test | What it verifies | +|---|---|---| +| 15 | `DespawnRespawn_UnderReusedId_RepopulatesFresh` | Populate, invalidate, populate again under same id with different batches → final state matches second populate. (Pins the audit's ObjDescEvent contract — ObjDescEvent is despawn+respawn, not in-place mutation. Audit §1 cites this.) | + +Total new tests: 15. Some can collapse if overlap is identified during implementation; baseline is "≥ 10 in `EntityClassificationCacheTests` + ≥ 2 in dispatcher integration + ≥ 1 DEBUG cross-check". + +### §7.6 Sentinel and baseline (existing tests, must stay green) + +- N.5b conformance sentinel: filter `TerrainSlot|TerrainModernConformance|Wb|MatrixComposition|TextureCacheBindless|SplitFormulaDivergence` → 94 passing. +- Full suite: 1688 passing, 8 pre-existing failures unchanged. + +--- + +## §8. Sequencing for implementation + +(The implementation plan from `superpowers:writing-plans` will refine this into per-task increments. Sketch:) + +1. **Skeleton + tests 1-10.** Add `CachedBatch`, `EntityCacheEntry`, `EntityClassificationCache`. Tests 1-10 in the new file. Cache exists but isn't wired to anything yet. +2. **Setup pre-flatten test (test 14) + populate path.** Synthetic `CachedBatch[]` populate; verify `Count` and `TryGet` round-trip on multi-part data shapes. +3. **Wire dispatcher: cache miss + populate.** Modify `WbDrawDispatcher.Draw` and `ClassifyBatches`. First-frame static entity populates; subsequent frames still go through slow path (cache hit branch not yet in). Build green. +4. **Wire dispatcher: cache hit + DEBUG cross-check.** Cache-hit fast path. Tests 11, 12, 13 added. +5. **Wire invalidation hooks.** `InvalidateEntity` from `RemoveLiveEntityByServerGuid`; `InvalidateLandblock` per chosen W1/W2/W3 from §5.3. Test 15. +6. **Visual gate.** Launch + walk Holtburg → North Yanshi at horizon-safe preset. Verify NPC animates, lifestone renders, buildings at correct positions. +7. **Perf gate.** `ACDREAM_WB_DIAG=1`; capture entity dispatcher cpu_us median + p95 over a ≥ 30s standstill at center of Holtburg. Confirm median ≤ 2.0 ms, p95 ≤ 2.5 ms. +8. **Ship.** Commit chain. Close #53 in `docs/ISSUES.md` Recently closed. Update `CLAUDE.md` "Currently in flight" (closes the post-A.5 polish phase). Update memory if any new gotchas surfaced. + +--- + +## §9. Acceptance criteria (whole spec) + +- [ ] `EntityClassificationCache.cs` exists with the public surface in §6.1. +- [ ] `WbDrawDispatcher` accepts the cache via ctor and routes static entities through the cache; animated entities bypass. +- [ ] `RemoveLiveEntityByServerGuid` invokes `InvalidateEntity`. +- [ ] LB demote / unload path invokes `InvalidateLandblock` (or per-entity invalidation, per chosen W1/W2/W3). +- [ ] All 15 new tests pass; no existing test regresses; 8 pre-existing failures unchanged. +- [ ] N.5b sentinel: 94/94 passing on every commit. +- [ ] Build green throughout. +- [ ] Visual gate: animation works on a moving NPC, the lifestone renders, buildings are at correct positions, no new artifacts. +- [ ] Perf gate at horizon-safe preset: entity dispatcher cpu_us median ≤ 2.0 ms; p95 ≤ 2.5 ms. +- [ ] ISSUE #53 moved to "Recently closed" with the closing commit SHA. +- [ ] `CLAUDE.md` "Currently in flight" updated to reflect post-A.5 polish phase complete. +- [ ] Memory updated (`project_phase_a5_state.md` or new entry) if any new gotchas surface during implementation. + +--- + +## §10. What this design explicitly does NOT do + +- Touch the animated path. Animated entities use today's `ClassifyBatches` flow unchanged. +- Touch the GPU upload pipeline (`_instanceSsbo`, `_batchSsbo`, `_indirectBuffer`). Same upload shape; just less CPU work to produce the inputs. +- Touch terrain. `TerrainModernRenderer` already runs at ~21 µs median; not in scope. +- Touch sky / particles / EnvCell rendering. All unchanged. +- Add new shader variants. The `mesh_modern.vert` / `mesh_modern.frag` pair is unchanged. +- Add new bindless texture handles. `TextureCache` is read-only from this work; it returns the same handle for the same surface id whether we ask once at populate or every frame. + +--- + +## §11. Open implementation choices for writing-plans + +These survive into the implementation plan because they're tactical (mechanical), not strategic: + +- **W1 vs W2 vs W3 for the LB invalidation wiring** (§5.3). Pick one; stick with it. +- **`GroupKey` visibility.** Today `private` inside the dispatcher. Either promote to `internal` (within `AcDream.App`) or pass an opaque payload through the cache. Either works. Lean: promote to `internal`. +- **`ResolveLandblockHint` placement.** On the dispatcher (uses dispatcher state for live-spawn entities) or on the cache (passed in by caller)? Lean: dispatcher computes it, passes to `Populate`. +- **`_populateScratch` reuse.** Per-frame field on the dispatcher (matches `_walkScratch` pattern) or per-call allocation? Lean: field, matching `_walkScratch`. +- **Test fixtures.** Synthetic `WorldEntity` / `MeshRef` instances may need helper builders. Lean: add a small `EntityClassificationCacheTestFixtures.cs` if the helpers grow past ~30 lines. + +--- + +**End of spec.** Implementation plan owned by `superpowers:writing-plans`. Audit foundation lives at [docs/research/2026-05-10-tier1-mutation-audit.md](../../research/2026-05-10-tier1-mutation-audit.md).