From f928e66119f561528142e12d60ebb953c4df7ca1 Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 10 May 2026 23:56:58 +0200 Subject: [PATCH] fix(render #53): incomplete-entity flag must persist across same-entity tuples MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User reported (cache enabled, post-c55acdc): drudge statue renders fully but many trees are missing branches. Cache-disabled A/B run rendered trees correctly. So the bug is in the cache wiring. Root cause: c55acdc's `currentEntityIncomplete = false;` reset fired UNCONDITIONALLY at the top of every iteration. For a tree with MeshRefs [trunk valid, branches null, leaves valid], the tuple sequence is: - tuple 0 (trunk): no flag set - tuple 1 (branches): TryGetRenderData null → set flag, continue - tuple 2 (leaves): unconditional reset → flag = false (WRONG) - end-of-entity: flag is false, scratch has trunk+leaves batches but NOT branches → MaybeFlushOnEntityChange populates a PARTIAL cache entry - cache hits forever serve trunk+leaves with no branches Drudge happened to render correctly because its missing MeshRef was at the END of its MeshRefs list — no later tuple reset the flag. Adds a per-tuple `prevTupleEntityId` tracker for entity-change detection, updated UNCONDITIONALLY at end of each tuple (including tuples that skip via null renderData). The flag-reset block now fires ONLY on actual entity change. Within the same entity, the flag accumulates across tuples. Also includes ACDREAM_DISABLE_TIER1_CACHE=1 diagnostic env-var added inline (was stashed previously) for future A/B testing. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Rendering/Wb/WbDrawDispatcher.cs | 37 ++++++++++++++++--- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs b/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs index 5ee496b..d0dbd82 100644 --- a/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs +++ b/src/AcDream.App/Rendering/Wb/WbDrawDispatcher.cs @@ -74,6 +74,11 @@ public sealed unsafe class WbDrawDispatcher : IDisposable // miss-populate / hit-fast-path through the loop. private readonly EntityClassificationCache _cache; + // ACDREAM_DISABLE_TIER1_CACHE=1 A/B diagnostic — forces every static + // entity through the slow path. Read once in ctor. + private readonly bool _tier1CacheDisabled = + string.Equals(Environment.GetEnvironmentVariable("ACDREAM_DISABLE_TIER1_CACHE"), "1", StringComparison.Ordinal); + /// /// A.5 T22.5: gate for GL_SAMPLE_ALPHA_TO_COVERAGE around the opaque pass. /// Default true matches T20 behavior. Set false for Low/Medium presets that @@ -423,6 +428,14 @@ public sealed unsafe class WbDrawDispatcher : IDisposable // some parts missing permanently. Reset on entity change. bool currentEntityIncomplete = false; + // Per-tuple entity tracker used purely for entity-change detection. + // Updated UNCONDITIONALLY at end of every tuple (including tuples that + // skip via null renderData), so the flag-reset block below correctly + // distinguishes "new entity" from "same entity, different tuple." + // populateEntityId can't be used for this because it's only set after + // a successful slow-path classification. + uint? prevTupleEntityId = null; + foreach (var (entity, partIdx, landblockId) in _walkScratch) { if (diag) _entitiesSeen++; @@ -454,12 +467,26 @@ public sealed unsafe class WbDrawDispatcher : IDisposable // the tracker so MaybeFlushOnEntityChange sees the cleaned state // and no-ops for this entity. Reset the incomplete flag for the // new entity so each one gets a fresh measurement. - if (populateEntityId.HasValue && populateEntityId.Value != entity.Id && currentEntityIncomplete) + // + // CRITICAL: the flag reset must fire ONLY on entity change, not + // every tuple. Resetting per-tuple within the same entity would + // undo a null-renderData flag set by a previous tuple of the same + // entity → if the missing MeshRef sits in the MIDDLE of the + // entity's MeshRefs list, a later valid tuple's reset would + // re-mark the entity "complete" and let partial data populate + // the cache. Trees with [trunk valid, branches null, leaves + // valid] hit this exactly — branches never recover. + bool isNewEntity = !prevTupleEntityId.HasValue || prevTupleEntityId.Value != entity.Id; + if (isNewEntity) { - _populateScratch.Clear(); - populateEntityId = null; + if (populateEntityId.HasValue && currentEntityIncomplete) + { + _populateScratch.Clear(); + populateEntityId = null; + } + currentEntityIncomplete = false; } - currentEntityIncomplete = false; + prevTupleEntityId = entity.Id; // Flush-on-entity-change: if the previous entity accumulated any // batches AND this iteration is for a different entity, populate @@ -489,7 +516,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, landblockId, out var cachedEntry)) + if (!isAnimated && !_tier1CacheDisabled && _cache.TryGet(entity.Id, landblockId, out var cachedEntry)) { ApplyCacheHit(cachedEntry!, entityWorld, AppendInstanceToGroup);