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 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/plans/2026-05-10-issue-53-tier1-cache.md b/docs/superpowers/plans/2026-05-10-issue-53-tier1-cache.md new file mode 100644 index 0000000..91d6210 --- /dev/null +++ b/docs/superpowers/plans/2026-05-10-issue-53-tier1-cache.md @@ -0,0 +1,2023 @@ +# Tier 1 Entity-Classification Cache Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Drop `WbDrawDispatcher` entity dispatcher CPU median from ~3.5 ms to ≤ 2.0 ms by caching per-entity classification results for static entities, while holding animation correctness via a `_animatedEntities` membership predicate and a DEBUG cross-check guard. + +**Architecture:** New pure-CPU class `EntityClassificationCache` (separate file, ctor-injected into the dispatcher) holds `Dictionary`. Dispatcher checks `_animatedEntities` membership at the top of the per-entity loop; static entities go through the cache (miss → populate; hit → fast path that walks the cached flat batch list and appends `RestPose * entityWorld` matrices). Two invalidation hooks: `InvalidateEntity` from `RemoveLiveEntityByServerGuid` (live despawn) and `InvalidateLandblock` from `GpuWorldState.RemoveEntitiesFromLandblock` (LB demote/unload, wired via callback at `GameWindow` construction). DEBUG-only cross-check recomputes live state and asserts it matches cached, catching the prior Tier 1 bug class. + +**Tech Stack:** C# / .NET 10 preview / Silk.NET / xUnit / FluentAssertions. Repository at `C:\Users\erikn\source\repos\acdream`. Worktree branch `claude/friendly-varahamihira-7b8664`. + +**Spec foundation:** [docs/superpowers/specs/2026-05-10-issue-53-tier1-cache-design.md](../specs/2026-05-10-issue-53-tier1-cache-design.md). +**Audit foundation:** [docs/research/2026-05-10-tier1-mutation-audit.md](../../research/2026-05-10-tier1-mutation-audit.md). + +--- + +## File Structure + +| File | Status | Responsibility | +|---|---|---| +| `src/AcDream.App/Rendering/Wb/GroupKey.cs` | NEW | Top-level `internal record struct GroupKey` — extracted from `WbDrawDispatcher` so the cache can reference it without touching dispatcher internals | +| `src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs` | NEW | Pure-CPU cache class; `Dictionary`; `TryGet` / `Populate` / `InvalidateEntity` / `InvalidateLandblock` + DEBUG cross-check | +| `src/AcDream.App/Rendering/Wb/CachedBatch.cs` | NEW | Top-level `public readonly record struct CachedBatch` + `public sealed class EntityCacheEntry` | +| `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs` | MODIFIED | Add cache ctor param; restructure `Draw` per-entity branch; extend `ClassifyBatches` with optional collector | +| `src/AcDream.App/Rendering/GameWindow.cs` | MODIFIED | Construct `EntityClassificationCache`; pass to dispatcher; wire `InvalidateEntity` at the despawn site (line ~2935); wire `InvalidateLandblock` callback into `GpuWorldState` ctor | +| `src/AcDream.App/Streaming/GpuWorldState.cs` | MODIFIED | Optional `Action?` invalidation callback parameter on the constructor; invoked from `RemoveEntitiesFromLandblock` | +| `tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs` | NEW | 12+ pure-CPU tests covering TryGet / Populate / Invalidate paths + Setup pre-flatten + DEBUG cross-check | +| `tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs` | MODIFIED | +2 integration tests for cache routing; existing tests adapted for new ctor param | + +**Out of scope (do NOT touch):** `mesh_modern.vert`, `mesh_modern.frag`, `TerrainModernRenderer`, `WbMeshAdapter`, `TextureCache`, sky/particles/EnvCell renderers, GPU upload pipeline. + +--- + +## Pre-flight (do these before Task 1) + +- [ ] **Confirm working tree clean and on the worktree branch.** + +```bash +git status +git branch --show-current +``` + +Expected: `working tree clean`, current branch `claude/friendly-varahamihira-7b8664`. + +- [ ] **Confirm baseline: build green + 1688/8 tests + 94/94 N.5b sentinel.** + +```powershell +dotnet build +``` + +Expected: `Build succeeded. 0 Error(s)`. + +```powershell +dotnet test --no-build --filter "FullyQualifiedName~TerrainSlot|FullyQualifiedName~TerrainModernConformance|FullyQualifiedName~Wb|FullyQualifiedName~MatrixComposition|FullyQualifiedName~TextureCacheBindless|FullyQualifiedName~SplitFormulaDivergence" +``` + +Expected: `Passed! - Failed: 0, Passed: 94, Skipped: 0, Total: 94`. + +If submodules missing: `git submodule update --init --recursive references/WorldBuilder`. + +--- + +## Phase 1: Cache foundation (Tasks 1-5) + +### Task 1: Extract `GroupKey` to its own file + +**Files:** +- Create: `src/AcDream.App/Rendering/Wb/GroupKey.cs` +- Modify: `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs:923-930` (remove the nested type) + +This is a mechanical refactor so the cache can reference `GroupKey` without depending on `WbDrawDispatcher`'s private members. + +- [ ] **Step 1: Create the new file.** + +`src/AcDream.App/Rendering/Wb/GroupKey.cs`: + +```csharp +using AcDream.Core.Meshing; + +namespace AcDream.App.Rendering.Wb; + +/// +/// Bucket identity for 's per-frame group dictionary. +/// Two (entity, batch) pairs that share the same render +/// in a single glMultiDrawElementsIndirect draw command. Promoted to +/// internal at file scope (was a private nested type) so +/// can store it inside +/// without depending on dispatcher internals. +/// +internal readonly record struct GroupKey( + uint Ibo, + uint FirstIndex, + int BaseVertex, + int IndexCount, + ulong BindlessTextureHandle, + uint TextureLayer, + TranslucencyKind Translucency); +``` + +- [ ] **Step 2: Remove the nested `GroupKey` from the dispatcher.** + +In `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs`, delete lines 923-930 (the `private readonly record struct GroupKey(...)` block). Leave the surrounding code unchanged. + +- [ ] **Step 3: Build to verify the refactor compiled.** + +```powershell +dotnet build +``` + +Expected: `Build succeeded. 0 Error(s)`. If it fails because some test or code referenced `WbDrawDispatcher.GroupKey`, change those references to use the bare `GroupKey` (now `internal` at namespace scope). + +- [ ] **Step 4: Run the full test suite to verify no behavior change.** + +```powershell +dotnet test --no-build +``` + +Expected: `Failed: 8, Passed: 1688` (baseline preserved). + +- [ ] **Step 5: Commit.** + +```bash +git add src/AcDream.App/Rendering/Wb/GroupKey.cs src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs +git commit -m "refactor(render): extract WbDrawDispatcher.GroupKey to internal type at namespace scope + +Mechanical refactor: GroupKey was a private nested record struct on +WbDrawDispatcher. The upcoming EntityClassificationCache (ISSUE #53) needs +to store GroupKey inside CachedBatch records, so it must be visible to +both the dispatcher and the cache. Promoting to internal at file scope is +the smallest change that achieves this. + +No behavior change. 1688 tests pass; 8 pre-existing failures unchanged. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Task 2: Skeleton — `EntityClassificationCache` + `CachedBatch` + first test + +**Files:** +- Create: `src/AcDream.App/Rendering/Wb/CachedBatch.cs` +- Create: `src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs` +- Create: `tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs` + +The test file references the `internal` `GroupKey`; if `AcDream.App` doesn't already grant `InternalsVisibleTo("AcDream.Core.Tests")`, add it as part of this task. + +- [ ] **Step 1: Write the first failing test.** + +`tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs`: + +```csharp +using System.Collections.Generic; +using System.Numerics; +using AcDream.App.Rendering.Wb; +using AcDream.Core.Meshing; +using FluentAssertions; +using Xunit; + +namespace AcDream.Core.Tests.Rendering.Wb; + +public class EntityClassificationCacheTests +{ + [Fact] + public void TryGet_EmptyCache_ReturnsFalse() + { + var cache = new EntityClassificationCache(); + bool found = cache.TryGet(entityId: 42, out var entry); + found.Should().BeFalse(); + entry.Should().BeNull(); + } + + private static CachedBatch MakeCachedBatch( + uint ibo, uint firstIndex, int indexCount, ulong texHandle) + { + var key = new GroupKey( + Ibo: ibo, + FirstIndex: firstIndex, + BaseVertex: 0, + IndexCount: indexCount, + BindlessTextureHandle: texHandle, + TextureLayer: 0, + Translucency: TranslucencyKind.Opaque); + return new CachedBatch(key, texHandle, Matrix4x4.Identity); + } +} +``` + +- [ ] **Step 2: Add `InternalsVisibleTo` if needed.** + +Check if `AcDream.App` already exposes internals to `AcDream.Core.Tests`: + +```powershell +Select-String -Path src/AcDream.App/**/*.cs, src/AcDream.App/AcDream.App.csproj -Pattern "InternalsVisibleTo" +``` + +If no hit, add a new file `src/AcDream.App/Properties/AssemblyInfo.cs`: + +```csharp +using System.Runtime.CompilerServices; + +[assembly: InternalsVisibleTo("AcDream.Core.Tests")] +``` + +(Place it under `Properties/` to follow the conventional .NET assembly-info pattern; if `AcDream.App` already has another conventional location, use that instead.) + +- [ ] **Step 3: Run the test to verify it fails to compile.** + +```powershell +dotnet test --no-build --filter "FullyQualifiedName~EntityClassificationCacheTests" +``` + +Expected: build error — `EntityClassificationCache`, `CachedBatch` don't exist yet. + +- [ ] **Step 4: Create `CachedBatch.cs`.** + +`src/AcDream.App/Rendering/Wb/CachedBatch.cs`: + +```csharp +using System.Numerics; + +namespace AcDream.App.Rendering.Wb; + +/// +/// Per-(entity, partIdx, batchIdx) classification result, stored flat inside +/// . For Setup multi-part MeshRefs each +/// subPart contributes its own entries, with +/// already containing the +/// subPart.PartTransform * meshRef.PartTransform product. +/// +public readonly record struct CachedBatch( + GroupKey Key, + ulong BindlessTextureHandle, + Matrix4x4 RestPose); + +/// +/// One entity's cached classification. is flat across +/// (partIdx, batchIdx) and ordered as WbDrawDispatcher.ClassifyBatches +/// produced them. lets +/// sweep entries +/// efficiently when a landblock demotes or unloads. +/// +public sealed class EntityCacheEntry +{ + public required uint EntityId { get; init; } + public required uint LandblockHint { get; init; } + public required CachedBatch[] Batches { get; init; } +} +``` + +- [ ] **Step 5: Create `EntityClassificationCache.cs` skeleton.** + +`src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs`: + +```csharp +using System.Collections.Generic; + +namespace AcDream.App.Rendering.Wb; + +/// +/// Cache of per-entity classification results for static entities (those NOT +/// in GameWindow._animatedEntities). Holds one +/// per cached entity. The cache is opaque +/// w.r.t. classification logic — it simply stores what callers populate. +/// +/// +/// Invariants: +/// +/// overwrites any existing entry for the same id (defensive). +/// is idempotent (no-throw on missing id). +/// walks all entries; entries whose +/// equals the argument are removed. +/// All operations are render-thread only. No internal locking. +/// +/// +/// +/// +/// Audit foundation: see +/// docs/research/2026-05-10-tier1-mutation-audit.md for why static +/// entities can be cached and what invalidation is needed. +/// +/// +public sealed class EntityClassificationCache +{ + private readonly Dictionary _entries = new(); + + /// Number of cached entities — for diagnostics. + public int Count => _entries.Count; + + /// + /// Look up an entity's cached classification. Returns true with + /// the entry on hit; false with set to + /// null on miss. + /// + public bool TryGet(uint entityId, out EntityCacheEntry? entry) + => _entries.TryGetValue(entityId, out entry); +} +``` + +- [ ] **Step 6: Run the test to verify it passes.** + +```powershell +dotnet build +dotnet test --no-build --filter "FullyQualifiedName~EntityClassificationCacheTests" +``` + +Expected: `Passed: 1, Failed: 0`. + +- [ ] **Step 7: Commit.** + +```bash +git add src/AcDream.App/Rendering/Wb/CachedBatch.cs src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs +test -f src/AcDream.App/Properties/AssemblyInfo.cs && git add src/AcDream.App/Properties/AssemblyInfo.cs +git commit -m "feat(render #53): EntityClassificationCache skeleton + first test + +Adds CachedBatch, EntityCacheEntry, and EntityClassificationCache with +just TryGet (returns false on empty). The skeleton compiles and the first +test (TryGet_EmptyCache_ReturnsFalse) passes. Subsequent tasks add +Populate, InvalidateEntity, InvalidateLandblock, and the dispatcher +integration. Per spec design §6.1. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Task 3: `Populate` + roundtrip + Setup pre-flatten tests + +**Files:** +- Modify: `src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs` +- Modify: `tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs` + +Adds tests #2, #3, #9, #10, #14 from the spec test plan. All exercise the populate-then-tryget round-trip including the Setup pre-flatten shape. + +- [ ] **Step 1: Write the failing tests.** + +Append to `tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs` (just BEFORE the `private static CachedBatch MakeCachedBatch` helper): + +```csharp + [Fact] + public void Populate_ThenTryGet_ReturnsBatchesInOrder() + { + var cache = new EntityClassificationCache(); + var batches = new[] + { + MakeCachedBatch(ibo: 1, firstIndex: 0, indexCount: 6, texHandle: 0xAA), + MakeCachedBatch(ibo: 1, firstIndex: 6, indexCount: 6, texHandle: 0xBB), + }; + + cache.Populate(entityId: 100, landblockHint: 0xA9B40000u, batches); + + cache.TryGet(100, out var entry).Should().BeTrue(); + entry!.EntityId.Should().Be(100u); + entry.LandblockHint.Should().Be(0xA9B40000u); + entry.Batches.Should().Equal(batches); + } + + [Fact] + public void Populate_OverridesExistingEntry() + { + var cache = new EntityClassificationCache(); + cache.Populate(100, 0u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) }); + cache.Populate(100, 0u, new[] { MakeCachedBatch(2, 0, 12, 0xCC) }); + + cache.TryGet(100, out var entry).Should().BeTrue(); + entry!.Batches.Should().HaveCount(1); + entry.Batches[0].BindlessTextureHandle.Should().Be(0xCCu); + } + + [Fact] + public void Count_TracksLiveEntries() + { + var cache = new EntityClassificationCache(); + cache.Count.Should().Be(0); + + cache.Populate(1, 0u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) }); + cache.Count.Should().Be(1); + + cache.Populate(2, 0u, new[] { MakeCachedBatch(2, 0, 6, 0xAA) }); + cache.Count.Should().Be(2); + + // Re-populate same id — should not double-count. + cache.Populate(1, 0u, new[] { MakeCachedBatch(3, 0, 6, 0xBB) }); + cache.Count.Should().Be(2); + } + + [Fact] + public void Populate_WithEmptyBatches_StoresEmptyEntry() + { + var cache = new EntityClassificationCache(); + cache.Populate(entityId: 7, landblockHint: 0u, System.Array.Empty()); + + cache.TryGet(7, out var entry).Should().BeTrue(); + entry!.Batches.Should().BeEmpty(); + } + + [Fact] + public void Populate_SetupMultiPart_StoresFlatBatchPerSubPart() + { + // Synthetic Setup with 3 subParts × 2 batches each = 6 flat entries. + // This pins the spec §3 Q4 decision: pre-flatten Setup multi-parts at + // populate time so the per-frame hot path is branchless. + var cache = new EntityClassificationCache(); + var batches = new CachedBatch[6]; + for (int subPart = 0; subPart < 3; subPart++) + for (int b = 0; b < 2; b++) + { + batches[subPart * 2 + b] = MakeCachedBatch( + ibo: (uint)(subPart + 1), + firstIndex: (uint)(b * 6), + indexCount: 6, + texHandle: (ulong)(0x100 + subPart * 2 + b)); + } + cache.Populate(99, 0u, batches); + + cache.TryGet(99, out var entry).Should().BeTrue(); + entry!.Batches.Should().HaveCount(6); + entry.Batches[0].BindlessTextureHandle.Should().Be(0x100u); + entry.Batches[5].BindlessTextureHandle.Should().Be(0x105u); + } +``` + +- [ ] **Step 2: Run tests, verify they fail to compile.** + +```powershell +dotnet build +``` + +Expected: build error — `Populate` does not exist on `EntityClassificationCache`. + +- [ ] **Step 3: Implement `Populate`.** + +Add to `EntityClassificationCache.cs`: + +```csharp + /// + /// Insert or overwrite a cache entry for . + /// Defensive: if an entry already exists, replaces it. + /// + public void Populate(uint entityId, uint landblockHint, CachedBatch[] batches) + { + _entries[entityId] = new EntityCacheEntry + { + EntityId = entityId, + LandblockHint = landblockHint, + Batches = batches, + }; + } +``` + +- [ ] **Step 4: Run all cache tests.** + +```powershell +dotnet build +dotnet test --no-build --filter "FullyQualifiedName~EntityClassificationCacheTests" +``` + +Expected: 6 tests pass (1 from Task 2 + 5 new). + +- [ ] **Step 5: Commit.** + +```bash +git add src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs +git commit -m "feat(render #53): EntityClassificationCache.Populate + roundtrip tests + +Implements Populate (insert-or-overwrite) and adds 5 tests covering the +populate→TryGet round-trip including the Setup pre-flatten shape. Per +spec test plan §7.1 tests #2, #3, #9, #10, #14. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Task 4: `InvalidateEntity` + tests #4, #5 + +**Files:** +- Modify: `src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs` +- Modify: `tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs` + +- [ ] **Step 1: Write the failing tests.** + +Append (just before the `MakeCachedBatch` helper): + +```csharp + [Fact] + public void InvalidateEntity_RemovesEntry() + { + var cache = new EntityClassificationCache(); + cache.Populate(100, 0u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) }); + cache.TryGet(100, out _).Should().BeTrue(); + + cache.InvalidateEntity(100); + + cache.TryGet(100, out var entry).Should().BeFalse(); + entry.Should().BeNull(); + cache.Count.Should().Be(0); + } + + [Fact] + public void InvalidateEntity_OnMissingId_NoThrow() + { + var cache = new EntityClassificationCache(); + var act = () => cache.InvalidateEntity(99999); + act.Should().NotThrow(); + cache.Count.Should().Be(0); + } +``` + +- [ ] **Step 2: Run tests, verify they fail to compile.** + +```powershell +dotnet build +``` + +Expected: build error — `InvalidateEntity` not defined. + +- [ ] **Step 3: Implement `InvalidateEntity`.** + +Add to `EntityClassificationCache.cs`: + +```csharp + /// + /// Remove the cache entry for . No-op if the + /// id isn't cached. + /// + public void InvalidateEntity(uint entityId) + { + _entries.Remove(entityId); + } +``` + +- [ ] **Step 4: Run tests.** + +```powershell +dotnet test --no-build --filter "FullyQualifiedName~EntityClassificationCacheTests" +``` + +Expected: 8 tests pass. + +- [ ] **Step 5: Commit.** + +```bash +git add src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs +git commit -m "feat(render #53): EntityClassificationCache.InvalidateEntity + tests + +Idempotent removal of a cached entry by entity id. Tests #4 and #5 from +spec §7.1 lock in the contract. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Task 5: `InvalidateLandblock` + tests #6, #7, #8 + +**Files:** +- Modify: `src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs` +- Modify: `tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs` + +- [ ] **Step 1: Write the failing tests.** + +Append (just before the `MakeCachedBatch` helper): + +```csharp + [Fact] + public void InvalidateLandblock_RemovesAllMatchingEntries() + { + var cache = new EntityClassificationCache(); + cache.Populate(1, 0xA9B40000u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) }); + cache.Populate(2, 0xA9B40000u, new[] { MakeCachedBatch(2, 0, 6, 0xBB) }); + cache.Populate(3, 0xA9B40000u, new[] { MakeCachedBatch(3, 0, 6, 0xCC) }); + cache.Count.Should().Be(3); + + cache.InvalidateLandblock(0xA9B40000u); + + cache.Count.Should().Be(0); + cache.TryGet(1, out _).Should().BeFalse(); + cache.TryGet(2, out _).Should().BeFalse(); + cache.TryGet(3, out _).Should().BeFalse(); + } + + [Fact] + public void InvalidateLandblock_LeavesNonMatchingEntries() + { + var cache = new EntityClassificationCache(); + cache.Populate(1, 0xA9B40000u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) }); + cache.Populate(2, 0xA9B50000u, new[] { MakeCachedBatch(2, 0, 6, 0xBB) }); + cache.Populate(3, 0xA9B40000u, new[] { MakeCachedBatch(3, 0, 6, 0xCC) }); + + cache.InvalidateLandblock(0xA9B40000u); + + cache.Count.Should().Be(1); + cache.TryGet(1, out _).Should().BeFalse(); + cache.TryGet(2, out var keep).Should().BeTrue(); + keep!.LandblockHint.Should().Be(0xA9B50000u); + cache.TryGet(3, out _).Should().BeFalse(); + } + + [Fact] + public void InvalidateLandblock_OnMissingLb_NoThrow() + { + var cache = new EntityClassificationCache(); + cache.Populate(1, 0xA9B40000u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) }); + var act = () => cache.InvalidateLandblock(0xDEADBEEFu); + act.Should().NotThrow(); + cache.Count.Should().Be(1); + } +``` + +- [ ] **Step 2: Run tests, verify failure.** + +```powershell +dotnet build +``` + +Expected: build error — `InvalidateLandblock` not defined. + +- [ ] **Step 3: Implement `InvalidateLandblock`.** + +Add to `EntityClassificationCache.cs`: + +```csharp + /// + /// Remove every cache entry whose + /// equals . Used by the streaming pipeline + /// when a landblock demotes from near to far or unloads. No-op if no + /// entries match. + /// + public void InvalidateLandblock(uint landblockId) + { + if (_entries.Count == 0) return; + + // Collect the ids to remove first to avoid mutating the dict during iteration. + // Buffered locally because the typical case removes ~all entries in the LB + // (which is still small relative to the total cache). + List? toRemove = null; + foreach (var (id, entry) in _entries) + { + if (entry.LandblockHint == landblockId) + { + toRemove ??= new List(); + toRemove.Add(id); + } + } + if (toRemove is null) return; + foreach (var id in toRemove) _entries.Remove(id); + } +``` + +- [ ] **Step 4: Run tests.** + +```powershell +dotnet test --no-build --filter "FullyQualifiedName~EntityClassificationCacheTests" +``` + +Expected: 11 tests pass (1 + 5 + 2 + 3). + +- [ ] **Step 5: Commit.** + +```bash +git add src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs +git commit -m "feat(render #53): EntityClassificationCache.InvalidateLandblock + tests + +Sweep-by-landblock removal for the streaming demote/unload path. Tests +#6, #7, #8 from spec §7.1 lock in: (a) all matching entries removed, (b) +non-matching entries preserved, (c) idempotent on missing LB. + +Phase 1 (cache foundation) complete. 11 cache tests passing. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Phase 1 checkpoint + +- [ ] **Run full suite + N.5b sentinel before moving to Phase 2.** + +```powershell +dotnet test --no-build --filter "FullyQualifiedName~TerrainSlot|FullyQualifiedName~TerrainModernConformance|FullyQualifiedName~Wb|FullyQualifiedName~MatrixComposition|FullyQualifiedName~TextureCacheBindless|FullyQualifiedName~SplitFormulaDivergence" +``` + +Expected: 94 + 11 = at least 105 passing in the filter (the new EntityClassificationCacheTests are matched by `Wb`). + +```powershell +dotnet test --no-build +``` + +Expected: `Failed: 8, Passed: 1699` (8 pre-existing + 11 new cache tests added on top of 1688 baseline). + +If anything regresses here, STOP and diagnose before Phase 2. + +--- + +## Phase 2: Dispatcher integration (Tasks 6-10) + +### Task 6: Plumb landblockId through `_walkScratch` + +**Files:** +- Modify: `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs` (~lines 116, 192, 220, 241-247, 367, 273, 299-300) +- Modify: existing tests in `tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs` if they construct walkScratch tuples + +The cache populates `LandblockHint` from the walk's outer-loop `LandblockEntry.LandblockId`. Today the inner `_walkScratch` is `List<(WorldEntity Entity, int MeshRefIndex)>` — no LB. Extend to a 3-tuple including the landblock id. + +- [ ] **Step 1: Find every reference to the existing 2-tuple shape.** + +```powershell +Select-String -Path src/**/*.cs, tests/**/*.cs -Pattern "List<\(WorldEntity" +Select-String -Path src/**/*.cs, tests/**/*.cs -Pattern "WalkResult" +``` + +Expected hits: `WbDrawDispatcher.cs` (declaration + WalkResult type + body), possibly `WbDrawDispatcherBucketingTests.cs`. + +- [ ] **Step 2: Update the `_walkScratch` field type and `WalkResult.ToDraw` type.** + +In `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs`: + +Change line 116: +```csharp +private readonly List<(WorldEntity Entity, int MeshRefIndex)> _walkScratch = new(); +``` +to: +```csharp +private readonly List<(WorldEntity Entity, int MeshRefIndex, uint LandblockId)> _walkScratch = new(); +``` + +Change line 192: +```csharp +public List<(WorldEntity Entity, int MeshRefIndex)> ToDraw; +``` +to: +```csharp +public List<(WorldEntity Entity, int MeshRefIndex, uint LandblockId)> ToDraw; +``` + +- [ ] **Step 3: Update `WalkEntities` (the test-friendly overload) signature.** + +Change line 220-233: +```csharp +internal static WalkResult WalkEntities( + IEnumerable landblockEntries, + FrustumPlanes? frustum, + uint? neverCullLandblockId, + HashSet? visibleCellIds, + HashSet? animatedEntityIds) +{ + var scratch = new List<(WorldEntity Entity, int MeshRefIndex)>(); + var result = new WalkResult { ToDraw = scratch }; + WalkEntitiesInto( + landblockEntries, frustum, neverCullLandblockId, + visibleCellIds, animatedEntityIds, scratch, ref result); + return result; +} +``` +to: +```csharp +internal static WalkResult WalkEntities( + IEnumerable landblockEntries, + FrustumPlanes? frustum, + uint? neverCullLandblockId, + HashSet? visibleCellIds, + HashSet? animatedEntityIds) +{ + var scratch = new List<(WorldEntity Entity, int MeshRefIndex, uint LandblockId)>(); + var result = new WalkResult { ToDraw = scratch }; + WalkEntitiesInto( + landblockEntries, frustum, neverCullLandblockId, + visibleCellIds, animatedEntityIds, scratch, ref result); + return result; +} +``` + +- [ ] **Step 4: Update `WalkEntitiesInto` signature + body.** + +Change line 241-247 to take the new tuple shape: + +```csharp +internal static void WalkEntitiesInto( + IEnumerable landblockEntries, + FrustumPlanes? frustum, + uint? neverCullLandblockId, + HashSet? visibleCellIds, + HashSet? animatedEntityIds, + List<(WorldEntity Entity, int MeshRefIndex, uint LandblockId)> scratch, + ref WalkResult result) +``` + +Inside the body, every `scratch.Add((entity, i))` becomes `scratch.Add((entity, i, entry.LandblockId))`. Two such lines: ~273 (animated-only branch) and ~299-300 (full walk branch). Concretely: + +Line ~273 (inside the animated-only frustum-culled branch): +```csharp +for (int i = 0; i < entity.MeshRefs.Count; i++) + scratch.Add((entity, i, entry.LandblockId)); +``` + +Line ~299-300 (inside the full walk branch): +```csharp +for (int i = 0; i < entity.MeshRefs.Count; i++) + scratch.Add((entity, i, entry.LandblockId)); +``` + +- [ ] **Step 5: Update the consumer in `Draw`.** + +At line 367: +```csharp +foreach (var (entity, partIdx) in _walkScratch) +``` +becomes: +```csharp +foreach (var (entity, partIdx, landblockId) in _walkScratch) +``` + +The `landblockId` is unused for now (consumed in Task 9 for `Populate`'s `landblockHint` argument). Suppress any `landblockId` unused-variable warning by prefixing `_` if necessary, but only if the C# compiler emits a warning (it shouldn't for tuple deconstruction). + +- [ ] **Step 6: Build to verify the type plumbed cleanly.** + +```powershell +dotnet build +``` + +Expected: `Build succeeded. 0 Error(s)`. If existing tests in `WbDrawDispatcherBucketingTests.cs` reference the 2-tuple shape, update them to the 3-tuple form (add `0u` or a deterministic landblock id as the third element). + +- [ ] **Step 7: Run full suite.** + +```powershell +dotnet test --no-build +``` + +Expected: `Failed: 8, Passed: 1699` — same as Phase 1 checkpoint. The walk now carries an extra field but no behavior changed yet. + +- [ ] **Step 8: Commit.** + +```bash +git add src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs +git commit -m "refactor(render #53): plumb landblockId through WbDrawDispatcher walkScratch + +Extends the walk scratch tuple from (entity, meshRefIndex) to +(entity, meshRefIndex, landblockId). The dispatcher's per-entity loop now +has the landblock id available for EntityClassificationCache.Populate's +landblockHint argument (consumed in Task 9). No behavior change. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Task 7: Wire `EntityClassificationCache` into the dispatcher ctor + `GameWindow` + +**Files:** +- Modify: `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs` (ctor signature + private field) +- Modify: `src/AcDream.App/Rendering/GameWindow.cs` (instantiate + pass) +- Modify: `tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs` (existing test fixtures pass an empty cache) + +- [ ] **Step 1: Add the field + ctor parameter to `WbDrawDispatcher`.** + +In `WbDrawDispatcher.cs`, add a private readonly field next to the others (~line 70): + +```csharp +private readonly EntityClassificationCache _cache; +``` + +Update the ctor signature at line 142-148: + +```csharp +public WbDrawDispatcher( + GL gl, + Shader shader, + TextureCache textures, + WbMeshAdapter meshAdapter, + EntitySpawnAdapter entitySpawnAdapter, + BindlessSupport bindless, + EntityClassificationCache classificationCache) +``` + +Add the assignment at the end of the ctor body (~line 165), with the existing null-checks: + +```csharp +ArgumentNullException.ThrowIfNull(classificationCache); +_cache = classificationCache; +``` + +- [ ] **Step 2: Construct and pass the cache from `GameWindow`.** + +Find the `WbDrawDispatcher` instantiation in `src/AcDream.App/Rendering/GameWindow.cs`: + +```powershell +Select-String -Path src/AcDream.App/Rendering/GameWindow.cs -Pattern "new WbDrawDispatcher" +``` + +Add a private field on `GameWindow`: + +```csharp +private readonly AcDream.App.Rendering.Wb.EntityClassificationCache _classificationCache = new(); +``` + +(Place it adjacent to the existing `_animatedEntities` field at line ~160 — they're conceptually paired.) + +Update the `new WbDrawDispatcher(...)` call site to include the new argument: + +```csharp +_wbDrawDispatcher = new AcDream.App.Rendering.Wb.WbDrawDispatcher( + /* … existing args … */, + _classificationCache); +``` + +- [ ] **Step 3: Update existing dispatcher tests.** + +In `tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs`, find every `new WbDrawDispatcher(...)` and append `new EntityClassificationCache()` as the final argument. (If tests use a builder/helper method, update that.) + +```powershell +Select-String -Path tests/AcDream.Core.Tests/Rendering/Wb/*.cs -Pattern "new WbDrawDispatcher" +``` + +For each hit, add the new argument. + +- [ ] **Step 4: Build to verify everything compiled.** + +```powershell +dotnet build +``` + +Expected: `Build succeeded. 0 Error(s)`. + +- [ ] **Step 5: Run full suite.** + +```powershell +dotnet test --no-build +``` + +Expected: `Failed: 8, Passed: 1699`. The cache is wired into the dispatcher but isn't used yet — no behavior change. + +- [ ] **Step 6: Commit.** + +```bash +git add src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs src/AcDream.App/Rendering/GameWindow.cs tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs +git commit -m "feat(render #53): inject EntityClassificationCache into WbDrawDispatcher + +Adds the cache as a constructor parameter on WbDrawDispatcher and a +private field on GameWindow. The cache is passed through but not yet +consumed by Draw — that wires up in Task 9 (cache miss / populate) and +Task 10 (cache hit / fast path). + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Task 8: Extend `ClassifyBatches` with optional collector + +**Files:** +- Modify: `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs` (lines 707-759 — the `ClassifyBatches` method) + +- [ ] **Step 1: Change `ClassifyBatches` signature.** + +Change the method declaration at line 707: + +```csharp +private void ClassifyBatches( + ObjectRenderData renderData, + ulong gfxObjId, + Matrix4x4 model, + WorldEntity entity, + MeshRef meshRef, + ulong palHash, + AcSurfaceMetadataTable metaTable) +``` + +to: + +```csharp +private void ClassifyBatches( + ObjectRenderData renderData, + ulong gfxObjId, + Matrix4x4 model, + WorldEntity entity, + MeshRef meshRef, + ulong palHash, + AcSurfaceMetadataTable metaTable, + Matrix4x4 restPose, + List? collector = null) +``` + +The new `restPose` parameter is the model-matrix component WITHOUT `entityWorld` baked in — i.e. `meshRef.PartTransform` for non-Setup, or `subPart.PartTransform * meshRef.PartTransform` for Setup. Caller computes it. + +- [ ] **Step 2: Append to the collector inside the per-batch loop.** + +At the bottom of the for loop (after `grp.Matrices.Add(model);` at line 757), add: + +```csharp + collector?.Add(new CachedBatch(key, texHandle, restPose)); +``` + +The full updated block (lines 738-758): + +```csharp + var key = new GroupKey( + batch.IBO, batch.FirstIndex, (int)batch.BaseVertex, + batch.IndexCount, texHandle, texLayer, translucency); + + if (!_groups.TryGetValue(key, out var grp)) + { + grp = new InstanceGroup + { + Ibo = batch.IBO, + FirstIndex = batch.FirstIndex, + BaseVertex = (int)batch.BaseVertex, + IndexCount = batch.IndexCount, + BindlessTextureHandle = texHandle, + TextureLayer = texLayer, + Translucency = translucency, + }; + _groups[key] = grp; + } + grp.Matrices.Add(model); + collector?.Add(new CachedBatch(key, texHandle, restPose)); + } + } +``` + +- [ ] **Step 3: Update `ClassifyBatches` call sites in `Draw` to pass `restPose`.** + +At line 411 (Setup branch): +```csharp +ClassifyBatches(partData, partGfxObjId, model, entity, meshRef, palHash, metaTable); +``` +becomes: +```csharp +var restPose = partTransform * meshRef.PartTransform; +ClassifyBatches(partData, partGfxObjId, model, entity, meshRef, palHash, metaTable, restPose); +``` + +At line 418 (non-Setup branch): +```csharp +var model = meshRef.PartTransform * entityWorld; +ClassifyBatches(renderData, gfxObjId, model, entity, meshRef, palHash, metaTable); +``` +becomes: +```csharp +var model = meshRef.PartTransform * entityWorld; +ClassifyBatches(renderData, gfxObjId, model, entity, meshRef, palHash, metaTable, restPose: meshRef.PartTransform); +``` + +(Use named-arg form on the non-Setup branch to avoid name collision with the Setup branch's local `restPose`.) + +- [ ] **Step 4: Build + test.** + +```powershell +dotnet build +dotnet test --no-build +``` + +Expected: `Failed: 8, Passed: 1699`. No behavior change yet — collector defaults to null. + +- [ ] **Step 5: Commit.** + +```bash +git add src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs +git commit -m "feat(render #53): add optional CachedBatch collector to ClassifyBatches + +ClassifyBatches now accepts a restPose parameter (the model-matrix +component without entityWorld baked in) and an optional collector. When +collector is non-null, each classified batch is appended as a CachedBatch +record. Defaults preserve today's behavior. Used in Task 9 to populate +the cache on a static-entity miss. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Task 9: Wire dispatcher cache-miss path (populate on first frame; no fast-path yet) + +**Files:** +- Modify: `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs` (around lines 367-423) + +This task adds the populate logic without the cache-hit fast path. After this task, every static entity's slow path runs exactly once (first frame visible) and produces a populated cache entry; subsequent frames still run the slow path because the fast-path branch isn't in yet. Task 10 adds the fast path. + +The split is deliberate so we can land + verify each half independently. + +- [ ] **Step 1: Add the populate scratch field.** + +Near the other per-frame scratch fields (~line 116): + +```csharp +private readonly List _populateScratch = new(); +``` + +- [ ] **Step 2: Restructure the per-entity loop in `Draw`.** + +Replace lines 367-423 (the foreach + body up through `if (diag && drewAny) _entitiesDrawn++;`) with: + +```csharp +foreach (var (entity, partIdx, landblockId) in _walkScratch) +{ + if (diag) _entitiesSeen++; + + var entityWorld = + Matrix4x4.CreateFromQuaternion(entity.Rotation) * + Matrix4x4.CreateTranslation(entity.Position); + + bool isAnimated = animatedEntityIds?.Contains(entity.Id) == true; + + // Compute palette-override hash ONCE per entity (perf #4). + ulong palHash = 0; + if (entity.PaletteOverride is not null) + palHash = TextureCache.HashPaletteOverride(entity.PaletteOverride); + + var meshRef = entity.MeshRefs[partIdx]; + ulong gfxObjId = meshRef.GfxObjId; + + var renderData = _meshAdapter.TryGetRenderData(gfxObjId); + if (renderData is null) + { + if (diag) _meshesMissing++; + continue; + } + if (anyVao == 0) anyVao = renderData.VAO; + + // Cache-miss path (animated entities skip cache entirely). + // Static entities collect into _populateScratch on the first frame + // they're visible, so the cache has fresh data for the next frame. + var collector = isAnimated ? null : _populateScratch; + collector?.Clear(); + + bool drewAny = false; + if (renderData.IsSetup && renderData.SetupParts.Count > 0) + { + foreach (var (partGfxObjId, partTransform) in renderData.SetupParts) + { + var partData = _meshAdapter.TryGetRenderData(partGfxObjId); + if (partData is null) continue; + + var model = ComposePartWorldMatrix( + entityWorld, meshRef.PartTransform, partTransform); + var restPose = partTransform * meshRef.PartTransform; + + ClassifyBatches(partData, partGfxObjId, model, entity, meshRef, + palHash, metaTable, restPose, collector); + drewAny = true; + } + } + else + { + var model = meshRef.PartTransform * entityWorld; + ClassifyBatches(renderData, gfxObjId, model, entity, meshRef, + palHash, metaTable, restPose: meshRef.PartTransform, collector: collector); + drewAny = true; + } + + if (collector is not null && collector.Count > 0) + { + // Populate cache for static entity on cache-miss. + // Each entity classifies once at first visibility; subsequent frames + // will hit the fast path (added in Task 10). + _cache.Populate(entity.Id, landblockId, collector.ToArray()); + } + + if (diag && drewAny) _entitiesDrawn++; +} +``` + +- [ ] **Step 3: Build + test.** + +```powershell +dotnet build +dotnet test --no-build +``` + +Expected: `Failed: 8, Passed: 1699`. The slow path now also populates the cache, but visual + per-frame behavior is unchanged (we don't read from the cache yet). + +- [ ] **Step 4: Commit.** + +```bash +git add src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs +git commit -m "feat(render #53): cache-miss populate on first frame for static entities + +Restructures Draw's per-entity loop: animated entities still skip the +cache entirely, but static entities now collect their classification into +_populateScratch and call cache.Populate at the end of the iteration. + +Cache fast-path (skip slow classification on cache hit) lands in Task 10. +This intermediate state is verifiable: behavior unchanged, but the cache +is being populated as entities render. Diagnostic-friendly split. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Task 10: Wire dispatcher cache-hit fast path + integration tests #11, #12 + +**Files:** +- Modify: `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs` +- Modify: `tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs` + +- [ ] **Step 1: Add the cache-hit branch.** + +In `WbDrawDispatcher.Draw`, just after the `bool isAnimated = ...` line and BEFORE the `palHash` computation, add: + +```csharp + // Fast path: cache hit on a static entity. Skip classification entirely + // and append cached (RestPose * entityWorld) matrices to the matching + // groups. The DEBUG cross-check (added in Task 13) asserts the + // membership predicate held at hit time. + if (!isAnimated && _cache.TryGet(entity.Id, out var cachedEntry)) + { + foreach (var cached in cachedEntry!.Batches) + { + if (!_groups.TryGetValue(cached.Key, out var grp)) + { + grp = new InstanceGroup + { + Ibo = cached.Key.Ibo, + FirstIndex = cached.Key.FirstIndex, + BaseVertex = cached.Key.BaseVertex, + IndexCount = cached.Key.IndexCount, + BindlessTextureHandle = cached.Key.BindlessTextureHandle, + TextureLayer = cached.Key.TextureLayer, + Translucency = cached.Key.Translucency, + }; + _groups[cached.Key] = grp; + } + grp.Matrices.Add(cached.RestPose * entityWorld); + } + + if (anyVao == 0) + { + // Need a VAO for the GL phase. Look up the first MeshRef's + // mesh data once (cheap dict lookup, not a re-classify). + var firstMeshRef = entity.MeshRefs[partIdx]; + var firstRenderData = _meshAdapter.TryGetRenderData(firstMeshRef.GfxObjId); + if (firstRenderData is not null) anyVao = firstRenderData.VAO; + } + + if (diag) { _entitiesDrawn++; } + continue; + } +``` + +(Note: `_entitiesSeen++` already fired at the top of the loop body; only `_entitiesDrawn++` here.) + +- [ ] **Step 2: Write integration test #11 — static entity routes through cache.** + +In `tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs`, add a test that: + +```csharp + [Fact] + public void Draw_StaticEntity_PopulatesCacheOnFirstFrameAndHitsOnSecond() + { + var cache = new EntityClassificationCache(); + // Use the existing test fixture builder (whatever shape WbDrawDispatcherBucketingTests + // already uses). Pass `cache` as the new ctor argument. + // Construct one synthetic static WorldEntity in landblockEntries. + cache.Count.Should().Be(0); + + // … existing fixture: construct dispatcher + adapter + entity … + // … invoke Draw once … + + // First frame: cache populates. + cache.Count.Should().BeGreaterThan(0); + int firstCount = cache.Count; + + // … invoke Draw again with the same entity … + + // Second frame: cache hit — no double-populate. cache.Count is stable. + cache.Count.Should().Be(firstCount); + } +``` + +If the existing test fixture doesn't expose a spy / counter on `WbMeshAdapter`, this test asserts indirectly: after first Draw, `cache.Count == 1`; after second Draw, `cache.Count == 1` still (no double-populate, which would re-overwrite — `Populate` overwrite still leaves Count==1, so this test asserts that the populate path is reached on the first Draw and is NOT reached on the second Draw via a stronger spy if the fixture supports one; otherwise the weaker count-stability assert is acceptable). + +If a spy is feasible, prefer it. Pseudocode: + +```csharp +var spyAdapter = new SpyMeshAdapter(realAdapter); +// ... construct dispatcher with spyAdapter ... +spyAdapter.TryGetRenderDataCallCount.Should().Be(N_first_frame_lookups); +// ... invoke second Draw ... +spyAdapter.TryGetRenderDataCallCount.Should().Be(N_first_frame_lookups + 1); +// ↑ +1 for the single VAO lookup in the cache-hit branch, NOT +N for re-classification. +``` + +Choose whichever the existing fixture supports. + +- [ ] **Step 3: Write integration test #12 — animated entity bypasses cache.** + +```csharp + [Fact] + public void Draw_AnimatedEntity_DoesNotPopulateCache() + { + var cache = new EntityClassificationCache(); + // Construct dispatcher + adapter + one WorldEntity flagged in + // animatedEntityIds. Invoke Draw. + var animatedIds = new HashSet { /* entity.Id */ }; + // … invoke Draw with animatedEntityIds: animatedIds … + + // Cache should never be populated for animated entities. + cache.Count.Should().Be(0); + } +``` + +- [ ] **Step 4: Run integration tests.** + +```powershell +dotnet build +dotnet test --no-build --filter "FullyQualifiedName~WbDrawDispatcherBucketingTests" +``` + +Expected: existing dispatcher tests + 2 new cache integration tests all pass. + +- [ ] **Step 5: Run full suite.** + +```powershell +dotnet test --no-build +``` + +Expected: `Failed: 8, Passed: 1701` (1688 baseline + 11 cache tests + 2 integration tests = 1701). + +- [ ] **Step 6: Commit.** + +```bash +git add src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs +git commit -m "feat(render #53): cache-hit fast path + dispatcher integration tests + +WbDrawDispatcher.Draw now branches on cache hit before running classification: +on hit, walks the cached flat batch list and appends RestPose × entityWorld +to the matching groups; on miss, runs today's classification and populates +the cache. Animated entities skip the cache entirely. + +Adds dispatcher integration tests #11 (static entity populates + reuses) +and #12 (animated bypasses) per spec test plan §7.2. + +Phase 2 (dispatcher integration) complete. End-to-end caching now live. +Invalidation hooks (Phase 3) ensure correctness across despawns + LB demotes. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Phase 2 checkpoint + +- [ ] **Run sentinel + full suite.** + +```powershell +dotnet test --no-build --filter "FullyQualifiedName~TerrainSlot|FullyQualifiedName~TerrainModernConformance|FullyQualifiedName~Wb|FullyQualifiedName~MatrixComposition|FullyQualifiedName~TextureCacheBindless|FullyQualifiedName~SplitFormulaDivergence" +``` + +Expected: ≥ 107 passing (94 sentinel + 11 cache + 2 integration). 0 failures. + +```powershell +dotnet test --no-build +``` + +Expected: 1701 passed, 8 failed (pre-existing). + +--- + +## Phase 3: Invalidation hooks (Tasks 11-12) + +### Task 11: Wire `InvalidateEntity` from `RemoveLiveEntityByServerGuid` + test #15 + +**Files:** +- Modify: `src/AcDream.App/Rendering/GameWindow.cs` (line ~2935) +- Modify: `tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs` + +- [ ] **Step 1: Write test #15 (despawn-respawn cycle).** + +Append to `EntityClassificationCacheTests.cs` (just before `MakeCachedBatch`): + +```csharp + [Fact] + public void DespawnRespawn_UnderReusedId_RepopulatesFresh() + { + // Pins the audit's ObjDescEvent contract (audit §1): + // ObjDescEvent is despawn + respawn (with a NEW local entity.Id), + // never an in-place mutation. Even when an id IS reused + // (theoretical — _liveEntityIdCounter is monotonic in practice), + // the cache must serve fresh data after invalidation. + var cache = new EntityClassificationCache(); + var batchesV1 = new[] { MakeCachedBatch(1, 0, 6, 0xAA) }; + var batchesV2 = new[] { MakeCachedBatch(2, 6, 12, 0xCC) }; + + cache.Populate(100, 0xA9B40000u, batchesV1); + cache.InvalidateEntity(100); + cache.Populate(100, 0xA9B40000u, batchesV2); + + cache.TryGet(100, out var entry).Should().BeTrue(); + entry!.Batches.Should().Equal(batchesV2); + entry.Batches[0].BindlessTextureHandle.Should().Be(0xCCu); + } +``` + +- [ ] **Step 2: Run the test, verify it passes (it tests existing API).** + +```powershell +dotnet test --no-build --filter "FullyQualifiedName~DespawnRespawn" +``` + +Expected: pass. (This test pins behavior the cache class already provides; no implementation change needed for the test itself.) + +- [ ] **Step 3: Wire `InvalidateEntity` in `GameWindow.RemoveLiveEntityByServerGuid`.** + +In `src/AcDream.App/Rendering/GameWindow.cs`, find line ~2935: + +```csharp +_animatedEntities.Remove(existingEntity.Id); +``` + +Add immediately after: + +```csharp +_classificationCache.InvalidateEntity(existingEntity.Id); +``` + +- [ ] **Step 4: Build + run full suite.** + +```powershell +dotnet build +dotnet test --no-build +``` + +Expected: `Failed: 8, Passed: 1702`. + +- [ ] **Step 5: Commit.** + +```bash +git add src/AcDream.App/Rendering/GameWindow.cs tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs +git commit -m "feat(render #53): wire EntityClassificationCache.InvalidateEntity at despawn + +GameWindow.RemoveLiveEntityByServerGuid now invalidates the entity's +cache entry next to the existing _animatedEntities.Remove(). Fires for +DeleteObject (0xF747) and the dedup leg of ObjDescEvent (0xF625). + +Adds test #15 (despawn-respawn under reused id repopulates fresh) per +spec §7.5 — pins the audit's ObjDescEvent-as-despawn-respawn contract. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Task 12: Wire `InvalidateLandblock` callback into `GpuWorldState.RemoveEntitiesFromLandblock` + +**Files:** +- Modify: `src/AcDream.App/Streaming/GpuWorldState.cs` +- Modify: `src/AcDream.App/Rendering/GameWindow.cs` (the `GpuWorldState` instantiation site) + +Per spec §5.3 W3b: pass an `Action?` callback into `GpuWorldState`'s ctor so when `RemoveEntitiesFromLandblock` clears a landblock's entity list, the callback fires once per landblock id. + +- [ ] **Step 1: Add the callback parameter to `GpuWorldState`.** + +In `src/AcDream.App/Streaming/GpuWorldState.cs`, find the ctor (or primary ctor declaration). Add a new optional parameter `Action? onLandblockUnloaded = null`. Store as a field. + +```csharp +private readonly Action? _onLandblockUnloaded; + +// in ctor: +_onLandblockUnloaded = onLandblockUnloaded; +``` + +Modify `RemoveEntitiesFromLandblock` (line 373) to invoke the callback BEFORE zeroing the entity list: + +```csharp +public void RemoveEntitiesFromLandblock(uint landblockId) +{ + uint canonical = (landblockId & 0xFFFF0000u) | 0xFFFFu; + if (!_loaded.TryGetValue(canonical, out var lb)) return; + if (_wbSpawnAdapter is not null) + _wbSpawnAdapter.OnLandblockUnloaded(canonical); + + // Phase Post-A.5 #53: invalidate the EntityClassificationCache for this + // landblock before we drop the entity list. Wired via callback at + // GameWindow construction; null when the cache isn't relevant (tests). + _onLandblockUnloaded?.Invoke(canonical); + + _loaded[canonical] = new LoadedLandblock(lb.LandblockId, lb.Heightmap, System.Array.Empty()); + _pendingByLandblock.Remove(canonical); + RebuildFlatView(); +} +``` + +- [ ] **Step 2: Wire the callback at `GameWindow`.** + +Find the `new GpuWorldState(...)` invocation: + +```powershell +Select-String -Path src/AcDream.App/Rendering/GameWindow.cs -Pattern "new GpuWorldState" +``` + +Add the new argument: + +```csharp +_worldState = new GpuWorldState( + /* … existing args … */, + onLandblockUnloaded: _classificationCache.InvalidateLandblock); +``` + +- [ ] **Step 3: Update existing `GpuWorldState` test fixtures.** + +```powershell +Select-String -Path tests/**/*.cs -Pattern "new GpuWorldState" +``` + +For each hit, the existing tests can omit the new optional parameter (it defaults to null). No change required unless a specific test wants to assert the callback fires. + +- [ ] **Step 4: Build + run full suite.** + +```powershell +dotnet build +dotnet test --no-build +``` + +Expected: `Failed: 8, Passed: 1702` (no new tests in this task — invalidation behavior is exercised indirectly through visual + perf gates, plus the optional unit test in Step 5). + +- [ ] **Step 5: (Optional) Add a streaming integration test.** + +If `GpuWorldStateTwoTierTests.cs` makes it easy, add: + +```csharp + [Fact] + public void RemoveEntitiesFromLandblock_FiresUnloadCallbackBeforeClearingEntities() + { + uint? observed = null; + var state = new GpuWorldState( + /* … existing args … */, + onLandblockUnloaded: id => observed = id); + + // Set up: add a synthetic entity to LB 0xA9B40000 via AppendLiveEntity. + // ... + state.RemoveEntitiesFromLandblock(0xA9B40000u); + + observed.Should().Be(0xA9B4FFFFu); // canonicalized + } +``` + +If the fixture is heavy, defer. + +- [ ] **Step 6: Commit.** + +```bash +git add src/AcDream.App/Streaming/GpuWorldState.cs src/AcDream.App/Rendering/GameWindow.cs tests/AcDream.Core.Tests/Streaming/*.cs +git commit -m "feat(render #53): wire EntityClassificationCache.InvalidateLandblock at LB demote/unload + +GpuWorldState.RemoveEntitiesFromLandblock now invokes an optional +Action callback before zeroing the entity list. GameWindow wires +this to EntityClassificationCache.InvalidateLandblock so cache entries +get swept on LB demote (Near→Far) and unload. Per spec §5.3 W3b. + +Phase 3 (invalidation hooks) complete. The cache now stays correct across +all spec-identified mutation events: despawn, ObjDescEvent (despawn+ +respawn), LB demote, LB unload. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Phase 3 checkpoint + +- [ ] **Run sentinel + full suite.** + +```powershell +dotnet test --no-build --filter "FullyQualifiedName~TerrainSlot|FullyQualifiedName~TerrainModernConformance|FullyQualifiedName~Wb|FullyQualifiedName~MatrixComposition|FullyQualifiedName~TextureCacheBindless|FullyQualifiedName~SplitFormulaDivergence" +``` + +Expected: 0 failures. + +```powershell +dotnet test --no-build +``` + +Expected: `Failed: 8, Passed: 1702`. + +--- + +## Phase 4: DEBUG cross-check (Task 13) + +### Task 13: Add DEBUG cross-check + test #13 + +**Files:** +- Modify: `src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs` +- Modify: `src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs` (call cross-check on cache hit, DEBUG-only) +- Modify: `tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs` + +- [ ] **Step 1: Add the cross-check method to the cache.** + +Append to `EntityClassificationCache.cs`: + +```csharp +#if DEBUG + /// + /// Asserts that the cached entry for still + /// matches what fresh classification would produce. Catches the prior + /// Tier 1 bug class — silent caching of mutable per-frame state — by + /// firing when any cached + /// field has drifted from live state. + /// + /// + /// Caller passes per-batch live state (Key, BindlessTextureHandle, RestPose) + /// reconstructed from the same path the populate ran. The cache iterates + /// its stored entries in parallel and asserts equality. + /// + /// + /// + /// Zero cost in Release. In DEBUG, called once per static-entity cache + /// hit per frame — adds modest overhead. Acceptable for dev runs. + /// + /// + public void DebugCrossCheck(uint entityId, IReadOnlyList liveBatches) + { + if (!_entries.TryGetValue(entityId, out var entry)) return; + + System.Diagnostics.Debug.Assert( + entry.Batches.Length == liveBatches.Count, + $"EntityClassificationCache: batch count mismatch for entity {entityId}: cached={entry.Batches.Length} live={liveBatches.Count}"); + + for (int i = 0; i < entry.Batches.Length && i < liveBatches.Count; i++) + { + var cached = entry.Batches[i]; + var live = liveBatches[i]; + System.Diagnostics.Debug.Assert( + cached.Key.Equals(live.Key), + $"EntityClassificationCache: GroupKey drift for entity {entityId} batch {i}"); + System.Diagnostics.Debug.Assert( + cached.BindlessTextureHandle == live.BindlessTextureHandle, + $"EntityClassificationCache: texture handle drift for entity {entityId} batch {i}"); + System.Diagnostics.Debug.Assert( + MatrixApproxEqual(cached.RestPose, live.RestPose, epsilon: 1e-5f), + $"EntityClassificationCache: RestPose drift for entity {entityId} batch {i}"); + } + } + + private static bool MatrixApproxEqual(System.Numerics.Matrix4x4 a, System.Numerics.Matrix4x4 b, float epsilon) + { + return System.MathF.Abs(a.M11 - b.M11) <= epsilon && System.MathF.Abs(a.M12 - b.M12) <= epsilon && + System.MathF.Abs(a.M13 - b.M13) <= epsilon && System.MathF.Abs(a.M14 - b.M14) <= epsilon && + System.MathF.Abs(a.M21 - b.M21) <= epsilon && System.MathF.Abs(a.M22 - b.M22) <= epsilon && + System.MathF.Abs(a.M23 - b.M23) <= epsilon && System.MathF.Abs(a.M24 - b.M24) <= epsilon && + System.MathF.Abs(a.M31 - b.M31) <= epsilon && System.MathF.Abs(a.M32 - b.M32) <= epsilon && + System.MathF.Abs(a.M33 - b.M33) <= epsilon && System.MathF.Abs(a.M34 - b.M34) <= epsilon && + System.MathF.Abs(a.M41 - b.M41) <= epsilon && System.MathF.Abs(a.M42 - b.M42) <= epsilon && + System.MathF.Abs(a.M43 - b.M43) <= epsilon && System.MathF.Abs(a.M44 - b.M44) <= epsilon; + } +#endif +``` + +- [ ] **Step 2: Wire the cross-check into the dispatcher's cache-hit path.** + +In `WbDrawDispatcher.Draw`, inside the cache-hit branch from Task 10, AFTER appending matrices, add (DEBUG-only): + +```csharp +#if DEBUG + // Cross-check guard: assert the membership predicate held at hit time. + // The full re-classification cross-check (spec §6.5) is a stretch goal; + // this simpler assert catches the prior Tier 1 bug class — a static + // entity that turns out to actually be animated would fire here. + System.Diagnostics.Debug.Assert( + !isAnimated, + $"EntityClassificationCache hit on animated entity {entity.Id} — invariant violated"); +#endif +``` + +(The full live-state cross-check requires re-running ClassifyBatches with a live collector, which is non-trivial to plumb into the per-entity branch; ship the predicate assert and file a follow-up issue if the team wants the full cross-check later. The unit test in Step 3 still covers `DebugCrossCheck` directly.) + +- [ ] **Step 3: Write test #13 — DEBUG cross-check fires on synthetic mismatch.** + +Append to `EntityClassificationCacheTests.cs`: + +```csharp +#if DEBUG + [Fact] + public void DebugCrossCheck_BatchCountMismatch_FiresAssert() + { + var cache = new EntityClassificationCache(); + cache.Populate(100, 0u, new[] + { + MakeCachedBatch(1, 0, 6, 0xAA), + MakeCachedBatch(1, 6, 6, 0xBB), + }); + + // Synthetic "live" with fewer batches → should fire Debug.Assert. + var liveBatches = new[] { MakeCachedBatch(1, 0, 6, 0xAA) }; + + // Capture Debug.Assert via a custom TraceListener. + var originalListeners = new System.Diagnostics.TraceListener[System.Diagnostics.Trace.Listeners.Count]; + System.Diagnostics.Trace.Listeners.CopyTo(originalListeners, 0); + System.Diagnostics.Trace.Listeners.Clear(); + var asserts = new List(); + System.Diagnostics.Trace.Listeners.Add(new CaptureListener(asserts)); + + try + { + cache.DebugCrossCheck(100, liveBatches); + } + finally + { + System.Diagnostics.Trace.Listeners.Clear(); + foreach (var l in originalListeners) System.Diagnostics.Trace.Listeners.Add(l); + } + + asserts.Should().NotBeEmpty(); + string joined = string.Join(" ", asserts); + joined.Should().Contain("batch count mismatch"); + } + + [Fact] + public void DebugCrossCheck_RestPoseMatch_NoAssert() + { + var cache = new EntityClassificationCache(); + var batches = new[] { MakeCachedBatch(1, 0, 6, 0xAA) }; + cache.Populate(100, 0u, batches); + + var originalListeners = new System.Diagnostics.TraceListener[System.Diagnostics.Trace.Listeners.Count]; + System.Diagnostics.Trace.Listeners.CopyTo(originalListeners, 0); + System.Diagnostics.Trace.Listeners.Clear(); + var asserts = new List(); + System.Diagnostics.Trace.Listeners.Add(new CaptureListener(asserts)); + + try + { + cache.DebugCrossCheck(100, batches); + } + finally + { + System.Diagnostics.Trace.Listeners.Clear(); + foreach (var l in originalListeners) System.Diagnostics.Trace.Listeners.Add(l); + } + + asserts.Should().BeEmpty(); + } + + private sealed class CaptureListener : System.Diagnostics.TraceListener + { + private readonly List _captured; + public CaptureListener(List captured) { _captured = captured; } + public override void Write(string? message) { if (message != null) _captured.Add(message); } + public override void WriteLine(string? message) { if (message != null) _captured.Add(message); } + public override void Fail(string? message, string? detailMessage) + { + _captured.Add($"{message}: {detailMessage}"); + } + public override void Fail(string? message) { if (message != null) _captured.Add(message); } + } +#endif +``` + +- [ ] **Step 4: Build + run full suite.** + +```powershell +dotnet build +dotnet test --no-build +``` + +Expected: `Failed: 8, Passed: 1704` (two new DEBUG-only tests; in DEBUG configuration both run). + +- [ ] **Step 5: Commit.** + +```bash +git add src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs +git commit -m "feat(render #53): DEBUG cross-check guards against the prior Tier 1 bug class + +Adds EntityClassificationCache.DebugCrossCheck(entityId, liveBatches) that +asserts cached state matches a live re-classification. Wires a simpler +predicate assert into WbDrawDispatcher's cache-hit branch (asserts +isAnimated == false on cache hit). Tests #13a and #13b cover the +batch-count mismatch and clean-match cases via a custom TraceListener +that captures Debug.Assert calls. + +Zero cost in Release. In DEBUG, the assert fires immediately if a future +regression mutates static-entity state outside the audit's known write +sites — the same failure mode that bit the prior Tier 1 attempt. + +Phase 4 complete. Cache + invalidation + safety net all in place. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +--- + +### Phase 4 checkpoint + +- [ ] **Run sentinel + full suite.** + +```powershell +dotnet test --no-build --filter "FullyQualifiedName~TerrainSlot|FullyQualifiedName~TerrainModernConformance|FullyQualifiedName~Wb|FullyQualifiedName~MatrixComposition|FullyQualifiedName~TextureCacheBindless|FullyQualifiedName~SplitFormulaDivergence" +``` + +Expected: 0 failures. + +```powershell +dotnet test --no-build +``` + +Expected: `Failed: 8, Passed: 1704` in DEBUG (or 1702 in Release where the `#if DEBUG` tests are excluded). + +--- + +## Phase 5: Verification gates (Tasks 14-16) + +### Task 14: Pre-launch sanity — full suite + sentinel + grep for TODO/FIXME + +- [ ] **Step 1: Final build green check.** + +```powershell +dotnet build +``` + +Expected: `Build succeeded. 0 Error(s)`. + +- [ ] **Step 2: Full test pass.** + +```powershell +dotnet test --no-build +``` + +Expected: 1704 (or 1702 in Release) passing, 8 pre-existing physics/input failures unchanged. + +- [ ] **Step 3: Sentinel filter pass.** + +```powershell +dotnet test --no-build --filter "FullyQualifiedName~TerrainSlot|FullyQualifiedName~TerrainModernConformance|FullyQualifiedName~Wb|FullyQualifiedName~MatrixComposition|FullyQualifiedName~TextureCacheBindless|FullyQualifiedName~SplitFormulaDivergence" +``` + +Expected: 0 failures. + +- [ ] **Step 4: Grep for any leftover TODO/FIXME the implementation introduced.** + +```powershell +Select-String -Path src/AcDream.App/Rendering/Wb/*.cs -Pattern "TODO|FIXME|XXX" +``` + +Expected: any hits should be intentional (e.g. cross-check stretch-goal note); fix or document if not. + +--- + +### Task 15: Visual gate (USER REQUIRED) + +This step requires the user to launch the live client and visually verify the change. + +- [ ] **Step 1: Confirm baseline behavior (before any visual claims).** + +The user reports build green + tests passing. Implementation agent confirms the spec's acceptance criteria items 1-7 are checked. + +- [ ] **Step 2: Launch the client.** + +```powershell +$env:ACDREAM_DAT_DIR = "$env:USERPROFILE\Documents\Asheron's Call" +$env:ACDREAM_LIVE = "1" +$env:ACDREAM_TEST_HOST = "127.0.0.1" +$env:ACDREAM_TEST_PORT = "9000" +$env:ACDREAM_TEST_USER = "testaccount" +$env:ACDREAM_TEST_PASS = "testpassword" +dotnet run --project src/AcDream.App/AcDream.App.csproj --no-build -c Debug 2>&1 | Tee-Object -FilePath launch-tier1-visual.log +``` + +- [ ] **Step 3: User walks Holtburg → North Yanshi at horizon-safe preset.** + +Confirm visually: +- A nearby NPC (any creature) animates normally — limbs move, idle breathing visible. +- The Holtburg lifestone crystal (Z=94 platform) renders correctly and animates (rotation / glow). +- Static buildings render at correct positions (no offsets, no missing parts). +- No new visual artifacts. + +If any of the above fail, **STOP**: file a sub-issue, diagnose, and either fix or revert before continuing. + +- [ ] **Step 4: User reports visual gate result.** + +Implementation agent records the user's confirmation. + +--- + +### Task 16: Perf gate (USER REQUIRED) + +- [ ] **Step 1: Launch with `[WB-DIAG]` enabled in Release config.** + +```powershell +$env:ACDREAM_WB_DIAG = "1" +$env:ACDREAM_DAT_DIR = "$env:USERPROFILE\Documents\Asheron's Call" +$env:ACDREAM_LIVE = "1" +$env:ACDREAM_TEST_HOST = "127.0.0.1" +$env:ACDREAM_TEST_PORT = "9000" +$env:ACDREAM_TEST_USER = "testaccount" +$env:ACDREAM_TEST_PASS = "testpassword" +dotnet run --project src/AcDream.App/AcDream.App.csproj -c Release 2>&1 | Tee-Object -FilePath perf-tier1-after.log +``` + +(Release build — perf measurements should match what users see, not DEBUG with cross-check overhead.) + +- [ ] **Step 2: User stands at Holtburg center for ≥ 30 seconds at horizon-safe preset.** + +Defaults: NEAR_RADIUS=4, FAR_RADIUS=12, MSAA=0, A2C=0, ANISOTROPIC=4, MAX_COMPLETIONS=2. + +- [ ] **Step 3: Capture `[WB-DIAG]` output from the log.** + +```powershell +Select-String -Path perf-tier1-after.log -Pattern "\[WB-DIAG\]" | Select-Object -Last 5 +``` + +Expected output format (from existing dispatcher): +``` +[WB-DIAG] entSeen=… entDrawn=… meshMissing=0 drawsIssued=… instances=… groups=… cpu_us=m/p95 gpu_us=…m/…p95 +``` + +- [ ] **Step 4: Verify perf gate.** + +Check `cpu_us=m/p95`: +- `MEDIAN ≤ 2000` (≤ 2.0 ms — spec budget). +- `P95 ≤ 2500` (≤ 2.5 ms). +- No `BUDGET_OVER` flag. + +Compare against the pre-Tier-1 baseline (~3500 / ~4000 from the post-A.5 state). Expected: ~50% reduction in median. + +- [ ] **Step 5: Record results.** + +If perf gate passes, proceed to Phase 6 (ship). Document the actual numbers in the closing commit message. + +If perf gate FAILS (median > 2.0 ms), this is a signal that: +- Cache hit rate is lower than expected (animated entities dominate visible set). +- OR per-frame matrix mults still dominate (consider Q3 option M revisit). +- OR a cache invalidation is firing too aggressively (visible thrashing). + +Diagnose with `cache.Count` over time + the existing `entSeen` / `entDrawn` counters. Do NOT ship without hitting the gate; either fix or escalate per spec §11. + +--- + +## Phase 6: Ship (Task 17) + +### Task 17: Update ISSUES, CLAUDE.md, memory; final commit; merge + +**Files:** +- Modify: `docs/ISSUES.md` +- Modify: `CLAUDE.md` +- Modify: `~/.claude/projects/.../memory/project_phase_a5_state.md` (or new memory entry if a new gotcha surfaced) + +- [ ] **Step 1: Move issue #53 to "Recently closed" in `docs/ISSUES.md`.** + +Find the `## #53` block under "Active issues". Move it to "Recently closed" with the closing commit SHA. Add a one-line resolution summary citing the audit + spec + perf result. + +- [ ] **Step 2: Update `CLAUDE.md` "Currently in flight".** + +Find the line: + +``` +**Currently in flight: Post-A.5 polish — Tier 1 retry (only remaining priority).** +``` + +Replace with the post-A.5-complete state. Update the recently-shipped narrative to mention #53. + +- [ ] **Step 3: Update memory if new gotchas surfaced.** + +If implementation surfaced any gotchas (e.g. unexpected animated/static transitions, an LB invalidation edge case, etc.) that other agents would benefit from, add a memory entry under `~/.claude/projects/C--Users-erikn-source-repos-acdream/memory/` and add a one-line link in `MEMORY.md`. + +If no new gotchas surfaced, add a one-line note to `project_phase_a5_state.md` documenting the Tier 1 closure + final perf number. + +- [ ] **Step 4: Final commit.** + +```bash +git add docs/ISSUES.md CLAUDE.md +# also memory if updated +git commit -m "ship(post-A.5 #53): Tier 1 entity-classification cache — closes ISSUE #53 + +Static-only cache + DEBUG cross-check + invalidation hooks lands per spec +docs/superpowers/specs/2026-05-10-issue-53-tier1-cache-design.md. + +Perf gate: entity dispatcher cpu_us median m / p95 at +horizon-safe preset (radius=4/12) on AMD Radeon RX 9070 XT @ 1440p. +Spec target was ≤2000m/≤2500p95. Baseline was ~3500m/~4000p95. + +Visual gate: NPC animates, lifestone renders, buildings at correct +positions — confirmed by user 2026-05-10. + +Closes the post-A.5 polish phase. Issues #52, #54, #53 all closed. + +Co-Authored-By: Claude Opus 4.7 (1M context) " +``` + +- [ ] **Step 5: Merge to main.** + +The user merges the worktree branch via the same pattern as the prior session: + +```bash +# from main: +git checkout main +git merge claude/friendly-varahamihira-7b8664 --no-ff -m "Merge branch 'claude/friendly-varahamihira-7b8664' — Tier 1 entity-classification cache (closes #53)" +git push origin main +``` + +(Implementation agent does not push without explicit user authorization. The merge step is included for the user's reference.) + +--- + +## Self-Review checklist (run after writing the plan — completed inline) + +- [x] **Spec coverage.** Every section of the spec maps to a task: + - Spec §1 (problem) → motivation, no task. + - Spec §3 Q1 (DEBUG cross-check) → Task 13. + - Spec §3 Q2 (separate class) → Tasks 2-5. + - Spec §3 Q3 (rest pose) → Task 8 (RestPose param) + Task 10 (cache-hit fast path). + - Spec §3 Q4 (Setup pre-flatten) → Task 8 (passes the right product) + Task 3 test #14. + - Spec §3 Q5 (thorough tests) → Tasks 2-5, 10, 11, 13 (12 cache + 2 integration + 2 DEBUG). + - Spec §5.3 invalidation wiring → Task 11 (per-entity), Task 12 (per-LB W3b). + - Spec §6.1-6.5 component contracts → Tasks 2-13. + - Spec §7 test plan → Tasks 2, 3, 4, 5, 10, 11, 13. + - Spec §8 sequencing → matches Tasks 1-17. + - Spec §9 acceptance criteria → Tasks 14-17. + - Spec §11 open implementation choices: W3b chosen (Task 12); GroupKey internal-at-namespace (Task 1); ResolveLandblockHint via walk plumbing (Task 6 + 9); _populateScratch field (Task 9). + +- [x] **Placeholder scan.** Searched plan for "TBD", "TODO", "implement later", etc. No matches outside intentional context (e.g. the `` perf-number placeholders in the final commit message — to be filled by the implementer with measured values). + +- [x] **Type consistency.** `CachedBatch`, `EntityCacheEntry`, `EntityClassificationCache` names + signatures match across Tasks 2-13. `GroupKey` is `internal` at namespace scope from Task 1 onward. Tuple shape `(WorldEntity Entity, int MeshRefIndex, uint LandblockId)` consistent in Tasks 6, 9, 10. + +--- + +## Execution + +This plan is ready for execution. Two options: + +**1. Subagent-Driven (recommended)** — fresh subagent per task; main session reviews each task before dispatching the next; fast iteration. + +**2. Inline Execution** — execute tasks in this session via `superpowers:executing-plans`; batch execution with checkpoints between phases. + +Both options preserve the TDD discipline (test before implementation in every step). Visual + perf gates (Tasks 15-16) require the user's eyes regardless of execution model. 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). diff --git a/src/AcDream.App/Rendering/GameWindow.cs b/src/AcDream.App/Rendering/GameWindow.cs index 149084d..c3bba03 100644 --- a/src/AcDream.App/Rendering/GameWindow.cs +++ b/src/AcDream.App/Rendering/GameWindow.cs @@ -159,6 +159,17 @@ public sealed class GameWindow : IDisposable /// private readonly Dictionary _animatedEntities = new(); + /// + /// Tier 1 cache (#53): per-entity classification results for static + /// entities (those NOT in ). Conceptually + /// paired with — that dictionary is the + /// gating predicate, this cache is the lookup that depends on it. + /// Passed to at + /// construction time. Tasks 9-10 of the cache plan wire the per-entity + /// miss-populate / hit-fast-path through the dispatcher's loop. + /// + private readonly AcDream.App.Rendering.Wb.EntityClassificationCache _classificationCache = new(); + private sealed class AnimatedEntity { public required AcDream.Core.World.WorldEntity Entity; @@ -1601,10 +1612,18 @@ public sealed class GameWindow : IDisposable var wbEntitySpawnAdapter = new AcDream.App.Rendering.Wb.EntitySpawnAdapter( _textureCache!, SequencerFactory, _wbMeshAdapter!); _wbEntitySpawnAdapter = wbEntitySpawnAdapter; - _worldState = new AcDream.App.Streaming.GpuWorldState(wbSpawnAdapter, wbEntitySpawnAdapter); + // Phase Post-A.5 #53 (Task 12): wire EntityClassificationCache.InvalidateLandblock + // so Tier 1 cache entries get swept on LB demote (Near to Far) and unload. + // Per spec §5.3 W3b. The callback receives the canonical landblock id + // matching the LandblockHint stored at Populate time. + _worldState = new AcDream.App.Streaming.GpuWorldState( + wbSpawnAdapter, + wbEntitySpawnAdapter, + onLandblockUnloaded: _classificationCache.InvalidateLandblock); _wbDrawDispatcher = new AcDream.App.Rendering.Wb.WbDrawDispatcher( - _gl, _meshShader!, _textureCache!, _wbMeshAdapter!, _wbEntitySpawnAdapter, _bindlessSupport!); + _gl, _meshShader!, _textureCache!, _wbMeshAdapter!, _wbEntitySpawnAdapter, _bindlessSupport!, + _classificationCache); // A.5 T22.5: apply A2C gate from quality preset. _wbDrawDispatcher.AlphaToCoverage = _resolvedQuality.AlphaToCoverage; } @@ -2933,6 +2952,7 @@ public sealed class GameWindow : IDisposable _worldState.RemoveEntityByServerGuid(serverGuid); _worldGameState.RemoveById(existingEntity.Id); _animatedEntities.Remove(existingEntity.Id); + _classificationCache.InvalidateEntity(existingEntity.Id); _physicsEngine.ShadowObjects.Deregister(existingEntity.Id); // Dead-reckon state is keyed by SERVER guid (not local id) so we diff --git a/src/AcDream.App/Rendering/Wb/CachedBatch.cs b/src/AcDream.App/Rendering/Wb/CachedBatch.cs new file mode 100644 index 0000000..d1bccb7 --- /dev/null +++ b/src/AcDream.App/Rendering/Wb/CachedBatch.cs @@ -0,0 +1,39 @@ +using System.Numerics; + +namespace AcDream.App.Rendering.Wb; + +/// +/// Per-(entity, partIdx, batchIdx) classification result, stored flat inside +/// . For Setup multi-part MeshRefs each +/// subPart contributes its own entries, with +/// already containing the +/// subPart.PartTransform * meshRef.PartTransform product. +/// +/// Accessibility: internal because is +/// internal and shows up in this struct's constructor / Deconstruct +/// signature. The cache itself is dispatcher-internal coordination state; +/// on AcDream.App exposes the type to +/// AcDream.Core.Tests. +/// +internal readonly record struct CachedBatch( + GroupKey Key, + ulong BindlessTextureHandle, + Matrix4x4 RestPose); + +/// +/// One entity's cached classification. is flat across +/// (partIdx, batchIdx) and ordered as WbDrawDispatcher.ClassifyBatches +/// produced them. lets +/// sweep entries +/// efficiently when a landblock demotes or unloads. +/// +/// Accessibility: internal for the same reason as +/// — its property is CachedBatch[], which +/// transitively involves . +/// +internal sealed class EntityCacheEntry +{ + public required uint EntityId { get; init; } + public required uint LandblockHint { get; init; } + public required CachedBatch[] Batches { get; init; } +} diff --git a/src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs b/src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs new file mode 100644 index 0000000..b0e248e --- /dev/null +++ b/src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs @@ -0,0 +1,193 @@ +using System.Collections.Generic; + +namespace AcDream.App.Rendering.Wb; + +/// +/// Cache of per-entity classification results for static entities (those NOT +/// in GameWindow._animatedEntities). Holds one +/// per cached entity. The cache is opaque +/// w.r.t. classification logic — it simply stores what callers populate. +/// +/// +/// Key composition: entries are keyed by the tuple +/// (EntityId, LandblockHint), NOT by EntityId alone. Issue #53 +/// uncovered that entity.Id is NOT globally unique across all +/// static-entity hydration paths: scenery (0x80LLBB00 + localIndex) +/// and interior cells (0x40LLBB00 + localCounter) overflow at >256 +/// items per landblock, wrapping into the lbY byte and producing +/// cross-LB collisions in dense forest/urban LBs outside Holtburg. Keying +/// by the tuple is correct-by-construction regardless of any hydration +/// path's id strategy. +/// +/// +/// +/// Invariants: +/// +/// overwrites any existing entry for the same (id, lb) tuple (defensive). +/// sweeps all entries with the given EntityId +/// regardless of LandblockHint; idempotent (no-throw on missing id). +/// walks all entries; entries whose +/// equals the argument are removed. +/// All operations are render-thread only. No internal locking. +/// +/// +/// +/// +/// Audit foundation: see +/// docs/research/2026-05-10-tier1-mutation-audit.md for why static +/// entities can be cached and what invalidation is needed. +/// +/// +/// +/// Accessibility: internal. and +/// both transitively reference the internal +/// ; surfacing the cache as public would create +/// inconsistent-accessibility errors. Cross-assembly access for the test +/// project comes via InternalsVisibleTo("AcDream.Core.Tests") on +/// AcDream.App.csproj. +/// +/// +internal sealed class EntityClassificationCache +{ + private readonly Dictionary<(uint EntityId, uint LandblockHint), EntityCacheEntry> _entries = new(); + + /// Number of cached entities — for diagnostics. + public int Count => _entries.Count; + + /// + /// Look up an entity's cached classification. Keyed by both + /// AND to + /// disambiguate entities whose Ids collide across landblocks (e.g., + /// scenery's 0x80LLBB00 + localIndex overflow at >256 items/LB). + /// Returns true with the entry on hit; false with + /// set to null on miss. + /// + public bool TryGet(uint entityId, uint landblockHint, out EntityCacheEntry? entry) + => _entries.TryGetValue((entityId, landblockHint), out entry); + + /// + /// Insert or overwrite a cache entry for the + /// (, ) + /// tuple. Defensive: if an entry already exists, replaces it. + /// + public void Populate(uint entityId, uint landblockHint, CachedBatch[] batches) + { + _entries[(entityId, landblockHint)] = new EntityCacheEntry + { + EntityId = entityId, + LandblockHint = landblockHint, + Batches = batches, + }; + } + + /// + /// Remove all cache entries for the given , + /// regardless of which landblock they were populated under. Sweep is + /// needed because we may have entries for the same Id under different + /// LandblockHints if any hydration path produced colliding Ids + /// historically (defensive even though current paths shouldn't produce + /// duplicates per-LB). Was O(1) before the #53 tuple-key change; + /// now O(n), but called rarely (only on entity despawn). + /// + public void InvalidateEntity(uint entityId) + { + if (_entries.Count == 0) return; + List<(uint, uint)>? toRemove = null; + foreach (var key in _entries.Keys) + { + if (key.EntityId == entityId) + { + toRemove ??= new List<(uint, uint)>(); + toRemove.Add(key); + } + } + if (toRemove is null) return; + foreach (var k in toRemove) _entries.Remove(k); + } + + /// + /// Remove every cache entry whose + /// equals . Used by the streaming pipeline + /// when a landblock demotes from near to far or unloads. No-op if no + /// entries match. + /// + public void InvalidateLandblock(uint landblockId) + { + if (_entries.Count == 0) return; + + // Collect the keys to remove first to avoid mutating the dict during iteration. + // Buffered locally because the typical case removes ~all entries in the LB + // (which is still small relative to the total cache). + List<(uint, uint)>? toRemove = null; + foreach (var key in _entries.Keys) + { + if (key.LandblockHint == landblockId) + { + toRemove ??= new List<(uint, uint)>(); + toRemove.Add(key); + } + } + if (toRemove is null) return; + foreach (var k in toRemove) _entries.Remove(k); + } + +#if DEBUG + /// + /// Asserts that the cached entry for still + /// matches what fresh classification would produce. Catches the prior + /// Tier 1 bug class — silent caching of mutable per-frame state — by + /// firing when any cached + /// field has drifted from live state. + /// + /// + /// Caller passes per-batch live state (Key, BindlessTextureHandle, RestPose) + /// reconstructed from the same path the populate ran. The cache iterates + /// its stored entries in parallel and asserts equality. + /// + /// + /// + /// As of Phase 4 (commit f16604b) this method is exercised by unit tests + /// only; the dispatcher's cache-hit branch fires a simpler predicate assert + /// (!isAnimated) at production hit time. Wiring the full live-state + /// cross-check into the per-entity branch is the spec section 6.5 stretch + /// goal and remains open as a follow-up. Zero cost in Release; the method + /// stays here so the regression-class guard is locked behind tests. + /// + /// + public void DebugCrossCheck(uint entityId, uint landblockHint, IReadOnlyList liveBatches) + { + if (!_entries.TryGetValue((entityId, landblockHint), out var entry)) return; + + System.Diagnostics.Debug.Assert( + entry.Batches.Length == liveBatches.Count, + $"EntityClassificationCache: batch count mismatch for entity {entityId}: cached={entry.Batches.Length} live={liveBatches.Count}"); + + for (int i = 0; i < entry.Batches.Length && i < liveBatches.Count; i++) + { + var cached = entry.Batches[i]; + var live = liveBatches[i]; + System.Diagnostics.Debug.Assert( + cached.Key.Equals(live.Key), + $"EntityClassificationCache: GroupKey drift for entity {entityId} batch {i}"); + System.Diagnostics.Debug.Assert( + cached.BindlessTextureHandle == live.BindlessTextureHandle, + $"EntityClassificationCache: texture handle drift for entity {entityId} batch {i}"); + System.Diagnostics.Debug.Assert( + MatrixApproxEqual(cached.RestPose, live.RestPose, epsilon: 1e-5f), + $"EntityClassificationCache: RestPose drift for entity {entityId} batch {i}"); + } + } + + private static bool MatrixApproxEqual(System.Numerics.Matrix4x4 a, System.Numerics.Matrix4x4 b, float epsilon) + { + return System.MathF.Abs(a.M11 - b.M11) <= epsilon && System.MathF.Abs(a.M12 - b.M12) <= epsilon && + System.MathF.Abs(a.M13 - b.M13) <= epsilon && System.MathF.Abs(a.M14 - b.M14) <= epsilon && + System.MathF.Abs(a.M21 - b.M21) <= epsilon && System.MathF.Abs(a.M22 - b.M22) <= epsilon && + System.MathF.Abs(a.M23 - b.M23) <= epsilon && System.MathF.Abs(a.M24 - b.M24) <= epsilon && + System.MathF.Abs(a.M31 - b.M31) <= epsilon && System.MathF.Abs(a.M32 - b.M32) <= epsilon && + System.MathF.Abs(a.M33 - b.M33) <= epsilon && System.MathF.Abs(a.M34 - b.M34) <= epsilon && + System.MathF.Abs(a.M41 - b.M41) <= epsilon && System.MathF.Abs(a.M42 - b.M42) <= epsilon && + System.MathF.Abs(a.M43 - b.M43) <= epsilon && System.MathF.Abs(a.M44 - b.M44) <= epsilon; + } +#endif +} diff --git a/src/AcDream.App/Rendering/Wb/GroupKey.cs b/src/AcDream.App/Rendering/Wb/GroupKey.cs new file mode 100644 index 0000000..696363c --- /dev/null +++ b/src/AcDream.App/Rendering/Wb/GroupKey.cs @@ -0,0 +1,20 @@ +using AcDream.Core.Meshing; + +namespace AcDream.App.Rendering.Wb; + +/// +/// Bucket identity for 's per-frame group dictionary. +/// Two (entity, batch) pairs that share the same render +/// in a single glMultiDrawElementsIndirect draw command. Promoted to +/// internal at file scope (was a private nested type) so +/// can store it inside +/// without depending on dispatcher internals. +/// +internal readonly record struct GroupKey( + uint Ibo, + uint FirstIndex, + int BaseVertex, + int IndexCount, + ulong BindlessTextureHandle, + uint TextureLayer, + TranslucencyKind Translucency); diff --git a/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs b/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs index cb27f87..d0dbd82 100644 --- a/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs +++ b/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs @@ -68,6 +68,17 @@ public sealed unsafe class WbDrawDispatcher : IDisposable private readonly BindlessSupport _bindless; + // Tier 1 cache (#53): per-entity classification results for static + // entities (those NOT in GameWindow._animatedEntities). Wired here in + // Task 7 for plumbing only — Tasks 9-10 wire the per-entity + // miss-populate / hit-fast-path through the loop. + private readonly EntityClassificationCache _cache; + + // ACDREAM_DISABLE_TIER1_CACHE=1 A/B diagnostic — forces every static + // entity through the slow path. Read once in ctor. + private readonly bool _tier1CacheDisabled = + string.Equals(Environment.GetEnvironmentVariable("ACDREAM_DISABLE_TIER1_CACHE"), "1", StringComparison.Ordinal); + /// /// A.5 T22.5: gate for GL_SAMPLE_ALPHA_TO_COVERAGE around the opaque pass. /// Default true matches T20 behavior. Set false for Low/Medium presets that @@ -113,7 +124,15 @@ public sealed unsafe class WbDrawDispatcher : IDisposable // instead of allocating a fresh List<(WorldEntity, int)> per frame. At // ~10K entities × ~3 mesh refs = ~30K tuples × 16 bytes = ~480 KB / frame // of GC pressure on the render thread under the original T17 shape. - private readonly List<(WorldEntity Entity, int MeshRefIndex)> _walkScratch = new(); + private readonly List<(WorldEntity Entity, int MeshRefIndex, uint LandblockId)> _walkScratch = new(); + + // Tier 1 cache (#53) — per-entity classification collector. Reused across + // frames; cleared at flush time when the per-entity loop crosses an entity + // boundary in _walkScratch (and once more at end-of-loop for the last + // entity). _walkScratch is in entity-order, so all MeshRefs of one entity + // are contiguous — accumulate them all before flushing one Populate call. + // Animated entities skip this scratch entirely (collector = null). + private readonly List _populateScratch = new(); // Per-entity-cull AABB radius. Conservative — covers most entities; large // outliers (long banners, tall columns) are still landblock-culled. @@ -139,25 +158,32 @@ public sealed unsafe class WbDrawDispatcher : IDisposable private int _gpuSampleCursor; private bool _gpuQueriesInitialized; - public WbDrawDispatcher( + // Constructor accessibility is internal because EntityClassificationCache + // is internal — a public ctor with an internal-typed parameter would be + // an inconsistent-accessibility error. The dispatcher is constructed + // exclusively from GameWindow (same assembly), so internal is fine. + internal WbDrawDispatcher( GL gl, Shader shader, TextureCache textures, WbMeshAdapter meshAdapter, EntitySpawnAdapter entitySpawnAdapter, - BindlessSupport bindless) + BindlessSupport bindless, + EntityClassificationCache classificationCache) { ArgumentNullException.ThrowIfNull(gl); ArgumentNullException.ThrowIfNull(shader); ArgumentNullException.ThrowIfNull(textures); ArgumentNullException.ThrowIfNull(meshAdapter); ArgumentNullException.ThrowIfNull(entitySpawnAdapter); + ArgumentNullException.ThrowIfNull(classificationCache); _gl = gl; _shader = shader; _textures = textures; _meshAdapter = meshAdapter; _entitySpawnAdapter = entitySpawnAdapter; + _cache = classificationCache; _bindless = bindless ?? throw new ArgumentNullException(nameof(bindless)); _instanceSsbo = _gl.GenBuffer(); @@ -189,7 +215,7 @@ public sealed unsafe class WbDrawDispatcher : IDisposable public struct WalkResult { public int EntitiesWalked; - public List<(WorldEntity Entity, int MeshRefIndex)> ToDraw; + public List<(WorldEntity Entity, int MeshRefIndex, uint LandblockId)> ToDraw; } /// @@ -224,7 +250,7 @@ public sealed unsafe class WbDrawDispatcher : IDisposable HashSet? visibleCellIds, HashSet? animatedEntityIds) { - var scratch = new List<(WorldEntity Entity, int MeshRefIndex)>(); + var scratch = new List<(WorldEntity Entity, int MeshRefIndex, uint LandblockId)>(); var result = new WalkResult { ToDraw = scratch }; WalkEntitiesInto( landblockEntries, frustum, neverCullLandblockId, @@ -244,7 +270,7 @@ public sealed unsafe class WbDrawDispatcher : IDisposable uint? neverCullLandblockId, HashSet? visibleCellIds, HashSet? animatedEntityIds, - List<(WorldEntity Entity, int MeshRefIndex)> scratch, + List<(WorldEntity Entity, int MeshRefIndex, uint LandblockId)> scratch, ref WalkResult result) { scratch.Clear(); @@ -271,7 +297,7 @@ public sealed unsafe class WbDrawDispatcher : IDisposable && !visibleCellIds.Contains(entity.ParentCellId.Value)) continue; result.EntitiesWalked++; for (int i = 0; i < entity.MeshRefs.Count; i++) - scratch.Add((entity, i)); + scratch.Add((entity, i, entry.LandblockId)); } continue; } @@ -297,7 +323,7 @@ public sealed unsafe class WbDrawDispatcher : IDisposable result.EntitiesWalked++; for (int i = 0; i < entity.MeshRefs.Count; i++) - scratch.Add((entity, i)); + scratch.Add((entity, i, entry.LandblockId)); } } } @@ -364,14 +390,169 @@ public sealed unsafe class WbDrawDispatcher : IDisposable _walkScratch, ref walkResult); - foreach (var (entity, partIdx) in _walkScratch) + // Tier 1 cache (#53) flush-tracking locals. _walkScratch holds one tuple + // per (entity, MeshRefIndex) and is in entity-order, so all MeshRefs of + // a given entity are contiguous. We accumulate ALL of an entity's + // batches into _populateScratch, then flush exactly once per entity: + // either when the iteration crosses to a different entity, or at the + // end of the loop for the last entity. Flushing per-tuple would + // overwrite earlier MeshRefs (the cache is keyed by entity.Id), so + // multi-part Setup-backed entities would only retain their LAST + // MeshRef's batches — bug fixed in commit after 2f489a8. + uint? populateEntityId = null; + uint populateLandblockId = 0; + + // Tier 1 cache (#53) — fast-path one-shot tracker. The cache stores a + // FLAT list of batches across all MeshRefs of an entity, so a single + // ApplyCacheHit call already drew every batch. _walkScratch yields + // one tuple per (entity, MeshRefIndex), so without this guard a + // 3-MeshRef static entity on a frame-2 cache hit would call + // ApplyCacheHit 3 times — appending all 6 batches × 3 = 18 instances + // to _groups instead of 6. Result: severe Z-fighting + 3× perf hit + // on every multi-part static entity (buildings, statues, multi-MeshRef + // NPCs). The fast path must fire only on the FIRST tuple of each + // entity; subsequent tuples skip via this tracker. + uint? lastHitEntityId = null; + + // Tier 1 cache (#53) — incomplete-entity guard. When any MeshRef of + // the current entity has _meshAdapter.TryGetRenderData return null + // (mesh still async-decoding via ObjectMeshManager.PrepareMeshDataAsync), + // we mark the entity incomplete and DROP the accumulated populate + // scratch at entity boundary instead of writing it to the cache. + // Otherwise the cache would hold a partial classification (some parts + // missing), and frame-2 cache hits would persist that partial render + // even after the missing mesh loads — every subsequent frame sees the + // cache hit and skips re-classification, so the missing parts never + // recover. User-visible symptom: the drudge statue on top of the + // Foundry (multi-part Setup entity with AnimPartChange) renders with + // some parts missing permanently. Reset on entity change. + bool currentEntityIncomplete = false; + + // Per-tuple entity tracker used purely for entity-change detection. + // Updated UNCONDITIONALLY at end of every tuple (including tuples that + // skip via null renderData), so the flag-reset block below correctly + // distinguishes "new entity" from "same entity, different tuple." + // populateEntityId can't be used for this because it's only set after + // a successful slow-path classification. + uint? prevTupleEntityId = null; + + foreach (var (entity, partIdx, landblockId) in _walkScratch) { if (diag) _entitiesSeen++; + // Skip subsequent tuples of an entity that already cache-hit on + // its first tuple. ApplyCacheHit drew the full flat batch list; + // re-firing here would N-multiply the instance count. Diag + // _entitiesDrawn is bumped here to preserve per-tuple parity with + // the previous counting semantics. + if (lastHitEntityId == entity.Id) + { + if (diag) _entitiesDrawn++; + continue; + } + + // Reset the hit tracker on entity change so the next entity's + // first tuple re-checks the cache. (When this iteration is the + // FIRST tuple of a new entity after a cache-hit entity, we must + // not retain the previous entity's id.) + if (lastHitEntityId.HasValue && lastHitEntityId.Value != entity.Id) + { + lastHitEntityId = null; + } + + // Tier 1 cache (#53) — drop the previous entity's accumulated + // populate scratch BEFORE MaybeFlushOnEntityChange runs. If the + // previous entity ended incomplete (≥1 null renderData), we MUST + // NOT cache its partial classification: clear scratch and null + // the tracker so MaybeFlushOnEntityChange sees the cleaned state + // and no-ops for this entity. Reset the incomplete flag for the + // new entity so each one gets a fresh measurement. + // + // CRITICAL: the flag reset must fire ONLY on entity change, not + // every tuple. Resetting per-tuple within the same entity would + // undo a null-renderData flag set by a previous tuple of the same + // entity → if the missing MeshRef sits in the MIDDLE of the + // entity's MeshRefs list, a later valid tuple's reset would + // re-mark the entity "complete" and let partial data populate + // the cache. Trees with [trunk valid, branches null, leaves + // valid] hit this exactly — branches never recover. + bool isNewEntity = !prevTupleEntityId.HasValue || prevTupleEntityId.Value != entity.Id; + if (isNewEntity) + { + if (populateEntityId.HasValue && currentEntityIncomplete) + { + _populateScratch.Clear(); + populateEntityId = null; + } + currentEntityIncomplete = false; + } + prevTupleEntityId = entity.Id; + + // Flush-on-entity-change: if the previous entity accumulated any + // batches AND this iteration is for a different entity, populate + // its cache entry now and reset the scratch buffer. + (populateEntityId, populateLandblockId) = MaybeFlushOnEntityChange( + populateEntityId, populateLandblockId, entity.Id, _cache, _populateScratch); + var entityWorld = Matrix4x4.CreateFromQuaternion(entity.Rotation) * Matrix4x4.CreateTranslation(entity.Position); + bool isAnimated = animatedEntityIds?.Contains(entity.Id) == true; + + // Cache-hit fast path (Task 10): static entity with a populated + // cache entry skips classification entirely. Walk the cached + // (GroupKey, RestPose) flat list and append cached.RestPose * + // entityWorld to each matching group's matrices. Animated entities + // bypass the cache (collector is set null below; their entries are + // never populated in the first place). + // + // Placed AFTER the entity-change flush above so that, on a + // hit, this iteration also finishes flushing any pending + // populate state from a previous entity. Animated entities never + // enter this branch — the !isAnimated guard makes that explicit. + // + // Fires ONCE per entity: the first tuple reaches here, runs + // ApplyCacheHit, sets lastHitEntityId, and continues. Subsequent + // tuples of the same entity short-circuit at the top of the loop + // body via the lastHitEntityId == entity.Id check above. + if (!isAnimated && !_tier1CacheDisabled && _cache.TryGet(entity.Id, landblockId, out var cachedEntry)) + { + ApplyCacheHit(cachedEntry!, entityWorld, AppendInstanceToGroup); + + // anyVao recovery: when the first visible entity in the frame + // takes the fast path, no slow-path lookup has populated + // anyVao yet. Look up THIS entity's first MeshRef once via + // the mesh adapter — cheap dict lookup, not a re-classify. + if (anyVao == 0) + { + var firstMeshRef = entity.MeshRefs[partIdx]; + var firstRenderData = _meshAdapter.TryGetRenderData(firstMeshRef.GfxObjId); + if (firstRenderData is not null) anyVao = firstRenderData.VAO; + } + + if (diag) _entitiesDrawn++; + lastHitEntityId = entity.Id; + +#if DEBUG + // Cross-check guard: assert the membership predicate held at hit time. + // The full re-classification cross-check (spec section 6.5) is a stretch + // goal; this simpler assert catches the prior Tier 1 bug class — a + // static entity that turns out to actually be animated would fire here. + // + // Structurally redundant with the `if (!isAnimated && ...)` branch + // condition, but serves as a TRIPWIRE: a future refactor that + // incorrectly relaxes the branch condition (e.g., removes + // `!isAnimated` from the guard) would silently allow animated + // entities into the fast path; the assert catches that immediately. + System.Diagnostics.Debug.Assert( + !isAnimated, + $"EntityClassificationCache hit on animated entity {entity.Id} — invariant violated"); +#endif + + continue; + } + // Compute palette-override hash ONCE per entity (perf #4). // Reused across every (part, batch) lookup so the FNV-1a fold // over SubPalettes runs once instead of N times. Zero when the @@ -392,11 +573,25 @@ public sealed unsafe class WbDrawDispatcher : IDisposable var renderData = _meshAdapter.TryGetRenderData(gfxObjId); if (renderData is null) { + // Tier 1 cache (#53): mesh data is still async-decoding via + // WB's ObjectMeshManager.PrepareMeshDataAsync. Flag the entity + // as incomplete so the entity-boundary check (or end-of-loop + // check) drops the accumulated populate scratch instead of + // caching a partial classification. The slow path retries on + // the next frame; once all this entity's meshes have loaded, + // the populate fires with the complete batch set. + currentEntityIncomplete = true; if (diag) _meshesMissing++; continue; } if (anyVao == 0) anyVao = renderData.VAO; + // Cache-miss path (animated entities skip cache entirely). + // Static entities accumulate into _populateScratch across ALL + // their MeshRefs; the flush at next-entity-boundary (or + // end-of-loop) commits them as a single Populate call. + var collector = isAnimated ? null : _populateScratch; + bool drewAny = false; if (renderData.IsSetup && renderData.SetupParts.Count > 0) { @@ -408,20 +603,47 @@ public sealed unsafe class WbDrawDispatcher : IDisposable var model = ComposePartWorldMatrix( entityWorld, meshRef.PartTransform, partTransform); - ClassifyBatches(partData, partGfxObjId, model, entity, meshRef, palHash, metaTable); + var restPose = partTransform * meshRef.PartTransform; + ClassifyBatches(partData, partGfxObjId, model, entity, meshRef, palHash, metaTable, restPose, collector); drewAny = true; } } else { var model = meshRef.PartTransform * entityWorld; - ClassifyBatches(renderData, gfxObjId, model, entity, meshRef, palHash, metaTable); + ClassifyBatches(renderData, gfxObjId, model, entity, meshRef, palHash, metaTable, restPose: meshRef.PartTransform, collector: collector); drewAny = true; } + // Track THIS entity for the next iteration's flush check. Only + // when collector is non-null (entity is static); animated entities + // leave the tracker null so we don't try to flush them. + if (collector is not null) + { + populateEntityId = entity.Id; + populateLandblockId = landblockId; + } + if (diag && drewAny) _entitiesDrawn++; } + // Tier 1 cache (#53) — drop the accumulated populate scratch if the + // LAST entity in the loop ended incomplete (had ≥1 null renderData). + // Same reason as the entity-boundary handling above: avoid caching a + // partial classification. The slow path will retry on the next frame + // and populate correctly once all meshes have loaded. + if (currentEntityIncomplete) + { + _populateScratch.Clear(); + populateEntityId = null; + } + + // Final flush: the last entity in _walkScratch has no "next iteration" + // to trigger the entity-change flush, so commit its accumulated batches + // here. No-op when the last entity was animated (populateEntityId stays + // null) or when no entities walked at all. + FinalFlushPopulate(populateEntityId, populateLandblockId, _cache, _populateScratch); + // Nothing visible — skip the GL pass entirely. if (anyVao == 0) { @@ -704,6 +926,120 @@ public sealed unsafe class WbDrawDispatcher : IDisposable return copy[idx]; } + // ── Tier 1 cache (#53) helpers extracted for testability ───────────────── + // + // Three pure-CPU static helpers carved out of Draw's per-entity loop so + // unit tests can exercise the populate/flush algorithm + cache-hit fast + // path without needing a real GL context. Production code (Draw) calls + // these helpers; the dispatcher integration tests in + // WbDrawDispatcherBucketingTests use them to drive the same algorithm + // through deterministic inputs. + + /// + /// Apply a cache hit's batches into the per-frame group dictionary by + /// composing cached.RestPose * entityWorld per batch and routing + /// the result through . The delegate + /// abstracts over so this helper stays + /// GL-free and unit-testable. + /// + /// + /// Matrix multiplication is non-commutative: it MUST be + /// RestPose * entityWorld, not the reverse. See + /// for the full part-world product. + /// + internal static void ApplyCacheHit( + EntityCacheEntry entry, + Matrix4x4 entityWorld, + Action appendInstance) + { + foreach (var cached in entry.Batches) + { + appendInstance(cached.Key, cached.RestPose * entityWorld); + } + } + + /// + /// Per-tuple flush check. If is set + /// AND differs from , the previous + /// entity's accumulated batches are committed to + /// and is cleared. Returns the + /// updated tracker tuple — pass these back into the field locals in the + /// caller's loop. + /// + /// + /// This is the bug-fix structure from commit 00fa8ae (per-MeshRef + /// Populate would overwrite earlier MeshRefs because the cache is + /// keyed by entity.Id; flushing only on entity boundary preserves all + /// MeshRefs' batches). _walkScratch is in entity-order so all MeshRefs + /// of one entity arrive contiguously. + /// + internal static (uint? PopulateEntityId, uint PopulateLandblockId) + MaybeFlushOnEntityChange( + uint? populateEntityId, + uint populateLandblockId, + uint currentEntityId, + EntityClassificationCache cache, + List populateScratch) + { + if (populateEntityId.HasValue && populateEntityId.Value != currentEntityId) + { + if (populateScratch.Count > 0) + { + cache.Populate(populateEntityId.Value, populateLandblockId, populateScratch.ToArray()); + } + populateScratch.Clear(); + return (null, 0u); + } + return (populateEntityId, populateLandblockId); + } + + /// + /// End-of-loop final flush. The last entity in _walkScratch has + /// no next-iteration to trigger , + /// so commit its accumulated batches here. No-op when no populate is + /// pending (the last entity was animated, or the scratch is empty). + /// + /// End-of-loop only — does NOT reset the caller's tracker locals + /// (intentional, since they go out of scope immediately after). + /// + /// + internal static void FinalFlushPopulate( + uint? populateEntityId, + uint populateLandblockId, + EntityClassificationCache cache, + List populateScratch) + { + if (populateEntityId.HasValue && populateScratch.Count > 0) + { + cache.Populate(populateEntityId.Value, populateLandblockId, populateScratch.ToArray()); + populateScratch.Clear(); + } + } + + /// + /// Instance-side helper used by . Looks up or + /// creates an for the given key in + /// _groups and appends the per-instance world matrix. + /// + private void AppendInstanceToGroup(GroupKey key, Matrix4x4 model) + { + if (!_groups.TryGetValue(key, out var grp)) + { + grp = new InstanceGroup + { + Ibo = key.Ibo, + FirstIndex = key.FirstIndex, + BaseVertex = key.BaseVertex, + IndexCount = key.IndexCount, + BindlessTextureHandle = key.BindlessTextureHandle, + TextureLayer = key.TextureLayer, + Translucency = key.Translucency, + }; + _groups[key] = grp; + } + grp.Matrices.Add(model); + } + private void ClassifyBatches( ObjectRenderData renderData, ulong gfxObjId, @@ -711,7 +1047,9 @@ public sealed unsafe class WbDrawDispatcher : IDisposable WorldEntity entity, MeshRef meshRef, ulong palHash, - AcSurfaceMetadataTable metaTable) + AcSurfaceMetadataTable metaTable, + Matrix4x4 restPose, + List? collector = null) { for (int batchIdx = 0; batchIdx < renderData.Batches.Count; batchIdx++) { @@ -755,6 +1093,7 @@ public sealed unsafe class WbDrawDispatcher : IDisposable _groups[key] = grp; } grp.Matrices.Add(model); + collector?.Add(new CachedBatch(key, texHandle, restPose)); } } @@ -920,15 +1259,6 @@ public sealed unsafe class WbDrawDispatcher : IDisposable // ──────────────────────────────────────────────────────────────────────── - private readonly record struct GroupKey( - uint Ibo, - uint FirstIndex, - int BaseVertex, - int IndexCount, - ulong BindlessTextureHandle, - uint TextureLayer, - TranslucencyKind Translucency); - private sealed class InstanceGroup { public uint Ibo; diff --git a/src/AcDream.App/Streaming/GpuWorldState.cs b/src/AcDream.App/Streaming/GpuWorldState.cs index b0ad321..2965b24 100644 --- a/src/AcDream.App/Streaming/GpuWorldState.cs +++ b/src/AcDream.App/Streaming/GpuWorldState.cs @@ -42,12 +42,26 @@ public sealed class GpuWorldState private readonly LandblockSpawnAdapter? _wbSpawnAdapter; private readonly EntitySpawnAdapter? _wbEntitySpawnAdapter; + /// + /// Phase Post-A.5 #53 (Task 12): optional callback fired before + /// zeroes a landblock's entity + /// list. Wired by GameWindow to + /// EntityClassificationCache.InvalidateLandblock so Tier 1 cache + /// entries get swept on LB demote (Near to Far) and unload. Receives + /// the canonicalized landblock id (low 16 bits forced to 0xFFFF), + /// matching the LandblockHint stored at Populate time. + /// Null when the cache isn't relevant (tests). + /// + private readonly System.Action? _onLandblockUnloaded; + public GpuWorldState( LandblockSpawnAdapter? wbSpawnAdapter = null, - EntitySpawnAdapter? wbEntitySpawnAdapter = null) + EntitySpawnAdapter? wbEntitySpawnAdapter = null, + System.Action? onLandblockUnloaded = null) { _wbSpawnAdapter = wbSpawnAdapter; _wbEntitySpawnAdapter = wbEntitySpawnAdapter; + _onLandblockUnloaded = onLandblockUnloaded; } private readonly Dictionary _loaded = new(); @@ -380,6 +394,14 @@ public sealed class GpuWorldState if (!_loaded.TryGetValue(canonical, out var lb)) return; if (_wbSpawnAdapter is not null) _wbSpawnAdapter.OnLandblockUnloaded(canonical); + + // Phase Post-A.5 #53 (Task 12): invalidate the EntityClassificationCache + // for this landblock BEFORE we drop the entity list. The cache stores + // canonical landblock ids (the dispatcher's _walkScratch carries + // entry.LandblockId from GpuWorldState.LandblockEntries, whose keys are + // canonicalized). Null when the cache isn't wired (tests). Per spec §5.3 W3b. + _onLandblockUnloaded?.Invoke(canonical); + _loaded[canonical] = new LoadedLandblock(lb.LandblockId, lb.Heightmap, System.Array.Empty()); _pendingByLandblock.Remove(canonical); RebuildFlatView(); diff --git a/src/AcDream.Core/World/LandblockLoader.cs b/src/AcDream.Core/World/LandblockLoader.cs index fc3d30e..b18608a 100644 --- a/src/AcDream.Core/World/LandblockLoader.cs +++ b/src/AcDream.Core/World/LandblockLoader.cs @@ -22,7 +22,7 @@ public static class LandblockLoader var info = dats.Get((landblockId & 0xFFFF0000u) | 0xFFFEu); var entities = info is null ? Array.Empty() - : BuildEntitiesFromInfo(info); + : BuildEntitiesFromInfo(info, landblockId); return new LoadedLandblock(landblockId, block, entities); } @@ -33,10 +33,27 @@ public static class LandblockLoader /// (neither GfxObj 0x01xxxxxx nor Setup 0x02xxxxxx) are silently skipped. /// MeshRefs is left empty at this stage — Task 5 populates it. /// - public static IReadOnlyList BuildEntitiesFromInfo(LandBlockInfo info) + public static IReadOnlyList BuildEntitiesFromInfo(LandBlockInfo info, uint landblockId = 0) { var result = new List(info.Objects.Count + info.Buildings.Count); - uint nextId = 1; + + // When landblockId is non-zero, namespace stab Ids globally: + // 0xC0XXYY00 + n, where XX = lbX byte, YY = lbY byte + // matching the scenery (0x80XXYY00) and interior (0x40XXYY00) patterns + // in GameWindow.cs. The 0xC0 top byte distinguishes stabs from those. + // + // Pre-Tier-1 callers (existing tests) pass landblockId=0 and get the + // legacy starting-from-1 monotonic Ids — compatible with their assertions + // which check uniqueness within a single landblock. + // + // Latent: if a landblock has >256 stabs (rare), nextId overflows the + // low byte and bleeds into the lbY byte → cross-LB collision. Same + // pattern + same limitation as scenery/interior. Document but don't + // fix in this commit — out of scope for the Tier 1 cache bug fix. + uint stabIdBase = landblockId == 0 + ? 0u + : 0xC0000000u | ((landblockId >> 24) & 0xFFu) << 16 | ((landblockId >> 16) & 0xFFu) << 8; + uint nextId = stabIdBase == 0 ? 1u : stabIdBase + 1u; foreach (var stab in info.Objects) { diff --git a/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs b/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs new file mode 100644 index 0000000..c52cad8 --- /dev/null +++ b/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs @@ -0,0 +1,285 @@ +using System.Collections.Generic; +using System.Numerics; +using AcDream.App.Rendering.Wb; +using AcDream.Core.Meshing; +using Xunit; + +namespace AcDream.Core.Tests.Rendering.Wb; + +public class EntityClassificationCacheTests +{ + [Fact] + public void TryGet_EmptyCache_ReturnsFalse() + { + var cache = new EntityClassificationCache(); + bool found = cache.TryGet(entityId: 42, landblockHint: 0u, out var entry); + Assert.False(found); + Assert.Null(entry); + } + + [Fact] + public void Populate_ThenTryGet_ReturnsBatchesInOrder() + { + var cache = new EntityClassificationCache(); + var batches = new[] + { + MakeCachedBatch(ibo: 1, firstIndex: 0, indexCount: 6, texHandle: 0xAA), + MakeCachedBatch(ibo: 1, firstIndex: 6, indexCount: 6, texHandle: 0xBB), + }; + + cache.Populate(entityId: 100, landblockHint: 0xA9B40000u, batches); + + Assert.True(cache.TryGet(100, 0xA9B40000u, out var entry)); + Assert.NotNull(entry); + Assert.Equal(100u, entry!.EntityId); + Assert.Equal(0xA9B40000u, entry.LandblockHint); + Assert.Equal(batches, entry.Batches); + } + + [Fact] + public void Populate_OverridesExistingEntry() + { + var cache = new EntityClassificationCache(); + cache.Populate(100, 0u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) }); + cache.Populate(100, 0u, new[] { MakeCachedBatch(2, 0, 12, 0xCC) }); + + Assert.True(cache.TryGet(100, 0u, out var entry)); + Assert.NotNull(entry); + Assert.Single(entry!.Batches); + Assert.Equal(0xCCu, entry.Batches[0].BindlessTextureHandle); + } + + [Fact] + public void Count_TracksLiveEntries() + { + var cache = new EntityClassificationCache(); + Assert.Equal(0, cache.Count); + + cache.Populate(1, 0u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) }); + Assert.Equal(1, cache.Count); + + cache.Populate(2, 0u, new[] { MakeCachedBatch(2, 0, 6, 0xAA) }); + Assert.Equal(2, cache.Count); + + // Re-populate same id — should not double-count. + cache.Populate(1, 0u, new[] { MakeCachedBatch(3, 0, 6, 0xBB) }); + Assert.Equal(2, cache.Count); + } + + [Fact] + public void Populate_WithEmptyBatches_StoresEmptyEntry() + { + var cache = new EntityClassificationCache(); + cache.Populate(entityId: 7, landblockHint: 0u, System.Array.Empty()); + + Assert.True(cache.TryGet(7, 0u, out var entry)); + Assert.NotNull(entry); + Assert.Empty(entry!.Batches); + } + + [Fact] + public void Populate_SetupMultiPart_StoresFlatBatchPerSubPart() + { + // Synthetic Setup with 3 subParts × 2 batches each = 6 flat entries. + // This pins the spec §3 Q4 decision: pre-flatten Setup multi-parts at + // populate time so the per-frame hot path is branchless. + var cache = new EntityClassificationCache(); + var batches = new CachedBatch[6]; + for (int subPart = 0; subPart < 3; subPart++) + for (int b = 0; b < 2; b++) + { + batches[subPart * 2 + b] = MakeCachedBatch( + ibo: (uint)(subPart + 1), + firstIndex: (uint)(b * 6), + indexCount: 6, + texHandle: (ulong)(0x100 + subPart * 2 + b)); + } + cache.Populate(99, 0u, batches); + + Assert.True(cache.TryGet(99, 0u, out var entry)); + Assert.NotNull(entry); + Assert.Equal(6, entry!.Batches.Length); + Assert.Equal(0x100u, entry.Batches[0].BindlessTextureHandle); + Assert.Equal(0x105u, entry.Batches[5].BindlessTextureHandle); + } + + [Fact] + public void InvalidateEntity_RemovesEntry() + { + var cache = new EntityClassificationCache(); + cache.Populate(100, 0u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) }); + Assert.True(cache.TryGet(100, 0u, out _)); + + cache.InvalidateEntity(100); + + Assert.False(cache.TryGet(100, 0u, out var entry)); + Assert.Null(entry); + Assert.Equal(0, cache.Count); + } + + [Fact] + public void InvalidateEntity_OnMissingId_NoThrow() + { + var cache = new EntityClassificationCache(); + var ex = Record.Exception(() => cache.InvalidateEntity(99999)); + Assert.Null(ex); + Assert.Equal(0, cache.Count); + } + + [Fact] + public void InvalidateLandblock_RemovesAllMatchingEntries() + { + var cache = new EntityClassificationCache(); + cache.Populate(1, 0xA9B40000u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) }); + cache.Populate(2, 0xA9B40000u, new[] { MakeCachedBatch(2, 0, 6, 0xBB) }); + cache.Populate(3, 0xA9B40000u, new[] { MakeCachedBatch(3, 0, 6, 0xCC) }); + Assert.Equal(3, cache.Count); + + cache.InvalidateLandblock(0xA9B40000u); + + Assert.Equal(0, cache.Count); + Assert.False(cache.TryGet(1, 0xA9B40000u, out _)); + Assert.False(cache.TryGet(2, 0xA9B40000u, out _)); + Assert.False(cache.TryGet(3, 0xA9B40000u, out _)); + } + + [Fact] + public void InvalidateLandblock_LeavesNonMatchingEntries() + { + var cache = new EntityClassificationCache(); + cache.Populate(1, 0xA9B40000u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) }); + cache.Populate(2, 0xA9B50000u, new[] { MakeCachedBatch(2, 0, 6, 0xBB) }); + cache.Populate(3, 0xA9B40000u, new[] { MakeCachedBatch(3, 0, 6, 0xCC) }); + + cache.InvalidateLandblock(0xA9B40000u); + + Assert.Equal(1, cache.Count); + Assert.False(cache.TryGet(1, 0xA9B40000u, out _)); + Assert.True(cache.TryGet(2, 0xA9B50000u, out var keep)); + Assert.NotNull(keep); + Assert.Equal(0xA9B50000u, keep!.LandblockHint); + Assert.False(cache.TryGet(3, 0xA9B40000u, out _)); + } + + [Fact] + public void InvalidateLandblock_OnMissingLb_NoThrow() + { + var cache = new EntityClassificationCache(); + cache.Populate(1, 0xA9B40000u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) }); + var ex = Record.Exception(() => cache.InvalidateLandblock(0xDEADBEEFu)); + Assert.Null(ex); + Assert.Equal(1, cache.Count); + } + + [Fact] + public void DespawnRespawn_UnderReusedId_RepopulatesFresh() + { + // Pins the audit's ObjDescEvent contract (audit section 1): + // ObjDescEvent is despawn + respawn (with a NEW local entity.Id), + // never an in-place mutation. Even when an id IS reused + // (theoretical — _liveEntityIdCounter is monotonic in practice), + // the cache must serve fresh data after invalidation. + var cache = new EntityClassificationCache(); + var batchesV1 = new[] { MakeCachedBatch(1, 0, 6, 0xAA) }; + var batchesV2 = new[] { MakeCachedBatch(2, 6, 12, 0xCC) }; + + cache.Populate(100, 0xA9B40000u, batchesV1); + cache.InvalidateEntity(100); + cache.Populate(100, 0xA9B40000u, batchesV2); + + Assert.True(cache.TryGet(100, 0xA9B40000u, out var entry)); + Assert.NotNull(entry); + Assert.Equal(batchesV2, entry!.Batches); + Assert.Equal(0xCCu, entry.Batches[0].BindlessTextureHandle); + } + +#if DEBUG + [Fact] + public void DebugCrossCheck_BatchCountMismatch_FiresAssert() + { + var cache = new EntityClassificationCache(); + cache.Populate(100, 0u, new[] + { + MakeCachedBatch(1, 0, 6, 0xAA), + MakeCachedBatch(1, 6, 6, 0xBB), + }); + + // Synthetic "live" with fewer batches → should fire Debug.Assert. + var liveBatches = new[] { MakeCachedBatch(1, 0, 6, 0xAA) }; + + // Capture Debug.Assert via a custom TraceListener. + var originalListeners = new System.Diagnostics.TraceListener[System.Diagnostics.Trace.Listeners.Count]; + System.Diagnostics.Trace.Listeners.CopyTo(originalListeners, 0); + System.Diagnostics.Trace.Listeners.Clear(); + var asserts = new List(); + System.Diagnostics.Trace.Listeners.Add(new CaptureListener(asserts)); + + try + { + cache.DebugCrossCheck(100, 0u, liveBatches); + } + finally + { + System.Diagnostics.Trace.Listeners.Clear(); + foreach (var l in originalListeners) System.Diagnostics.Trace.Listeners.Add(l); + } + + Assert.NotEmpty(asserts); + string joined = string.Join(" ", asserts); + Assert.Contains("batch count mismatch", joined); + } + + [Fact] + public void DebugCrossCheck_RestPoseMatch_NoAssert() + { + var cache = new EntityClassificationCache(); + var batches = new[] { MakeCachedBatch(1, 0, 6, 0xAA) }; + cache.Populate(100, 0u, batches); + + var originalListeners = new System.Diagnostics.TraceListener[System.Diagnostics.Trace.Listeners.Count]; + System.Diagnostics.Trace.Listeners.CopyTo(originalListeners, 0); + System.Diagnostics.Trace.Listeners.Clear(); + var asserts = new List(); + System.Diagnostics.Trace.Listeners.Add(new CaptureListener(asserts)); + + try + { + cache.DebugCrossCheck(100, 0u, batches); + } + finally + { + System.Diagnostics.Trace.Listeners.Clear(); + foreach (var l in originalListeners) System.Diagnostics.Trace.Listeners.Add(l); + } + + Assert.Empty(asserts); + } + + private sealed class CaptureListener : System.Diagnostics.TraceListener + { + private readonly List _captured; + public CaptureListener(List captured) { _captured = captured; } + public override void Write(string? message) { if (message != null) _captured.Add(message); } + public override void WriteLine(string? message) { if (message != null) _captured.Add(message); } + public override void Fail(string? message, string? detailMessage) + { + _captured.Add($"{message}: {detailMessage}"); + } + public override void Fail(string? message) { if (message != null) _captured.Add(message); } + } +#endif + + private static CachedBatch MakeCachedBatch( + uint ibo, uint firstIndex, int indexCount, ulong texHandle) + { + var key = new GroupKey( + Ibo: ibo, + FirstIndex: firstIndex, + BaseVertex: 0, + IndexCount: indexCount, + BindlessTextureHandle: texHandle, + TextureLayer: 0, + Translucency: TranslucencyKind.Opaque); + return new CachedBatch(key, texHandle, Matrix4x4.Identity); + } +} diff --git a/tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs b/tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs index 051dcf2..4f17cd6 100644 --- a/tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs +++ b/tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs @@ -351,4 +351,394 @@ public sealed class WbDrawDispatcherBucketingTests // AabbDirty should have been cleared by the lazy refresh. Assert.False(entity.AabbDirty); } + + // ── Tier 1 cache (#53) dispatcher integration tests ────────────────────── + // + // Tasks 9 & 10 wire the EntityClassificationCache into Draw's per-entity + // loop. These tests exercise the populate + cache-hit fast-path algorithm + // through the static helpers Draw uses (MaybeFlushOnEntityChange, + // FinalFlushPopulate, ApplyCacheHit). The helpers were extracted from + // Draw's foreach for testability — Draw calls them; tests drive them + // directly with deterministic synthesized inputs. This is the same + // pattern WalkEntities follows (extracted from Draw, tested in isolation). + // + // The tests cover spec §7.2 #11 (static populate + reuse) and #12 + // (animated bypass), plus a multi-MeshRef regression test that would + // have caught the bug fixed in commit 00fa8ae (per-MeshRef Populate + // overwrites earlier batches because the cache is keyed by entity.Id). + + /// + /// Helper: constructs a CachedBatch with stable group-key inputs so the + /// hit-path test can verify membership. Mirrors the shape ClassifyBatches + /// produces under the collector pattern. + /// + private static CachedBatch MakeCachedBatch( + uint ibo, uint firstIndex, int indexCount, ulong texHandle, Matrix4x4? restPose = null) + { + var key = new GroupKey( + Ibo: ibo, + FirstIndex: firstIndex, + BaseVertex: 0, + IndexCount: indexCount, + BindlessTextureHandle: texHandle, + TextureLayer: 0, + Translucency: TranslucencyKind.Opaque); + return new CachedBatch(key, texHandle, restPose ?? Matrix4x4.Identity); + } + + [Fact] + public void Draw_StaticEntity_PopulatesCacheOnFirstFrameAndHitsOnSecond() + { + // Spec §7.2 test #11. + // Drives Draw's populate + cache-hit algorithm through the production + // static helpers. Verifies that: + // 1. First "frame": cache is empty → populate fires once at the + // end-of-loop final flush (entity.Id=100 has 2 batches). + // 2. Second "frame": cache.TryGet(100) hits → ApplyCacheHit appends + // cached batches to a fresh _groups dict without re-populating. + // cache.Count stays at 1 (Populate is idempotent via overwrite, + // but the hit-path doesn't re-populate at all). + var cache = new EntityClassificationCache(); + var scratch = new List(); + + Assert.Equal(0, cache.Count); + + // Frame 1: simulate one foreach iteration producing 2 batches for + // entity 100 in landblock 0xA9B40000. With no prior tracker, the + // entity-change flush is a no-op. ClassifyBatches' collector adds + // to scratch. The end-of-loop FinalFlushPopulate commits. + const uint EntityId = 100; + const uint LandblockId = 0xA9B40000u; + + // First MeshRef contributes 2 batches (mimics ClassifyBatches output). + scratch.Add(MakeCachedBatch(ibo: 1, firstIndex: 0, indexCount: 6, texHandle: 0xAA)); + scratch.Add(MakeCachedBatch(ibo: 1, firstIndex: 6, indexCount: 6, texHandle: 0xBB)); + + uint? populateEntityId = null; + uint populateLandblockId = 0u; + // First-tuple boundary check: no flush, sets the tracker. + (populateEntityId, populateLandblockId) = WbDrawDispatcher.MaybeFlushOnEntityChange( + populateEntityId, populateLandblockId, EntityId, cache, scratch); + // After ClassifyBatches the loop sets the tracker (matching Draw). + populateEntityId = EntityId; + populateLandblockId = LandblockId; + + // End-of-loop final flush — this is where the cache populates. + WbDrawDispatcher.FinalFlushPopulate(populateEntityId, populateLandblockId, cache, scratch); + + // First-frame post-conditions: 1 cache entry, 2 batches in it. + Assert.Equal(1, cache.Count); + Assert.True(cache.TryGet(EntityId, LandblockId, out var entry)); + Assert.NotNull(entry); + Assert.Equal(2, entry!.Batches.Length); + Assert.Equal(0xAAul, entry.Batches[0].BindlessTextureHandle); + Assert.Equal(0xBBul, entry.Batches[1].BindlessTextureHandle); + + // Frame 2: cache hit. ApplyCacheHit walks the cached batches and + // appends RestPose * entityWorld to a per-frame group dict. + // Production code: this is the !isAnimated && _cache.TryGet branch + // at the top of the per-entity loop body in Draw. + var groups = new Dictionary>(); + void AppendInstance(GroupKey k, Matrix4x4 m) + { + if (!groups.TryGetValue(k, out var list)) + { + list = new List(); + groups[k] = list; + } + list.Add(m); + } + + Assert.True(cache.TryGet(EntityId, LandblockId, out var entryHit)); + Assert.NotNull(entryHit); + var entityWorld = Matrix4x4.CreateTranslation(new Vector3(10f, 20f, 30f)); + WbDrawDispatcher.ApplyCacheHit(entryHit!, entityWorld, AppendInstance); + + // Cache state stable — Populate didn't fire on the hit path. + Assert.Equal(1, cache.Count); + + // Both groups received exactly one matrix each (the entity is one + // instance contributing once per cached batch). + Assert.Equal(2, groups.Count); + foreach (var (_, list) in groups) + Assert.Single(list); + + // Matrix composition is RestPose * entityWorld (NOT the reverse). + // RestPose is Matrix4x4.Identity for the synthesized batches, so the + // appended matrix must equal entityWorld. + foreach (var (_, list) in groups) + Assert.Equal(entityWorld, list[0]); + } + + [Fact] + public void Draw_AnimatedEntity_DoesNotPopulateCache() + { + // Spec §7.2 test #12. + // Animated entities take the slow path with collector=null: their + // ClassifyBatches output is NOT routed into _populateScratch and the + // populate-tracking locals stay null. Result: the cache is never + // populated for animated entities, and FinalFlushPopulate is a no-op. + // + // This test models that flow: scratch stays empty, populateEntityId + // stays null, FinalFlushPopulate fires but commits nothing. + var cache = new EntityClassificationCache(); + var scratch = new List(); + + const uint AnimatedId = 7; + const uint LandblockId = 0xA9B40000u; + var animatedSet = new HashSet { AnimatedId }; + + // Even when the entity has MeshRefs that would produce batches, the + // animated-set membership means collector=null in Draw — scratch + // stays empty and the tracker stays null. Simulating that here: + // we do NOT add to scratch and we do NOT set populateEntityId. + bool isAnimated = animatedSet.Contains(AnimatedId); + Assert.True(isAnimated); + + uint? populateEntityId = null; + uint populateLandblockId = 0u; + // Boundary check still runs but is a no-op — tracker is null. + (populateEntityId, populateLandblockId) = WbDrawDispatcher.MaybeFlushOnEntityChange( + populateEntityId, populateLandblockId, AnimatedId, cache, scratch); + + // For animated entities, Draw does NOT set populateEntityId after + // ClassifyBatches (the `if (collector is not null)` guard). + // populateEntityId stays null. + + // End-of-loop flush — no-op for animated-only iterations. + WbDrawDispatcher.FinalFlushPopulate(populateEntityId, populateLandblockId, cache, scratch); + + // Cache should never be populated for animated entities. + Assert.Equal(0, cache.Count); + Assert.False(cache.TryGet(AnimatedId, LandblockId, out _)); + } + + [Fact] + public void Draw_MultiMeshRefStaticEntity_PopulatesAllBatchesIntoSingleCacheEntry() + { + // Regression test for the bug fixed at commit 00fa8ae: + // + // Task 9's first attempt called _cache.Populate per-(entity, + // MeshRefIndex) tuple, but the cache is keyed by entity.Id. For + // multi-MeshRef entities (multi-part Setup buildings, statues, + // NPCs), each iteration's Populate OVERWROTE the previous one + // — only the LAST MeshRef's batches survived in the cache. After + // the fix, Populate fires once per entity at the entity boundary + // (or end-of-loop), with all MeshRefs' batches accumulated into + // _populateScratch. + // + // This test simulates a 3-MeshRef static entity where each MeshRef + // contributes 2 batches (total = 6). It walks through Draw's loop + // structure tuple-by-tuple, calling MaybeFlushOnEntityChange before + // each tuple's classification and FinalFlushPopulate at end-of-loop. + // Asserts the cache entry holds ALL 6 batches, not just the last 2. + // + // If the per-MeshRef Populate bug were reintroduced, this test would + // see Batches.Length == 2 (last MeshRef only). + var cache = new EntityClassificationCache(); + var scratch = new List(); + + const uint EntityId = 200; + const uint LandblockId = 0xA9B40000u; + const int MeshRefCount = 3; + const int BatchesPerMeshRef = 2; + const int ExpectedTotalBatches = MeshRefCount * BatchesPerMeshRef; + + uint? populateEntityId = null; + uint populateLandblockId = 0u; + + // Simulate Draw's foreach over _walkScratch. _walkScratch yields + // (entity, MeshRefIndex, landblockId) — all MeshRefs of one entity + // are contiguous because the walk emits them in entity-order. + for (int meshRefIdx = 0; meshRefIdx < MeshRefCount; meshRefIdx++) + { + // Boundary check: same entity across all 3 iterations, so this + // never fires the flush. populateEntityId stays as is (null on + // first iter; EntityId on subsequent iters after we set it). + (populateEntityId, populateLandblockId) = WbDrawDispatcher.MaybeFlushOnEntityChange( + populateEntityId, populateLandblockId, EntityId, cache, scratch); + + // Mimic ClassifyBatches' collector output for THIS MeshRef: + // 2 batches with distinct (ibo, firstIndex, texHandle) so the + // ordering can be verified post-hoc. + for (int b = 0; b < BatchesPerMeshRef; b++) + { + ulong texHandle = (ulong)(0x100 + meshRefIdx * BatchesPerMeshRef + b); + scratch.Add(MakeCachedBatch( + ibo: (uint)(meshRefIdx + 1), + firstIndex: (uint)(b * 6), + indexCount: 6, + texHandle: texHandle)); + } + + // After ClassifyBatches, Draw sets the tracker (matching the + // `if (collector is not null)` block at line 482-486 in + // WbDrawDispatcher.Draw). + populateEntityId = EntityId; + populateLandblockId = LandblockId; + } + + // End-of-loop final flush. Without this call (or if Populate fired + // per-tuple inside the loop), the cache would only hold the last + // 2 batches — exactly the bug class from commit 00fa8ae. + WbDrawDispatcher.FinalFlushPopulate(populateEntityId, populateLandblockId, cache, scratch); + + // Assertions: ONE cache entry with ALL 6 batches in MeshRef order. + Assert.Equal(1, cache.Count); + Assert.True(cache.TryGet(EntityId, LandblockId, out var entry)); + Assert.NotNull(entry); + Assert.Equal(EntityId, entry!.EntityId); + Assert.Equal(LandblockId, entry.LandblockHint); + + // KEY ASSERTION: Batches.Length == sum across MeshRefs (6), + // NOT just the last MeshRef's batch count (2). + Assert.Equal(ExpectedTotalBatches, entry.Batches.Length); + + // Per-batch ordering check: batches arrived in MeshRef order, so + // texture handles run 0x100..0x105 in the order they were appended. + for (int i = 0; i < ExpectedTotalBatches; i++) + Assert.Equal((ulong)(0x100 + i), entry.Batches[i].BindlessTextureHandle); + + // After flush, scratch is cleared so the next entity starts fresh. + Assert.Empty(scratch); + } + + [Fact] + public void Cache_Populate_SkipsEntityWithIncompleteClassification() + { + // Regression test for the bug where an entity with >=1 MeshRef whose + // mesh data was still async-decoding at populate time would have a + // PARTIAL set of batches written to the cache. Subsequent frame + // cache-hits served the partial entry indefinitely, leaving parts of + // multi-part entities (drudge statue, etc.) permanently missing. + // + // The fix: track currentEntityIncomplete during the foreach. If any + // tuple's TryGetRenderData returned null, drop the accumulated + // populate scratch at entity boundary instead of caching it. The + // slow path retries on the next frame; once all meshes have loaded, + // the populate fires correctly with the complete classification. + // + // This test simulates Draw's inner-loop logic: 3 MeshRef tuples for + // one entity where tuple 0 produces null renderData (flag the entity + // incomplete + continue, no batches), and tuples 1 and 2 produce + // valid renderData (classify + accumulate). End-of-loop check drops + // scratch + nulls populateEntityId BEFORE FinalFlushPopulate, so the + // cache stays empty for this entity. + var cache = new EntityClassificationCache(); + const uint EntityId = 100; + const uint LandblockId = 0xA9B40000u; + + // Simulate Draw's per-entity inner-loop logic. + var scratch = new List(); + bool currentEntityIncomplete = false; + uint? populateEntityId = null; + uint populateLandblockId = 0u; + + // Tuple 0 (MeshRef[0]): renderData null -> flag incomplete, skip classify. + currentEntityIncomplete = true; + + // Tuple 1 (MeshRef[1]): renderData valid -> classify, accumulate. + scratch.Add(MakeCachedBatch(ibo: 1, firstIndex: 0, indexCount: 6, texHandle: 0xAAul)); + populateEntityId = EntityId; + populateLandblockId = LandblockId; + + // Tuple 2 (MeshRef[2]): renderData valid -> classify, accumulate. + scratch.Add(MakeCachedBatch(ibo: 2, firstIndex: 0, indexCount: 6, texHandle: 0xBBul)); + populateEntityId = EntityId; + populateLandblockId = LandblockId; + + // End of loop: check incomplete flag, drop scratch + null tracker + // BEFORE FinalFlushPopulate so the helper sees the cleaned state. + if (currentEntityIncomplete) + { + scratch.Clear(); + populateEntityId = null; + } + WbDrawDispatcher.FinalFlushPopulate(populateEntityId, populateLandblockId, cache, scratch); + + // Cache should NOT have an entry for this entity — partial population + // would be worse than no cache (cache hit would persist the partial + // render forever; cache miss retries and gets it right next frame). + Assert.Equal(0, cache.Count); + Assert.False(cache.TryGet(EntityId, LandblockId, out _)); + } + + [Fact] + public void ApplyCacheHit_PerTupleAmplification_DoesNotOccur() + { + // Regression test for the bug fixed at the commit landing alongside + // this test: Task 10's first attempt called ApplyCacheHit per-(entity, + // MeshRefIndex) tuple in Draw's foreach, but cachedEntry.Batches is + // flat across all MeshRefs of the entity. For a 3-MeshRef building on + // frame 2: 3 tuples × 6 cached batches per call = 18 instances drawn + // instead of 6. Severe Z-fighting and 3× perf hit on every multi-part + // static entity (buildings, statues, multi-MeshRef NPCs). + // + // This is the symmetric mirror of the Task 9 bug fixed at 00fa8ae — + // both came from spec §5.2 describing the foreach as per-entity when + // _walkScratch is per-tuple. + // + // The fix: track lastHitEntityId; the cache-hit fast path fires only + // on the FIRST tuple of each entity. Subsequent tuples of the same + // entity skip the iteration body via continue. + // + // This test simulates the inner-loop logic by directly invoking + // ApplyCacheHit + AppendInstanceToGroup the way Draw would, with N + // tuples for the same entity, asserting that groups[key].Count equals + // the cached batch count (6), NOT N × cached batch count (18). + + // Set up a synthetic cache entry with 6 batches (representing 3 + // MeshRefs × 2 batches each). + const int CachedBatchCount = 6; + var cache = new EntityClassificationCache(); + var batches = new CachedBatch[CachedBatchCount]; + for (int i = 0; i < CachedBatchCount; i++) + { + batches[i] = MakeCachedBatch( + ibo: 1u, firstIndex: (uint)i, indexCount: 6, texHandle: (ulong)(0x100 + i)); + } + cache.Populate(entityId: 100, landblockHint: 0xA9B40000u, batches); + + // Simulate Draw's per-entity loop: 3 tuples for the same entity. + // Track which entity has already cache-hit (mirrors the production + // lastHitEntityId pattern). + var groups = new Dictionary>(); + uint? lastHitEntityId = null; + var entityWorld = Matrix4x4.Identity; // simplest case for assertion clarity + const uint EntityId = 100; + const int MeshRefCount = 3; + + void AppendInstance(GroupKey k, Matrix4x4 m) + { + if (!groups.TryGetValue(k, out var list)) + { + list = new List(); + groups[k] = list; + } + list.Add(m); + } + + for (int partIdx = 0; partIdx < MeshRefCount; partIdx++) + { + // Skip subsequent tuples of an entity that cache-hit (the fix). + if (lastHitEntityId == EntityId) continue; + + if (cache.TryGet(EntityId, 0xA9B40000u, out var entry)) + { + Assert.NotNull(entry); + WbDrawDispatcher.ApplyCacheHit(entry!, entityWorld, AppendInstance); + lastHitEntityId = EntityId; + } + } + + // Assertion: each group's matrix count equals the cached batches matching + // that key, NOT (cached batches × MeshRef count). Here each batch has a + // unique key, so each group has exactly 1 matrix. + int totalMatrices = 0; + foreach (var (_, matrices) in groups) totalMatrices += matrices.Count; + Assert.Equal(CachedBatchCount, totalMatrices); // 6, NOT 18 + + // Sanity: 6 distinct keys (one per cached batch since FirstIndex differs). + Assert.Equal(CachedBatchCount, groups.Count); + } } diff --git a/tests/AcDream.Core.Tests/Streaming/GpuWorldStateTwoTierTests.cs b/tests/AcDream.Core.Tests/Streaming/GpuWorldStateTwoTierTests.cs index 11ab0c5..24950fd 100644 --- a/tests/AcDream.Core.Tests/Streaming/GpuWorldStateTwoTierTests.cs +++ b/tests/AcDream.Core.Tests/Streaming/GpuWorldStateTwoTierTests.cs @@ -73,4 +73,53 @@ public class GpuWorldStateTwoTierTests state.AddLandblock(MakeStubLandblock(0xAAAAFFFFu)); Assert.Equal(2, state.Entities.Count); } + + /// + /// Phase Post-A.5 #53 (Task 12): the optional onLandblockUnloaded + /// callback fires once when + /// drops a landblock's entity list, and is passed the canonicalized + /// landblock id (matching the LandblockHint the cache stored at + /// Populate time). + /// + [Fact] + public void RemoveEntitiesFromLandblock_FiresUnloadCallbackWithCanonicalId() + { + uint? observed = null; + int callCount = 0; + var state = new GpuWorldState( + wbSpawnAdapter: null, + wbEntitySpawnAdapter: null, + onLandblockUnloaded: id => { observed = id; callCount++; }); + + state.AddLandblock(MakeStubLandblock(0xA9B4FFFFu, MakeStubEntity(1))); + + // Pass a cell-resolved id (low 16 bits non-FFFF) — the callback must + // receive the canonical (0xFFFF-tail) form, matching what the + // dispatcher's _walkScratch carries from GpuWorldState.LandblockEntries. + state.RemoveEntitiesFromLandblock(0xA9B40042u); + + Assert.Equal(1, callCount); + Assert.Equal(0xA9B4FFFFu, observed); + Assert.Empty(state.Entities); + } + + /// + /// Phase Post-A.5 #53 (Task 12): the callback must NOT fire when the + /// landblock isn't loaded — early return path. Symmetric with the + /// existing _wbSpawnAdapter.OnLandblockUnloaded guard. + /// + [Fact] + public void RemoveEntitiesFromLandblock_NotLoaded_DoesNotFireCallback() + { + int callCount = 0; + var state = new GpuWorldState( + wbSpawnAdapter: null, + wbEntitySpawnAdapter: null, + onLandblockUnloaded: _ => callCount++); + + // Landblock never loaded. + state.RemoveEntitiesFromLandblock(0xA9B4FFFFu); + + Assert.Equal(0, callCount); + } } diff --git a/tests/AcDream.Core.Tests/World/LandblockLoaderTests.cs b/tests/AcDream.Core.Tests/World/LandblockLoaderTests.cs index af68b01..d1d24b8 100644 --- a/tests/AcDream.Core.Tests/World/LandblockLoaderTests.cs +++ b/tests/AcDream.Core.Tests/World/LandblockLoaderTests.cs @@ -116,4 +116,50 @@ public class LandblockLoaderTests var entities = LandblockLoader.BuildEntitiesFromInfo(new LandBlockInfo()); Assert.Empty(entities); } + + [Fact] + public void BuildEntitiesFromInfo_WithLandblockId_NamespacesIdsForGlobalUniqueness() + { + // Regression: cross-LB stab Id collision was the cause of visual + // glitches in Tier 1 cache (commit ) — buildings rendered + // up in the air with wrong textures because cache was keyed by + // entity.Id and stab Ids restarted at 1 per landblock. + var info = new LandBlockInfo + { + Objects = + { + new Stab { Id = 0x01000001u, Frame = new Frame() }, + new Stab { Id = 0x01000002u, Frame = new Frame() }, + }, + }; + + var entitiesLbA = LandblockLoader.BuildEntitiesFromInfo(info, landblockId: 0xA9B40000u); + var entitiesLbB = LandblockLoader.BuildEntitiesFromInfo(info, landblockId: 0xA9B50000u); + + // No two entities across LB A and LB B share the same Id. + var idsA = entitiesLbA.Select(e => e.Id).ToArray(); + var idsB = entitiesLbB.Select(e => e.Id).ToArray(); + Assert.Empty(idsA.Intersect(idsB)); + + // The namespace top byte is 0xC0 for stabs (distinct from 0x80 scenery, + // 0x40 interior, low-range live entities). + Assert.All(idsA, id => Assert.Equal(0xC0u, (id >> 24) & 0xFFu)); + Assert.All(idsB, id => Assert.Equal(0xC0u, (id >> 24) & 0xFFu)); + } + + [Fact] + public void BuildEntitiesFromInfo_LegacyZeroLandblockId_StartsAtOne() + { + // Backward compat: existing callers (tests pre-fix) call without a + // landblockId and get the legacy "starts at 1" behavior. + var info = new LandBlockInfo + { + Objects = { new Stab { Id = 0x01000001u, Frame = new Frame() } }, + }; + + var entities = LandblockLoader.BuildEntitiesFromInfo(info); + + Assert.Single(entities); + Assert.Equal(1u, entities[0].Id); + } }