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>
193 lines
8.9 KiB
C#
193 lines
8.9 KiB
C#
using System.Collections.Generic;
|
|
|
|
namespace AcDream.App.Rendering.Wb;
|
|
|
|
/// <summary>
|
|
/// Cache of per-entity classification results for static entities (those NOT
|
|
/// in <c>GameWindow._animatedEntities</c>). Holds one
|
|
/// <see cref="EntityCacheEntry"/> per cached entity. The cache is opaque
|
|
/// 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, 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>
|
|
/// </list>
|
|
/// </para>
|
|
///
|
|
/// <para>
|
|
/// <b>Audit foundation:</b> see
|
|
/// <c>docs/research/2026-05-10-tier1-mutation-audit.md</c> for why static
|
|
/// entities can be cached and what invalidation is needed.
|
|
/// </para>
|
|
///
|
|
/// <para>
|
|
/// <b>Accessibility:</b> <c>internal</c>. <see cref="EntityCacheEntry"/> and
|
|
/// <see cref="CachedBatch"/> both transitively reference the <c>internal</c>
|
|
/// <see cref="GroupKey"/>; surfacing the cache as <c>public</c> would create
|
|
/// inconsistent-accessibility errors. Cross-assembly access for the test
|
|
/// project comes via <c>InternalsVisibleTo("AcDream.Core.Tests")</c> on
|
|
/// <c>AcDream.App.csproj</c>.
|
|
/// </para>
|
|
/// </summary>
|
|
internal sealed class EntityClassificationCache
|
|
{
|
|
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. 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, uint landblockHint, out EntityCacheEntry? entry)
|
|
=> _entries.TryGetValue((entityId, landblockHint), out entry);
|
|
|
|
/// <summary>
|
|
/// 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, landblockHint)] = new EntityCacheEntry
|
|
{
|
|
EntityId = entityId,
|
|
LandblockHint = landblockHint,
|
|
Batches = batches,
|
|
};
|
|
}
|
|
|
|
/// <summary>
|
|
/// 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)
|
|
{
|
|
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>
|
|
/// Remove every cache entry whose <see cref="EntityCacheEntry.LandblockHint"/>
|
|
/// equals <paramref name="landblockId"/>. Used by the streaming pipeline
|
|
/// when a landblock demotes from near to far or unloads. No-op if no
|
|
/// entries match.
|
|
/// </summary>
|
|
public void InvalidateLandblock(uint landblockId)
|
|
{
|
|
if (_entries.Count == 0) return;
|
|
|
|
// 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, uint)>? toRemove = null;
|
|
foreach (var key in _entries.Keys)
|
|
{
|
|
if (key.LandblockHint == landblockId)
|
|
{
|
|
toRemove ??= new List<(uint, uint)>();
|
|
toRemove.Add(key);
|
|
}
|
|
}
|
|
if (toRemove is null) return;
|
|
foreach (var k in toRemove) _entries.Remove(k);
|
|
}
|
|
|
|
#if DEBUG
|
|
/// <summary>
|
|
/// Asserts that the cached entry for <paramref name="entityId"/> still
|
|
/// matches what fresh classification would produce. Catches the prior
|
|
/// Tier 1 bug class — silent caching of mutable per-frame state — by
|
|
/// firing <see cref="System.Diagnostics.Debug.Assert"/> when any cached
|
|
/// field has drifted from live state.
|
|
///
|
|
/// <para>
|
|
/// Caller passes per-batch live state (Key, BindlessTextureHandle, RestPose)
|
|
/// reconstructed from the same path the populate ran. The cache iterates
|
|
/// its stored entries in parallel and asserts equality.
|
|
/// </para>
|
|
///
|
|
/// <para>
|
|
/// As of Phase 4 (commit f16604b) this method is exercised by unit tests
|
|
/// only; the dispatcher's cache-hit branch fires a simpler predicate assert
|
|
/// (<c>!isAnimated</c>) at production hit time. Wiring the full live-state
|
|
/// cross-check into the per-entity branch is the spec section 6.5 stretch
|
|
/// goal and remains open as a follow-up. Zero cost in Release; the method
|
|
/// stays here so the regression-class guard is locked behind tests.
|
|
/// </para>
|
|
/// </summary>
|
|
public void DebugCrossCheck(uint entityId, uint landblockHint, IReadOnlyList<CachedBatch> liveBatches)
|
|
{
|
|
if (!_entries.TryGetValue((entityId, landblockHint), out var entry)) return;
|
|
|
|
System.Diagnostics.Debug.Assert(
|
|
entry.Batches.Length == liveBatches.Count,
|
|
$"EntityClassificationCache: batch count mismatch for entity {entityId}: cached={entry.Batches.Length} live={liveBatches.Count}");
|
|
|
|
for (int i = 0; i < entry.Batches.Length && i < liveBatches.Count; i++)
|
|
{
|
|
var cached = entry.Batches[i];
|
|
var live = liveBatches[i];
|
|
System.Diagnostics.Debug.Assert(
|
|
cached.Key.Equals(live.Key),
|
|
$"EntityClassificationCache: GroupKey drift for entity {entityId} batch {i}");
|
|
System.Diagnostics.Debug.Assert(
|
|
cached.BindlessTextureHandle == live.BindlessTextureHandle,
|
|
$"EntityClassificationCache: texture handle drift for entity {entityId} batch {i}");
|
|
System.Diagnostics.Debug.Assert(
|
|
MatrixApproxEqual(cached.RestPose, live.RestPose, epsilon: 1e-5f),
|
|
$"EntityClassificationCache: RestPose drift for entity {entityId} batch {i}");
|
|
}
|
|
}
|
|
|
|
private static bool MatrixApproxEqual(System.Numerics.Matrix4x4 a, System.Numerics.Matrix4x4 b, float epsilon)
|
|
{
|
|
return System.MathF.Abs(a.M11 - b.M11) <= epsilon && System.MathF.Abs(a.M12 - b.M12) <= epsilon &&
|
|
System.MathF.Abs(a.M13 - b.M13) <= epsilon && System.MathF.Abs(a.M14 - b.M14) <= epsilon &&
|
|
System.MathF.Abs(a.M21 - b.M21) <= epsilon && System.MathF.Abs(a.M22 - b.M22) <= epsilon &&
|
|
System.MathF.Abs(a.M23 - b.M23) <= epsilon && System.MathF.Abs(a.M24 - b.M24) <= epsilon &&
|
|
System.MathF.Abs(a.M31 - b.M31) <= epsilon && System.MathF.Abs(a.M32 - b.M32) <= epsilon &&
|
|
System.MathF.Abs(a.M33 - b.M33) <= epsilon && System.MathF.Abs(a.M34 - b.M34) <= epsilon &&
|
|
System.MathF.Abs(a.M41 - b.M41) <= epsilon && System.MathF.Abs(a.M42 - b.M42) <= epsilon &&
|
|
System.MathF.Abs(a.M43 - b.M43) <= epsilon && System.MathF.Abs(a.M44 - b.M44) <= epsilon;
|
|
}
|
|
#endif
|
|
}
|