fix(render #53): key cache by (entityId, landblockHint) to defeat ID collision

User confirmed via A/B test (ACDREAM_DISABLE_TIER1_CACHE=1) that the
visual bug — buildings rendering up in the air outside Holtburg — is in
the cache wiring, not elsewhere. The matrix math (restPose * entityWorld
== model) was provably correct, so the bug had to be cache key collision.

Stabs were namespaced in commit 71d0edc, but scenery (0x80LLBB00 +
localIndex) and interior (0x40LLBB00 + localCounter) still have the
same 256-overflow risk. Dense LBs outside Holtburg (forest, urban) push
localIndex past 255, wrapping into the lbY byte and creating cross-LB
collisions.

Fix: change the cache key from uint entityId to (uint, uint) tuple of
(EntityId, LandblockHint). The cache is now correct-by-construction
regardless of any hydration path's Id-generation strategy. Defensive
against future regressions in any ID namespace.

InvalidateEntity becomes a sweep (was O(1)), but it's called rarely
(only on live-entity despawn). InvalidateLandblock was already a sweep.

Updated 14 existing cache tests + 1 dispatcher integration test to thread
landblockHint through TryGet / DebugCrossCheck calls.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs

@@ -9,10 +9,23 @@ namespace AcDream.App.Rendering.Wb;
 /// w.r.t. classification logic — it simply stores what callers populate.
 ///
 /// <para>
+/// <b>Key composition:</b> entries are keyed by the tuple
+/// <c>(EntityId, LandblockHint)</c>, NOT by <c>EntityId</c> alone. Issue #53
+/// uncovered that <c>entity.Id</c> is NOT globally unique across all
+/// static-entity hydration paths: scenery (<c>0x80LLBB00 + localIndex</c>)
+/// and interior cells (<c>0x40LLBB00 + localCounter</c>) overflow at >256
+/// items per landblock, wrapping into the <c>lbY</c> 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.
+/// </para>
+///
+/// <para>
 /// <b>Invariants:</b>
 /// <list type="bullet">
-/// <item><see cref="Populate"/> overwrites any existing entry for the same id (defensive).</item>
+/// <item><see cref="Populate"/> overwrites any existing entry for the same (id, lb) tuple (defensive).</item>
-/// <item><see cref="InvalidateEntity"/> is idempotent (no-throw on missing id).</item>
+/// <item><see cref="InvalidateEntity"/> sweeps all entries with the given <c>EntityId</c>
+///   regardless of <c>LandblockHint</c>; idempotent (no-throw on missing id).</item>
 /// <item><see cref="InvalidateLandblock"/> walks all entries; entries whose
 ///   <see cref="EntityCacheEntry.LandblockHint"/> equals the argument are removed.</item>
 /// <item>All operations are render-thread only. No internal locking.</item>
@@ -36,26 +49,30 @@ namespace AcDream.App.Rendering.Wb;
 /// </summary>
 internal sealed class EntityClassificationCache
 {
-    private readonly Dictionary<uint, EntityCacheEntry> _entries = new();
+    private readonly Dictionary<(uint EntityId, uint LandblockHint), EntityCacheEntry> _entries = new();
 
     /// <summary>Number of cached entities — for diagnostics.</summary>
     public int Count => _entries.Count;
 
     /// <summary>
-    /// Look up an entity's cached classification. Returns <c>true</c> with
+    /// Look up an entity's cached classification. Keyed by both
-    /// the entry on hit; <c>false</c> with <paramref name="entry"/> set to
+    /// <paramref name="entityId"/> AND <paramref name="landblockHint"/> to
-    /// <c>null</c> on miss.
+    /// disambiguate entities whose Ids collide across landblocks (e.g.,
+    /// scenery's <c>0x80LLBB00 + localIndex</c> overflow at >256 items/LB).
+    /// Returns <c>true</c> with the entry on hit; <c>false</c> with
+    /// <paramref name="entry"/> set to <c>null</c> on miss.
     /// </summary>
-    public bool TryGet(uint entityId, out EntityCacheEntry? entry)
+    public bool TryGet(uint entityId, uint landblockHint, out EntityCacheEntry? entry)
-        => _entries.TryGetValue(entityId, out entry);
+        => _entries.TryGetValue((entityId, landblockHint), out entry);
 
     /// <summary>
-    /// Insert or overwrite a cache entry for <paramref name="entityId"/>.
+    /// Insert or overwrite a cache entry for the
-    /// Defensive: if an entry already exists, replaces it.
+    /// <c>(<paramref name="entityId"/>, <paramref name="landblockHint"/>)</c>
+    /// tuple. Defensive: if an entry already exists, replaces it.
     /// </summary>
     public void Populate(uint entityId, uint landblockHint, CachedBatch[] batches)
     {
-        _entries[entityId] = new EntityCacheEntry
+        _entries[(entityId, landblockHint)] = new EntityCacheEntry
         {
             EntityId = entityId,
             LandblockHint = landblockHint,
@@ -64,12 +81,28 @@ internal sealed class EntityClassificationCache
     }
 
     /// <summary>
-    /// Remove the cache entry for <paramref name="entityId"/>. No-op if the
+    /// Remove all cache entries for the given <paramref name="entityId"/>,
-    /// id isn't cached.
+    /// 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 <c>O(1)</c> before the #53 tuple-key change;
+    /// now <c>O(n)</c>, but called rarely (only on entity despawn).
     /// </summary>
     public void InvalidateEntity(uint entityId)
     {
-        _entries.Remove(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);
     }
 
     /// <summary>
@@ -82,20 +115,20 @@ internal sealed class EntityClassificationCache
     {
         if (_entries.Count == 0) return;
 
-        // Collect the ids to remove first to avoid mutating the dict during iteration.
+        // 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>? toRemove = null;
+        List<(uint, uint)>? toRemove = null;
-        foreach (var (id, entry) in _entries)
+        foreach (var key in _entries.Keys)
         {
-            if (entry.LandblockHint == landblockId)
+            if (key.LandblockHint == landblockId)
             {
-                toRemove ??= new List<uint>();
+                toRemove ??= new List<(uint, uint)>();
-                toRemove.Add(id);
+                toRemove.Add(key);
             }
         }
         if (toRemove is null) return;
-        foreach (var id in toRemove) _entries.Remove(id);
+        foreach (var k in toRemove) _entries.Remove(k);
     }
 
 #if DEBUG
@@ -121,9 +154,9 @@ internal sealed class EntityClassificationCache
     /// stays here so the regression-class guard is locked behind tests.
     /// </para>
     /// </summary>
-    public void DebugCrossCheck(uint entityId, IReadOnlyList<CachedBatch> liveBatches)
+    public void DebugCrossCheck(uint entityId, uint landblockHint, IReadOnlyList<CachedBatch> liveBatches)
     {
-        if (!_entries.TryGetValue(entityId, out var entry)) return;
+        if (!_entries.TryGetValue((entityId, landblockHint), out var entry)) return;
 
         System.Diagnostics.Debug.Assert(
             entry.Batches.Length == liveBatches.Count,

src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs

@@ -461,7 +461,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 && _cache.TryGet(entity.Id, out var cachedEntry))
+            if (!isAnimated && _cache.TryGet(entity.Id, landblockId, out var cachedEntry))
             {
                 ApplyCacheHit(cachedEntry!, entityWorld, AppendInstanceToGroup);
 

tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs

@@ -12,7 +12,7 @@ public class EntityClassificationCacheTests
     public void TryGet_EmptyCache_ReturnsFalse()
     {
         var cache = new EntityClassificationCache();
-        bool found = cache.TryGet(entityId: 42, out var entry);
+        bool found = cache.TryGet(entityId: 42, landblockHint: 0u, out var entry);
         Assert.False(found);
         Assert.Null(entry);
     }
@@ -29,7 +29,7 @@ public class EntityClassificationCacheTests
 
         cache.Populate(entityId: 100, landblockHint: 0xA9B40000u, batches);
 
-        Assert.True(cache.TryGet(100, out var entry));
+        Assert.True(cache.TryGet(100, 0xA9B40000u, out var entry));
         Assert.NotNull(entry);
         Assert.Equal(100u, entry!.EntityId);
         Assert.Equal(0xA9B40000u, entry.LandblockHint);
@@ -43,7 +43,7 @@ public class EntityClassificationCacheTests
         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, out var entry));
+        Assert.True(cache.TryGet(100, 0u, out var entry));
         Assert.NotNull(entry);
         Assert.Single(entry!.Batches);
         Assert.Equal(0xCCu, entry.Batches[0].BindlessTextureHandle);
@@ -72,7 +72,7 @@ public class EntityClassificationCacheTests
         var cache = new EntityClassificationCache();
         cache.Populate(entityId: 7, landblockHint: 0u, System.Array.Empty<CachedBatch>());
 
-        Assert.True(cache.TryGet(7, out var entry));
+        Assert.True(cache.TryGet(7, 0u, out var entry));
         Assert.NotNull(entry);
         Assert.Empty(entry!.Batches);
     }
@@ -96,7 +96,7 @@ public class EntityClassificationCacheTests
         }
         cache.Populate(99, 0u, batches);
 
-        Assert.True(cache.TryGet(99, out var entry));
+        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);
@@ -108,11 +108,11 @@ public class EntityClassificationCacheTests
     {
         var cache = new EntityClassificationCache();
         cache.Populate(100, 0u, new[] { MakeCachedBatch(1, 0, 6, 0xAA) });
-        Assert.True(cache.TryGet(100, out _));
+        Assert.True(cache.TryGet(100, 0u, out _));
 
         cache.InvalidateEntity(100);
 
-        Assert.False(cache.TryGet(100, out var entry));
+        Assert.False(cache.TryGet(100, 0u, out var entry));
         Assert.Null(entry);
         Assert.Equal(0, cache.Count);
     }
@@ -138,9 +138,9 @@ public class EntityClassificationCacheTests
         cache.InvalidateLandblock(0xA9B40000u);
 
         Assert.Equal(0, cache.Count);
-        Assert.False(cache.TryGet(1, out _));
+        Assert.False(cache.TryGet(1, 0xA9B40000u, out _));
-        Assert.False(cache.TryGet(2, out _));
+        Assert.False(cache.TryGet(2, 0xA9B40000u, out _));
-        Assert.False(cache.TryGet(3, out _));
+        Assert.False(cache.TryGet(3, 0xA9B40000u, out _));
     }
 
     [Fact]
@@ -154,11 +154,11 @@ public class EntityClassificationCacheTests
         cache.InvalidateLandblock(0xA9B40000u);
 
         Assert.Equal(1, cache.Count);
-        Assert.False(cache.TryGet(1, out _));
+        Assert.False(cache.TryGet(1, 0xA9B40000u, out _));
-        Assert.True(cache.TryGet(2, out var keep));
+        Assert.True(cache.TryGet(2, 0xA9B50000u, out var keep));
         Assert.NotNull(keep);
         Assert.Equal(0xA9B50000u, keep!.LandblockHint);
-        Assert.False(cache.TryGet(3, out _));
+        Assert.False(cache.TryGet(3, 0xA9B40000u, out _));
     }
 
     [Fact]
@@ -187,7 +187,7 @@ public class EntityClassificationCacheTests
         cache.InvalidateEntity(100);
         cache.Populate(100, 0xA9B40000u, batchesV2);
 
-        Assert.True(cache.TryGet(100, out var entry));
+        Assert.True(cache.TryGet(100, 0xA9B40000u, out var entry));
         Assert.NotNull(entry);
         Assert.Equal(batchesV2, entry!.Batches);
         Assert.Equal(0xCCu, entry.Batches[0].BindlessTextureHandle);
@@ -216,7 +216,7 @@ public class EntityClassificationCacheTests
 
         try
         {
-            cache.DebugCrossCheck(100, liveBatches);
+            cache.DebugCrossCheck(100, 0u, liveBatches);
         }
         finally
         {
@@ -244,7 +244,7 @@ public class EntityClassificationCacheTests
 
         try
         {
-            cache.DebugCrossCheck(100, batches);
+            cache.DebugCrossCheck(100, 0u, batches);
         }
         finally
         {

tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs

@@ -428,7 +428,7 @@ public sealed class WbDrawDispatcherBucketingTests
 
         // First-frame post-conditions: 1 cache entry, 2 batches in it.
         Assert.Equal(1, cache.Count);
-        Assert.True(cache.TryGet(EntityId, out var entry));
+        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);
@@ -449,7 +449,7 @@ public sealed class WbDrawDispatcherBucketingTests
             list.Add(m);
         }
 
-        Assert.True(cache.TryGet(EntityId, out var entryHit));
+        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);
@@ -510,11 +510,7 @@ public sealed class WbDrawDispatcherBucketingTests
 
         // Cache should never be populated for animated entities.
         Assert.Equal(0, cache.Count);
-        Assert.False(cache.TryGet(AnimatedId, out _));
+        Assert.False(cache.TryGet(AnimatedId, LandblockId, out _));
-
-        // Suppress unused-variable warning — LandblockId is here for parity
-        // with the static-entity test's structure.
-        _ = LandblockId;
     }
 
     [Fact]
@@ -589,7 +585,7 @@ public sealed class WbDrawDispatcherBucketingTests
 
         // Assertions: ONE cache entry with ALL 6 batches in MeshRef order.
         Assert.Equal(1, cache.Count);
-        Assert.True(cache.TryGet(EntityId, out var entry));
+        Assert.True(cache.TryGet(EntityId, LandblockId, out var entry));
         Assert.NotNull(entry);
         Assert.Equal(EntityId, entry!.EntityId);
         Assert.Equal(LandblockId, entry.LandblockHint);
@@ -667,7 +663,7 @@ public sealed class WbDrawDispatcherBucketingTests
             // Skip subsequent tuples of an entity that cache-hit (the fix).
             if (lastHitEntityId == EntityId) continue;
 
-            if (cache.TryGet(EntityId, out var entry))
+            if (cache.TryGet(EntityId, 0xA9B40000u, out var entry))
             {
                 Assert.NotNull(entry);
                 WbDrawDispatcher.ApplyCacheHit(entry!, entityWorld, AppendInstance);