diff --git a/src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs b/src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs index f50d298..b0e248e 100644 --- a/src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs +++ b/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. /// /// +/// 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 (defensive). -/// is idempotent (no-throw on missing id). +/// 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. @@ -36,26 +49,30 @@ namespace AcDream.App.Rendering.Wb; /// internal sealed class EntityClassificationCache { - private readonly Dictionary _entries = new(); + 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. Returns true with - /// the entry on hit; false with set to - /// null on miss. + /// 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, out EntityCacheEntry? entry) - => _entries.TryGetValue(entityId, out entry); + public bool TryGet(uint entityId, uint landblockHint, out EntityCacheEntry? entry) + => _entries.TryGetValue((entityId, landblockHint), out entry); /// - /// Insert or overwrite a cache entry for . - /// Defensive: if an entry already exists, replaces it. + /// 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] = new EntityCacheEntry + _entries[(entityId, landblockHint)] = new EntityCacheEntry { EntityId = entityId, LandblockHint = landblockHint, @@ -64,12 +81,28 @@ internal sealed class EntityClassificationCache } /// - /// Remove the cache entry for . No-op if the - /// id isn't cached. + /// 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) { - _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); } /// @@ -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? toRemove = null; - foreach (var (id, entry) in _entries) + List<(uint, uint)>? toRemove = null; + foreach (var key in _entries.Keys) { - if (entry.LandblockHint == landblockId) + if (key.LandblockHint == landblockId) { - toRemove ??= new List(); - toRemove.Add(id); + toRemove ??= new List<(uint, uint)>(); + 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. /// /// - public void DebugCrossCheck(uint entityId, IReadOnlyList liveBatches) + public void DebugCrossCheck(uint entityId, uint landblockHint, IReadOnlyList 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, diff --git a/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs b/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs index cdbbb5f..123351a 100644 --- a/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs +++ b/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); diff --git a/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs b/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs index b9d8dff..c52cad8 100644 --- a/tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs +++ b/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()); - 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(2, out _)); - Assert.False(cache.TryGet(3, out _)); + Assert.False(cache.TryGet(1, 0xA9B40000u, out _)); + Assert.False(cache.TryGet(2, 0xA9B40000u, 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.True(cache.TryGet(2, out var keep)); + 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, 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 { diff --git a/tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs b/tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs index 06e814b..ee37668 100644 --- a/tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs +++ b/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 _)); - - // Suppress unused-variable warning — LandblockId is here for parity - // with the static-entity test's structure. - _ = LandblockId; + Assert.False(cache.TryGet(AnimatedId, LandblockId, out _)); } [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);