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 <noreply@anthropic.com>
200 lines
9.4 KiB
C#
200 lines
9.4 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>0x80XXYY00 + localIndex</c>)
|
|
/// and interior cells (<c>0x40XXYY00 + localCounter</c>, X-byte fixed
|
|
/// 2026-06-11 — it used to be discarded entirely, #119) 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 ONLY when the hint identifies the
|
|
/// entity's OWNING landblock — callers must derive it via
|
|
/// <c>WbDrawDispatcher.ResolveCacheLandblockHint</c> (the entity's
|
|
/// ParentCellId landblock when present, canonicalized <c>0xXXYYFFFF</c>),
|
|
/// 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.
|
|
/// </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
|
|
}
|