fix(render #53): cache Populate must flush at entity boundary, not per-MeshRef tuple
Task 9 (commit 2f489a8) called _cache.Populate inside the per-tuple
foreach loop, but _walkScratch contains one tuple per (entity, MeshRefIndex)
and the cache is keyed by entity.Id. For multi-MeshRef entities (multi-part
Setup buildings, statues, multi-MeshRef NPCs), each iteration's Populate
OVERWROTE the previous one — only the last MeshRef's batches survived.
The bug was invisible at commit time because Task 10 had not landed
(cache populates but isn't read). It would have manifested the moment
Task 10 wired the cache-hit fast path: every multi-part static building
in Holtburg would render as N stacked copies of its last part.
Fix: restructure the per-entity loop with a flush-on-entity-change pattern.
Track the previous entity's Id; when the iteration moves to a different
entity, flush the previous entity's accumulated _populateScratch via one
Populate call. After the loop, flush the final entity. _populateScratch
is now cleared at flush time, not per-iteration.
Caught by code review (subagent-driven-development) before Task 10 dispatched.
Verified: 1699/8 baseline preserved, sentinel 105/105 unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
2f489a83a7
commit
00fa8ae839
1 changed files with 50 additions and 10 deletions
|
|
@ -122,8 +122,11 @@ public sealed unsafe class WbDrawDispatcher : IDisposable
|
||||||
private readonly List<(WorldEntity Entity, int MeshRefIndex, uint LandblockId)> _walkScratch = new();
|
private readonly List<(WorldEntity Entity, int MeshRefIndex, uint LandblockId)> _walkScratch = new();
|
||||||
|
|
||||||
// Tier 1 cache (#53) — per-entity classification collector. Reused across
|
// Tier 1 cache (#53) — per-entity classification collector. Reused across
|
||||||
// frames; cleared once per static entity inside Draw. Animated entities
|
// frames; cleared at flush time when the per-entity loop crosses an entity
|
||||||
// skip this scratch entirely (collector = null).
|
// boundary in _walkScratch (and once more at end-of-loop for the last
|
||||||
|
// entity). _walkScratch is in entity-order, so all MeshRefs of one entity
|
||||||
|
// are contiguous — accumulate them all before flushing one Populate call.
|
||||||
|
// Animated entities skip this scratch entirely (collector = null).
|
||||||
private readonly List<CachedBatch> _populateScratch = new();
|
private readonly List<CachedBatch> _populateScratch = new();
|
||||||
|
|
||||||
// Per-entity-cull AABB radius. Conservative — covers most entities; large
|
// Per-entity-cull AABB radius. Conservative — covers most entities; large
|
||||||
|
|
@ -382,10 +385,35 @@ public sealed unsafe class WbDrawDispatcher : IDisposable
|
||||||
_walkScratch,
|
_walkScratch,
|
||||||
ref walkResult);
|
ref walkResult);
|
||||||
|
|
||||||
|
// Tier 1 cache (#53) flush-tracking locals. _walkScratch holds one tuple
|
||||||
|
// per (entity, MeshRefIndex) and is in entity-order, so all MeshRefs of
|
||||||
|
// a given entity are contiguous. We accumulate ALL of an entity's
|
||||||
|
// batches into _populateScratch, then flush exactly once per entity:
|
||||||
|
// either when the iteration crosses to a different entity, or at the
|
||||||
|
// end of the loop for the last entity. Flushing per-tuple would
|
||||||
|
// overwrite earlier MeshRefs (the cache is keyed by entity.Id), so
|
||||||
|
// multi-part Setup-backed entities would only retain their LAST
|
||||||
|
// MeshRef's batches — bug fixed in commit after 2f489a8.
|
||||||
|
uint? populateEntityId = null;
|
||||||
|
uint populateLandblockId = 0;
|
||||||
|
|
||||||
foreach (var (entity, partIdx, landblockId) in _walkScratch)
|
foreach (var (entity, partIdx, landblockId) in _walkScratch)
|
||||||
{
|
{
|
||||||
if (diag) _entitiesSeen++;
|
if (diag) _entitiesSeen++;
|
||||||
|
|
||||||
|
// 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.
|
||||||
|
if (populateEntityId.HasValue && populateEntityId.Value != entity.Id)
|
||||||
|
{
|
||||||
|
if (_populateScratch.Count > 0)
|
||||||
|
{
|
||||||
|
_cache.Populate(populateEntityId.Value, populateLandblockId, _populateScratch.ToArray());
|
||||||
|
}
|
||||||
|
_populateScratch.Clear();
|
||||||
|
populateEntityId = null;
|
||||||
|
}
|
||||||
|
|
||||||
var entityWorld =
|
var entityWorld =
|
||||||
Matrix4x4.CreateFromQuaternion(entity.Rotation) *
|
Matrix4x4.CreateFromQuaternion(entity.Rotation) *
|
||||||
Matrix4x4.CreateTranslation(entity.Position);
|
Matrix4x4.CreateTranslation(entity.Position);
|
||||||
|
|
@ -418,12 +446,12 @@ public sealed unsafe class WbDrawDispatcher : IDisposable
|
||||||
if (anyVao == 0) anyVao = renderData.VAO;
|
if (anyVao == 0) anyVao = renderData.VAO;
|
||||||
|
|
||||||
// Cache-miss path (animated entities skip cache entirely).
|
// Cache-miss path (animated entities skip cache entirely).
|
||||||
// Static entities collect into _populateScratch on the first frame
|
// Static entities accumulate into _populateScratch across ALL
|
||||||
// they're visible, so the cache has fresh data for the next frame.
|
// their MeshRefs; the flush at next-entity-boundary (or
|
||||||
|
// end-of-loop) commits them as a single Populate call.
|
||||||
// Task 10 will add the cache-hit fast path that skips slow
|
// Task 10 will add the cache-hit fast path that skips slow
|
||||||
// classification when an entry already exists.
|
// classification when an entry already exists.
|
||||||
var collector = isAnimated ? null : _populateScratch;
|
var collector = isAnimated ? null : _populateScratch;
|
||||||
collector?.Clear();
|
|
||||||
|
|
||||||
bool drewAny = false;
|
bool drewAny = false;
|
||||||
if (renderData.IsSetup && renderData.SetupParts.Count > 0)
|
if (renderData.IsSetup && renderData.SetupParts.Count > 0)
|
||||||
|
|
@ -448,17 +476,29 @@ public sealed unsafe class WbDrawDispatcher : IDisposable
|
||||||
drewAny = true;
|
drewAny = true;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (collector is not null && collector.Count > 0)
|
// Track THIS entity for the next iteration's flush check. Only
|
||||||
|
// when collector is non-null (entity is static); animated entities
|
||||||
|
// leave the tracker null so we don't try to flush them.
|
||||||
|
if (collector is not null)
|
||||||
{
|
{
|
||||||
// Populate cache for static entity on cache-miss.
|
populateEntityId = entity.Id;
|
||||||
// Each entity classifies once at first visibility; subsequent
|
populateLandblockId = landblockId;
|
||||||
// frames will hit the fast path (added in Task 10).
|
|
||||||
_cache.Populate(entity.Id, landblockId, collector.ToArray());
|
|
||||||
}
|
}
|
||||||
|
|
||||||
if (diag && drewAny) _entitiesDrawn++;
|
if (diag && drewAny) _entitiesDrawn++;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Final flush: the last entity in _walkScratch has no "next iteration"
|
||||||
|
// to trigger the entity-change flush, so commit its accumulated batches
|
||||||
|
// here. No-op when the last entity was animated (populateEntityId stays
|
||||||
|
// null) or when no entities walked at all.
|
||||||
|
if (populateEntityId.HasValue && _populateScratch.Count > 0)
|
||||||
|
{
|
||||||
|
_cache.Populate(populateEntityId.Value, populateLandblockId, _populateScratch.ToArray());
|
||||||
|
_populateScratch.Clear();
|
||||||
|
populateEntityId = null;
|
||||||
|
}
|
||||||
|
|
||||||
// Nothing visible — skip the GL pass entirely.
|
// Nothing visible — skip the GL pass entirely.
|
||||||
if (anyVao == 0)
|
if (anyVao == 0)
|
||||||
{
|
{
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue