erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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

Browse source 
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>
This commit is contained in:
Erik 2026-05-10 23:02:14 +02:00
parent 71d0edc3d7
commit 95ebbf3004
4 changed files with 78 additions and 49 deletions
Download patch file Download diff file Expand all files Collapse all files

79
src/AcDream.App/Rendering/Wb/EntityClassificationCache.cs
View file

@ -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="InvalidateEntity"/> is idempotent (no-throw on missing id).</item>
/// <item><see cref="Populate"/> overwrites any existing entry for the same (id, lb) tuple (defensive).</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
/// the entry on hit; <c>false</c> with <paramref name="entry"/> set to
/// <c>null</c> on miss.
/// Look up an entity's cached classification. Keyed by both
/// <paramref name="entityId"/> AND <paramref name="landblockHint"/> to
/// 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)
=> _entries.TryGetValue(entityId, out entry);
public bool TryGet(uint entityId, uint landblockHint, out EntityCacheEntry? entry)
=> _entries.TryGetValue((entityId, landblockHint), out entry);
/// <summary>
/// Insert or overwrite a cache entry for <paramref name="entityId"/>.
/// Defensive: if an entry already exists, replaces it.
/// Insert or overwrite a cache entry for the
/// <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
/// id isn't cached.
/// Remove all cache entries for the given <paramref name="entityId"/>,
/// 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;
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<uint>();
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.
/// </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,

2
src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs
View file

@ -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);

32
tests/AcDream.Core.Tests/Rendering/Wb/EntityClassificationCacheTests.cs
View file

@ -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(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
{

14
tests/AcDream.Core.Tests/Rendering/Wb/WbDrawDispatcherBucketingTests.cs
View file

@ -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);