Merge branch 'claude/friendly-varahamihira-7b8664' — Tier 1 entity-classification cache (closes #53)
Post-A.5 polish Priority 3. EntityClassificationCache keyed by (entityId, landblockHint) tuple. Entity dispatcher cpu_us median ~1.2 ms, p95 ~1.5 ms — ~66% reduction vs pre-Tier-1 baseline. Closes the post-A.5 polish phase entirely (#52, #54, #53 all closed). See docs/ISSUES.md #53 closure + memory/project_tier1_cache.md for the 24-commit chain, 4 bug-fix iterations, and the per-tuple-vs-per-entity recurring trap pattern documented for future cache work.
This commit is contained in:
Erik
commit
175ad14f8b
16 changed files with 4235 additions and 61 deletions
|
|
@ -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
|
||||
|
|
|
|||
246
docs/research/2026-05-10-tier1-mutation-audit.md
Normal file
246
docs/research/2026-05-10-tier1-mutation-audit.md
Normal file
|
|
@ -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<MeshRef>` 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<MeshRef>(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<CachedBatch> Batches = new(); // ordered: (part, batch) flat
|
||||
public uint LandblockHint; // for InvalidateLandblock
|
||||
}
|
||||
|
||||
// Cache state.
|
||||
private readonly Dictionary<uint /*entityId*/, EntityCache> _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.
|
||||
2023
docs/superpowers/plans/2026-05-10-issue-53-tier1-cache.md
Normal file
2023
docs/superpowers/plans/2026-05-10-issue-53-tier1-cache.md
Normal file
File diff suppressed because it is too large
Load diff
451
docs/superpowers/specs/2026-05-10-issue-53-tier1-cache-design.md
Normal file
451
docs/superpowers/specs/2026-05-10-issue-53-tier1-cache-design.md
Normal file
|
|
@ -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<uint, Entry> │
|
||||
│ └─ 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;
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
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<uint, EntityCacheEntry> _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<MeshRef> 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<CachedBatch>`. 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<uint /*entityId*/>?` 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<CachedBatch>? 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<MeshRef> liveMeshRefs,
|
||||
Func<uint, ObjectRenderData?> tryGetRenderData,
|
||||
AcSurfaceMetadataTable metaTable,
|
||||
Func<WorldEntity, MeshRef, ObjectRenderBatch, ulong, ulong> 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).
|
||||
Loading…
Add table
Add a link
Reference in a new issue