From 21633080327432c43f9d15a6644ba397fbfacb1a Mon Sep 17 00:00:00 2001 From: Erik Date: Thu, 11 Jun 2026 21:43:45 +0200 Subject: [PATCH] #119 ROOT CAUSE: interior-id X-byte collision + player-landblock cache hints = cross-entity batch serving The decisive probe (3cf6bcc) caught it live in ONE session: a 43-part staircase entity (src=0x020003F2, healthy MeshRefs tZ=[0.35..15.15]) drew with cache=hit:3 restZero=3 - THREE batches belonging to a 1-part entity - then under a different hint the correct hit:119. Two compounding bugs: 1. interiorIdBase = 0x40000000 | (landblockId & 0x00FFFF00) resolved to 0x40YYFF00 for landblock keys 0xXXYYFFFF - the landblock X byte DISCARDED. Every landblock in a map Y-row shared one id space: Holtburg town A9B3's 9th interior stab == the AAB3 tower's spiral staircase, both 0x40B3FF09. Fixed to 0x40000000|(lbX<<16)|(lbY<<8) (the scenery 0x80XXYY## scheme). 2. The Tier-1 classification cache's #53 tuple key (EntityId, LandblockHint) was fed the PLAYER's landblock at bucket-draw time (RetailPViewRenderer.DrawEntityBucket fabricates its tuple with ctx.PlayerLandblockId), so colliding ids from different landblocks shared a key: whichever entity classified first under a hint won, and the loser wore its batches all session (static fast path never re-classifies). Also: bucket-hinted entries were never swept by InvalidateLandblock(owner) - stale entries survived owner unload. Fixed: ResolveCacheLandblockHint derives the hint from the entity's owning cell (ParentCellId landblock, canonical 0xXXYYFFFF), falling back to the tuple id for ownerless paths (outdoor stabs/scenery, where the tuple IS the owner). Explains the session-shaped repro exactly: town-login + run to the tower hydrates/classifies town interiors first -> the tower staircase cache-hits the town twin's batches (stairs missing/partial + a wrong object near the floor - the "water barrel"); login-inside classifies the tower first -> usually clean. meshMissing=0 / entSeen==entDrawn both ways (everything draws, wrong batches). Likely also feeds #113's distance-dependent phantom staircase (the town twin wearing the tower's staircase batches). 3 new cache tests pin the collision contract + hint derivation. Suites: App green / Core 1430+2skip / UI 420 / Net 294. Co-Authored-By: Claude Fable 5 --- src/AcDream.App/Rendering/GameWindow.cs | 21 ++++++- .../Rendering/Wb/EntityClassificationCache.cs | 15 +++-- .../Rendering/Wb/WbDrawDispatcher.cs | 36 ++++++++++- .../Wb/EntityClassificationCacheTests.cs | 60 +++++++++++++++++++ 4 files changed, 122 insertions(+), 10 deletions(-) diff --git a/src/AcDream.App/Rendering/GameWindow.cs b/src/AcDream.App/Rendering/GameWindow.cs index d2a71d44..cad6484d 100644 --- a/src/AcDream.App/Rendering/GameWindow.cs +++ b/src/AcDream.App/Rendering/GameWindow.cs @@ -5514,9 +5514,24 @@ public sealed class GameWindow : IDisposable (lbY - _liveCenterY) * 192f, 0f); - // Per-landblock id namespace: 0x40000000 | (lbId & 0x00FFFF00) | local_counter. - // Distinct from scenery (0x80000000+) and stabs (ids from LandblockLoader). - uint interiorIdBase = 0x40000000u | (landblockId & 0x00FFFF00u); + // Per-landblock id namespace: 0x40000000 | (lbX << 16) | (lbY << 8) | local_counter — + // the same 0xNNXXYY## scheme scenery uses (0x80XXYY##). Distinct from scenery + // (0x80000000+) and stabs (ids from LandblockLoader). + // + // #119 ROOT-CAUSE FIX (2026-06-11): this used to be + // `0x40000000 | (landblockId & 0x00FFFF00)`, which for landblock keys 0xXXYYFFFF + // resolves to 0x40YYFF00 — the landblock X byte DISCARDED. Every landblock in a + // map Y-row produced the same id base, so interior statics collided across + // landblocks (Holtburg town A9B3's 9th stab == the AAB3 tower's 43-part spiral + // staircase, both 0x40B3FF09). The Tier-1 classification cache then served one + // entity's batches to the other (the cache hint at bucket-draw time was the + // player's landblock, identical for both) — the session-sticky "broken stairs + + // water barrel". Counter overflow past 0xFF still bleeds into the lbY byte (the + // documented #53 scenery-namespace caveat); the cache's (EntityId, owner-derived + // LandblockHint) tuple key disambiguates that residual case. + uint interiorLbX = (landblockId >> 24) & 0xFFu; + uint interiorLbY = (landblockId >> 16) & 0xFFu; + uint interiorIdBase = 0x40000000u | (interiorLbX << 16) | (interiorLbY << 8); uint localCounter = 0; uint firstCellId = (landblockId & 0xFFFF0000u) | 0x0100u; diff --git a/src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs b/src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs index b0e248e4..4023541c 100644 --- a/src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs +++ b/src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs @@ -12,12 +12,19 @@ namespace AcDream.App.Rendering.Wb; /// 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 +/// static-entity hydration paths: scenery (0x80XXYY00 + localIndex) +/// and interior cells (0x40XXYY00 + localCounter, X-byte fixed +/// 2026-06-11 — it used to be discarded entirely, #119) 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. +/// by the tuple is correct-by-construction ONLY when the hint identifies the +/// entity's OWNING landblock — callers must derive it via +/// WbDrawDispatcher.ResolveCacheLandblockHint (the entity's +/// ParentCellId landblock when present, canonicalized 0xXXYYFFFF), +/// never a call-site landblock. The #119 "broken stairs + water barrel" was +/// exactly this: the bucket draw path hinted every entity with the PLAYER's +/// landblock, so colliding ids from different landblocks shared a key and +/// served each other's batches. /// /// /// diff --git a/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs b/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs index 4b36c689..e266be8c 100644 --- a/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs +++ b/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs @@ -736,6 +736,28 @@ public sealed unsafe class WbDrawDispatcher : IDisposable } } + /// + /// #119 ROOT-CAUSE FIX (2026-06-11): the Tier-1 cache hint must identify the + /// entity's OWNING landblock, not the Draw call's tuple landblock. + /// RetailPViewRenderer.DrawEntityBucket fabricates its tuple with the + /// PLAYER's landblock id, so every bucket entity that frame shared one hint — + /// and colliding entity ids from different landblocks (the pre-fix + /// 0x40YYFF00 interior namespace discarded the landblock X byte) mapped + /// to the SAME cache key and served each other's batches: the AAB3 tower's + /// 43-part staircase drew a 1-part entity's 3 zero-RestPose batches + /// (captured live, tower-dump-launch1.log) — the session-sticky "broken + /// stairs + water barrel". Interior statics carry their owning cell; derive + /// the hint from it, canonicalized to the same 0xXXYYFFFF key format + /// the streaming entries and + /// use — which also makes owner-unload invalidation actually hit these + /// entries (bucket-hinted entries were previously orphaned forever). + /// Entities without a ParentCellId (outdoor stabs / scenery / building + /// shells via GpuWorldState entries) keep the tuple id, which IS their + /// owner on those paths. + /// + internal static uint ResolveCacheLandblockHint(WorldEntity entity, uint tupleLandblockId) + => entity.ParentCellId is uint pc ? ((pc & 0xFFFF0000u) | 0xFFFFu) : tupleLandblockId; + /// /// #119 decisive probe: rate-limited [dump-entity] WALK-REJECT line /// for an ACDREAM_DUMP_ENTITY-targeted entity that the walk filtered @@ -1005,6 +1027,11 @@ public sealed unsafe class WbDrawDispatcher : IDisposable // 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. + // #119 root-cause fix: cache operations key on the entity's OWNING + // landblock, never the Draw call's tuple landblock (which is the + // PLAYER's landblock on the bucket path). See ResolveCacheLandblockHint. + uint cacheLb = ResolveCacheLandblockHint(entity, landblockId); + bool isNewEntity = !prevTupleEntityId.HasValue || prevTupleEntityId.Value != entity.Id; if (isNewEntity) { @@ -1029,7 +1056,7 @@ public sealed unsafe class WbDrawDispatcher : IDisposable // #119 decisive probe: one-shot dump (+ change re-emission) for // ACDREAM_DUMP_ENTITY-targeted entities. Before the culled-continue // so a routed-out entity still reports its state. - MaybeEmitEntityDump(entity, landblockId, _currentEntityCulled); + MaybeEmitEntityDump(entity, cacheLb, _currentEntityCulled); } prevTupleEntityId = entity.Id; @@ -1072,7 +1099,7 @@ public sealed unsafe class WbDrawDispatcher : IDisposable // 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)) + if (!isAnimated && !_tier1CacheDisabled && _cache.TryGet(entity.Id, cacheLb, out var cachedEntry)) { ApplyCacheHit(cachedEntry!, entityWorld, AppendInstanceToGroup); @@ -1254,10 +1281,13 @@ public sealed unsafe class WbDrawDispatcher : IDisposable // 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. + // #119: the populate commits under the OWNER-derived hint so the + // entry is found by the same key on the next frame's TryGet and + // swept by InvalidateLandblock when the OWNING landblock unloads. if (collector is not null) { populateEntityId = entity.Id; - populateLandblockId = landblockId; + populateLandblockId = cacheLb; } if (diag && drewAny) _entitiesDrawn++; diff --git a/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs b/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs index c52cad8e..16b6e878 100644 --- a/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs +++ b/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs @@ -49,6 +49,66 @@ public class EntityClassificationCacheTests Assert.Equal(0xCCu, entry.Batches[0].BindlessTextureHandle); } + // ── #119 root-cause regression (2026-06-11): colliding entity ids across + // landblocks must never share a cache entry. The bucket draw path used to + // hint every entity with the PLAYER's landblock, so the AAB3 tower's + // 43-part staircase (id 0x40B3FF09) cache-hit the batches of Holtburg + // town A9B3's 9th interior stab (same id under the old 0x40YYFF00 + // namespace) — "broken stairs + water barrel". The hint must be derived + // from the entity's OWNER via WbDrawDispatcher.ResolveCacheLandblockHint. + + [Fact] + public void ResolveCacheLandblockHint_InteriorEntity_DerivesOwnerLandblock() + { + var towerStairs = MakeEntity(id: 0x40AAB309u, parentCellId: 0xAAB30107u); + // Tuple landblock = the PLAYER's landblock (the bucket path) — must be ignored. + uint hint = WbDrawDispatcher.ResolveCacheLandblockHint(towerStairs, tupleLandblockId: 0xA9B3FFFFu); + Assert.Equal(0xAAB3FFFFu, hint); + } + + [Fact] + public void ResolveCacheLandblockHint_NoParentCell_KeepsTupleLandblock() + { + var outdoorStab = MakeEntity(id: 0x00001234u, parentCellId: null); + uint hint = WbDrawDispatcher.ResolveCacheLandblockHint(outdoorStab, tupleLandblockId: 0xA9B4FFFFu); + Assert.Equal(0xA9B4FFFFu, hint); + } + + [Fact] + public void CollidingEntityIds_UnderOwnerHints_KeepDistinctBatchSets() + { + // Same entity id (the residual >256-counter overlap the tuple key exists + // for), two different owning landblocks → two entries, each serving its + // own batches. Pre-fix, both would have been keyed under one player-lb + // hint and the second entity would draw the first one's batches. + var cache = new EntityClassificationCache(); + var townBatches = new[] { MakeCachedBatch(1, 0, 6, 0xAA) }; + var towerBatches = new[] { MakeCachedBatch(2, 0, 12, 0xBB), MakeCachedBatch(2, 12, 6, 0xCC) }; + + cache.Populate(0x40B3FF09u, 0xA9B3FFFFu, townBatches); + cache.Populate(0x40B3FF09u, 0xAAB3FFFFu, towerBatches); + + Assert.True(cache.TryGet(0x40B3FF09u, 0xA9B3FFFFu, out var town)); + Assert.True(cache.TryGet(0x40B3FF09u, 0xAAB3FFFFu, out var tower)); + Assert.Equal(townBatches, town!.Batches); + Assert.Equal(towerBatches, tower!.Batches); + + // Owner-landblock invalidation sweeps ONLY the owner's entry. + cache.InvalidateLandblock(0xA9B3FFFFu); + Assert.False(cache.TryGet(0x40B3FF09u, 0xA9B3FFFFu, out _)); + Assert.True(cache.TryGet(0x40B3FF09u, 0xAAB3FFFFu, out _)); + } + + private static AcDream.Core.World.WorldEntity MakeEntity(uint id, uint? parentCellId) => new() + { + Id = id, + SourceGfxObjOrSetupId = 0x020003F2u, + Position = Vector3.Zero, + Rotation = Quaternion.Identity, + MeshRefs = new List(), + ParentCellId = parentCellId, + }; + [Fact] public void Count_TracksLiveEntries() {