fix(render #53): cache-hit fast path must fire per-entity, not per-tuple
Task 10 (commit0cbef3c) called ApplyCacheHit inside the per-(entity, partIdx) foreach loop, but cachedEntry.Batches is flat across all MeshRefs of the entity. For a 3-MeshRef static building on frame 2: 3 tuples times 6 cached batches per call = 18 instances drawn instead of 6. Severe Z-fighting and 3x perf hit on every multi-part static entity (buildings, statues, multi- MeshRef NPCs). This is the symmetric mirror of the Task 9 bug fixed at00fa8ae. Both spec section 5.2 and the plan describe the foreach as per-entity, but _walkScratch has been per-tuple since Task 6. The implementation faithfully ported the buggy spec. Fix: track lastHitEntityId; the cache-hit fast path fires only on the first tuple of each entity, and subsequent tuples skip the iteration body via continue. Adds a regression test pinning the per-entity amplification invariant. Caught by code review (subagent-driven-development) before Phase 3 dispatched. The bug was invisible in the no-multi-frame-test 1702/8 baseline; would have manifested as visible Z-fighting on every multi- part building on second-and-subsequent frames once Task 13 perf gate captured live runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
0cbef3c8b3
commit
f7e38c214d
2 changed files with 121 additions and 0 deletions
|
|
@ -397,10 +397,42 @@ public sealed unsafe class WbDrawDispatcher : IDisposable
|
|||
uint? populateEntityId = null;
|
||||
uint populateLandblockId = 0;
|
||||
|
||||
// Tier 1 cache (#53) — fast-path one-shot tracker. The cache stores a
|
||||
// FLAT list of batches across all MeshRefs of an entity, so a single
|
||||
// ApplyCacheHit call already drew every batch. _walkScratch yields
|
||||
// one tuple per (entity, MeshRefIndex), so without this guard a
|
||||
// 3-MeshRef static entity on a frame-2 cache hit would call
|
||||
// ApplyCacheHit 3 times — appending all 6 batches × 3 = 18 instances
|
||||
// to _groups instead of 6. Result: severe Z-fighting + 3× perf hit
|
||||
// on every multi-part static entity (buildings, statues, multi-MeshRef
|
||||
// NPCs). The fast path must fire only on the FIRST tuple of each
|
||||
// entity; subsequent tuples skip via this tracker.
|
||||
uint? lastHitEntityId = null;
|
||||
|
||||
foreach (var (entity, partIdx, landblockId) in _walkScratch)
|
||||
{
|
||||
if (diag) _entitiesSeen++;
|
||||
|
||||
// Skip subsequent tuples of an entity that already cache-hit on
|
||||
// its first tuple. ApplyCacheHit drew the full flat batch list;
|
||||
// re-firing here would N-multiply the instance count. Diag
|
||||
// _entitiesDrawn is bumped here to preserve per-tuple parity with
|
||||
// the previous counting semantics.
|
||||
if (lastHitEntityId == entity.Id)
|
||||
{
|
||||
if (diag) _entitiesDrawn++;
|
||||
continue;
|
||||
}
|
||||
|
||||
// Reset the hit tracker on entity change so the next entity's
|
||||
// first tuple re-checks the cache. (When this iteration is the
|
||||
// FIRST tuple of a new entity after a cache-hit entity, we must
|
||||
// not retain the previous entity's id.)
|
||||
if (lastHitEntityId.HasValue && lastHitEntityId.Value != entity.Id)
|
||||
{
|
||||
lastHitEntityId = null;
|
||||
}
|
||||
|
||||
// Flush-on-entity-change: if the previous entity accumulated any
|
||||
// batches AND this iteration is for a different entity, populate
|
||||
// its cache entry now and reset the scratch buffer.
|
||||
|
|
@ -424,6 +456,11 @@ public sealed unsafe class WbDrawDispatcher : IDisposable
|
|||
// hit, this iteration also finishes flushing any pending
|
||||
// populate state from a previous entity. Animated entities never
|
||||
// enter this branch — the !isAnimated guard makes that explicit.
|
||||
//
|
||||
// Fires ONCE per entity: the first tuple reaches here, runs
|
||||
// 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))
|
||||
{
|
||||
ApplyCacheHit(cachedEntry!, entityWorld, AppendInstanceToGroup);
|
||||
|
|
@ -440,6 +477,7 @@ public sealed unsafe class WbDrawDispatcher : IDisposable
|
|||
}
|
||||
|
||||
if (diag) _entitiesDrawn++;
|
||||
lastHitEntityId = entity.Id;
|
||||
continue;
|
||||
}
|
||||
|
||||
|
|
@ -869,6 +907,10 @@ public sealed unsafe class WbDrawDispatcher : IDisposable
|
|||
/// no next-iteration to trigger <see cref="MaybeFlushOnEntityChange"/>,
|
||||
/// so commit its accumulated batches here. No-op when no populate is
|
||||
/// pending (the last entity was animated, or the scratch is empty).
|
||||
/// <para>
|
||||
/// End-of-loop only — does NOT reset the caller's tracker locals
|
||||
/// (intentional, since they go out of scope immediately after).
|
||||
/// </para>
|
||||
/// </summary>
|
||||
internal static void FinalFlushPopulate(
|
||||
uint? populateEntityId,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue