From f160f3fee19efafb087a6dc2a5c92222c2630f5e Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 25 Aug 2026 19:16:53 +0200 Subject: [PATCH] fix(ui): wait for private viewport mesh residency --- docs/ISSUES.md | 43 +-- .../PrivateEntityViewportRenderer.cs | 271 +++++++++++++++--- ...ateEntityViewportRendererResidencyTests.cs | 225 +++++++++++++++ 3 files changed, 476 insertions(+), 63 deletions(-) create mode 100644 tests/AcDream.App.Tests/Rendering/PrivateEntityViewportRendererResidencyTests.cs diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 24cec938..5bb2231a 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -26,8 +26,7 @@ What does NOT go here: ## #443 — Examination/paperdoll private viewport: doll appears only after a delay on first open (was: "renders nothing") -**Status:** OPEN (narrowed at the gate — intermittent first-open DELAY, not -a hard break; NOT gate-blocking, owner passed the Campaign AS gate with it). +**Status:** FIXED / OWNER-ACCEPTED 2026-08-25. **Component:** private entity viewports (examination clone, inventory paperdoll — shared `PrivateEntityViewportRenderer`). **Filed:** 2026-08-25, AS-GF1 gate-fix session. **Narrowed same day at the @@ -46,6 +45,20 @@ extras rows at small window heights) is the authored scroll-less clipped-list behavior AS-GF1 ruled retail-correct below, plus the clip line moving with resize — owner-accepted at the gate. +**Fix:** `PrivateEntityViewportRenderer.EntitySlot` now uses a two-phase +mesh-residency handoff. A replacement first acquires/pins its complete mesh +ownership set, remains pending while each drawable `MeshRef` crosses the +render-thread upload barrier, then atomically replaces the active entity and +texture-owner generation. While a replacement is pending the renderer keeps +the last completed viewport texture; on first open it publishes no texture so +the authored panel art remains visible instead of exposing a black-cleared +render target. The slot also detects GfxObj-id changes made in place by the +animated appraisal/chargen paths and supersedes stale pending owners without +leaking references. Focused paperdoll, appraisal, draw-order, synthetic-owner, +and new residency tests pass 30/30; the App hermetic lane passes 6,358/6,358. +The owner then live-verified repeated inventory and monster/player assessment +opens against the local ACE test server: "Good. works." + Owner report at the Campaign AS connected gate: the animated 3-D paperdoll in the examination window (LayoutDesc `0x2100006B` element `0x10000148`) worked correctly at baseline `974fe88a` (praised the same session) and was @@ -107,23 +120,15 @@ Campaign AS diff for either symptom:** presenter at all) are byte-for-byte UNCHANGED across the whole `974fe88a..87e98395` window (`git log -p` for both files is empty). -**Conclusion:** the trigger is one of `TryGetVisibleTarget`'s -`CurrentObjectId` check or `TrySynchronize`'s `LiveEntityRuntime. -TryGetWorldEntity`/`MeshRefs.Count` check, in code nothing in Campaign AS -touches — meaning either a pre-existing, previously-latent condition this -gate round happened to trigger, or a live/timing condition a hermetic test -cannot reproduce (no live entity, no live wire exchange). - -**Probe added this session** (temporary — delete with the real fix): -`ACDREAM_PROBE_CREATURE_APPRAISAL_VIEWPORT=1` — -`CreatureAppraisalViewportDiagnostics` in `CreatureAppraisalPresentation.cs` -logs `[AS-GF1-PROBE] creature-appraisal viewport: ` on every REASON -TRANSITION (not every frame) from both `TryGetVisibleTarget` and -`RetailCreatureAppraisalCloneFactory.TrySynchronize`. Next step: relaunch -with the flag set, examine a player, and read which one of the five -possible reasons (`no ActiveView`, `windowFrame hidden`, `viewport hidden`, -`no CurrentObjectId`, `entity not found`, `no MeshRefs`) fires — that -pinpoints the real fix. +**Conclusion after live recurrence:** the temporary probe ruled out every +higher-level gate: the target, clone, 34 MeshRefs, camera and nonzero texture +handle were all healthy while the pane was visibly empty. The shared slot had +published the clone immediately after `IncrementRefCount`, but that operation +only schedules asynchronous preparation/upload. The private pass then cleared +its target to black while `WbDrawDispatcher` skipped every nonresident mesh. +Residency/backlog timing explains both intermittent first-open delay and the +same symptom across inventory and monster/player examination. The probe was +deleted in `ddbd7e40` per the probe-dies rule; no diagnostic flag remains. ## #442 — Flake: DirectionalShadowCasterFrameTests.WarmDenseChangedFrames_AllocateZeroAndReadNoSceneRecords fails intermittently under full parallel suite load diff --git a/src/AcDream.App/Rendering/PrivateEntityViewportRenderer.cs b/src/AcDream.App/Rendering/PrivateEntityViewportRenderer.cs index f293218b..a5bc2e6a 100644 --- a/src/AcDream.App/Rendering/PrivateEntityViewportRenderer.cs +++ b/src/AcDream.App/Rendering/PrivateEntityViewportRenderer.cs @@ -99,6 +99,7 @@ internal sealed class PrivateEntityViewportRenderer : private GpuTextureSlot _slot = GpuTextureSlot.Unassigned; private int _fbW; private int _fbH; + private bool _hasRenderedScene; public PrivateEntityViewportRenderer( IWorldPassScope scope, @@ -197,6 +198,23 @@ internal sealed class PrivateEntityViewportRenderer : /// public uint Render(int width, int height) { + // #443: acquiring a synthetic mesh reference only schedules CPU + // preparation/GPU upload; it does not make the mesh drawable. Keep the + // last completed private scene intact until every drawable mesh in the + // replacement has crossed that upload barrier. On first open there is + // no completed scene, so return zero and let the authored panel art + // show through instead of publishing a freshly-cleared black target. + bool mainReady = _mainSlot.PrepareForDraw(); + bool backdropReady = _backdropSlot?.PrepareForDraw() ?? true; + if (!mainReady || !backdropReady) + { + return _mainSlot.Entity is not null + && _hasRenderedScene + && _slot.IsAssigned + ? UiTextureTableHandle.FromSlot(_slot) + : 0u; + } + WorldEntity? entity = _mainSlot.Entity; if (entity is null || entity.MeshRefs.Count == 0 || width <= 0 || height <= 0) return 0u; @@ -256,6 +274,7 @@ internal sealed class PrivateEntityViewportRenderer : neverCullLandblockId: PrivateLandblockId, visibleCellIds: null, animatedEntityIds: _animatedIds); + _hasRenderedScene = true; return UiTextureTableHandle.FromSlot(_slot); } @@ -367,6 +386,7 @@ internal sealed class PrivateEntityViewportRenderer : _target = null; _fbW = 0; _fbH = 0; + _hasRenderedScene = false; } public void Dispose() @@ -405,31 +425,88 @@ internal sealed class PrivateEntityViewportRenderer : } } - private static IEnumerable CollectMeshIds(WorldEntity entity) - { - for (int i = 0; i < entity.MeshRefs.Count; i++) - yield return entity.MeshRefs[i].GfxObjId; - for (int i = 0; i < entity.PartOverrides.Count; i++) - yield return entity.PartOverrides[i].GfxObjId; - } - /// - /// One private entity's own mesh-reference/texture-owner lifetime, - /// independent of any other slot on the same renderer. Factored out at - /// Campaign CC gate round 1 Batch D so the chargen backdrop entity gets - /// the EXACT SAME acquire/replace/retire behavior the main entity already - /// had — a single-owner class shared by both slots rather than a second, - /// hand-duplicated copy of 's - /// pre-Batch-D body. + /// One private entity's mesh-reference/texture-owner lifetime, independent + /// of every other slot on the renderer. Publication is two-phase: a candidate owns its mesh + /// references while preparation/upload runs, but does not replace the + /// drawable entity until all of its actual + /// are resident. Kept internal so #443's lifetime/readiness behavior can + /// be pinned without constructing a live GPU device. /// - private sealed class EntitySlot + internal sealed class EntitySlot { + private sealed class MeshSnapshot + { + private readonly ulong[] _ownedIds; + private readonly int _drawMeshCount; + + private MeshSnapshot(ulong[] ownedIds, int drawMeshCount) + { + _ownedIds = ownedIds; + _drawMeshCount = drawMeshCount; + } + + public IReadOnlyList OwnedIds => _ownedIds; + + public static MeshSnapshot Capture(WorldEntity entity) + { + int drawMeshCount = entity.MeshRefs.Count; + var ids = new ulong[drawMeshCount + entity.PartOverrides.Count]; + for (int i = 0; i < drawMeshCount; i++) + ids[i] = entity.MeshRefs[i].GfxObjId; + for (int i = 0; i < entity.PartOverrides.Count; i++) + ids[drawMeshCount + i] = entity.PartOverrides[i].GfxObjId; + return new MeshSnapshot(ids, drawMeshCount); + } + + /// + /// Residency identity deliberately excludes part transforms, + /// palette ranges and surface overrides: changing those does not + /// require another mesh upload. A different entity instance still + /// stages a replacement so its fixed texture owner is refreshed. + /// + public bool Matches(WorldEntity entity) + { + if (entity.MeshRefs.Count != _drawMeshCount + || entity.PartOverrides.Count != _ownedIds.Length - _drawMeshCount) + { + return false; + } + + for (int i = 0; i < _drawMeshCount; i++) + if (entity.MeshRefs[i].GfxObjId != _ownedIds[i]) + return false; + for (int i = 0; i < entity.PartOverrides.Count; i++) + if (entity.PartOverrides[i].GfxObjId != _ownedIds[_drawMeshCount + i]) + return false; + return true; + } + + public bool AreDrawMeshesReady(IWbMeshAdapter adapter) + { + for (int i = 0; i < _drawMeshCount; i++) + { + ulong id = _ownedIds[i]; + if (id == 0u || !adapter.IsRenderDataReady(id)) + return false; + } + return true; + } + } + + private sealed record PendingEntity( + WorldEntity Entity, + MeshSnapshot Snapshot, + SyntheticEntityMeshReferenceOwner MeshReferences); + private readonly IWbMeshAdapter _meshAdapter; private readonly FixedEntityTextureOwnerLease _textureOwnerLease; private readonly string _diagnosticName; private readonly List _retiringMeshReferences = []; private SyntheticEntityMeshReferenceOwner? _meshReferences; + private MeshSnapshot? _meshSnapshot; + private PendingEntity? _pending; public EntitySlot( IWbMeshAdapter meshAdapter, @@ -444,75 +521,181 @@ internal sealed class PrivateEntityViewportRenderer : public WorldEntity? Entity { get; private set; } + internal bool HasPending => _pending is not null; + public void Set(WorldEntity? entity) { ReleaseRetiringMeshReferences(); - if (ReferenceEquals(Entity, entity)) + if (entity is null) + { + Clear(); + return; + } + + // The common animated appraisal path reuses one clone object. Let + // PrepareForDraw perform its allocation-free mesh-id comparison; + // if no candidate is pending, reference equality is enough here. + if (ReferenceEquals(Entity, entity) && _pending is null) return; - SyntheticEntityMeshReferenceOwner? replacement = null; - if (entity is not null) + if (ReferenceEquals(Entity, entity) + && _meshSnapshot?.Matches(entity) == true) { - replacement = new SyntheticEntityMeshReferenceOwner( - _meshAdapter, - CollectMeshIds(entity)); - replacement.Acquire(); + ReleasePending(); + return; } - SyntheticEntityMeshReferenceOwner? previous = _meshReferences; + if (_pending is { } pending + && ReferenceEquals(pending.Entity, entity) + && pending.Snapshot.Matches(entity)) + { + return; + } + + Stage(entity); + } + + /// + /// Refreshes an in-place MeshRefs mutation, re-arms missing uploads, + /// and atomically promotes a fully drawable candidate. False tells the + /// renderer to preserve its last completed render target this frame. + /// + public bool PrepareForDraw() + { + ReleaseRetiringMeshReferences(); + + if (_pending is { } pending) + { + if (!pending.Snapshot.Matches(pending.Entity)) + Stage(pending.Entity); + } + else if (Entity is { } current + && _meshSnapshot?.Matches(current) != true) + { + // Chargen animation and appraisal synchronization both mutate + // a retained WorldEntity in place. Detect a changed GfxObj set + // here even when the caller did not issue another Set call. + Stage(current); + } + + pending = _pending; + if (pending is null) + return true; + if (!pending.Snapshot.AreDrawMeshesReady(_meshAdapter)) + return false; + + PromotePending(pending); + return true; + } + + private void Stage(WorldEntity entity) + { + MeshSnapshot snapshot = MeshSnapshot.Capture(entity); + var replacement = new SyntheticEntityMeshReferenceOwner( + _meshAdapter, + snapshot.OwnedIds); try { - _textureOwnerLease.Replace(entity is not null); + replacement.Acquire(); } - catch (Exception textureFailure) + catch (Exception acquisitionFailure) { - if (replacement is null) - throw; - try { replacement.Dispose(); } catch (Exception rollbackFailure) { + _retiringMeshReferences.Add(replacement); throw new AggregateException( - $"The {_diagnosticName} texture-owner replacement failed " - + "and the replacement mesh-owner rollback did not converge.", - textureFailure, + $"The {_diagnosticName} candidate mesh acquisition failed " + + "and its rollback did not converge.", + acquisitionFailure, rollbackFailure); } System.Runtime.ExceptionServices.ExceptionDispatchInfo - .Capture(textureFailure) + .Capture(acquisitionFailure) .Throw(); + throw new InvalidOperationException("Unreachable exception dispatch path."); } - _meshReferences = replacement; - Entity = entity; + PendingEntity? previous = _pending; + _pending = new PendingEntity(entity, snapshot, replacement); + if (previous is not null) + Retire(previous.MeshReferences); + } + + private void PromotePending(PendingEntity pending) + { + // Release the fixed texture owner's prior composites only at the + // same atomic edge that publishes the new mesh set. If release + // fails, the candidate remains pending and can retry intact. + _textureOwnerLease.Replace(hasReplacement: true); + + SyntheticEntityMeshReferenceOwner? previous = _meshReferences; + _meshReferences = pending.MeshReferences; + _meshSnapshot = pending.Snapshot; + Entity = pending.Entity; + _pending = null; if (previous is not null) + Retire(previous); + } + + private void Clear() + { + if (Entity is null && _pending is null) + return; + + ReleasePending(); + _textureOwnerLease.Replace(hasReplacement: false); + + SyntheticEntityMeshReferenceOwner? previous = _meshReferences; + _meshReferences = null; + _meshSnapshot = null; + Entity = null; + if (previous is not null) + Retire(previous); + } + + private void ReleasePending() + { + PendingEntity? pending = _pending; + if (pending is null) + return; + _pending = null; + Retire(pending.MeshReferences); + } + + private void Retire(SyntheticEntityMeshReferenceOwner owner) + { + try { - try - { - previous.Dispose(); - } - catch - { - _retiringMeshReferences.Add(previous); - throw; - } + owner.Dispose(); + } + catch + { + _retiringMeshReferences.Add(owner); + throw; } } public void Dispose() { Entity = null; + _meshSnapshot = null; if (_meshReferences is { } current) { _meshReferences = null; _retiringMeshReferences.Add(current); } + if (_pending is { } pending) + { + _pending = null; + _retiringMeshReferences.Add(pending.MeshReferences); + } List? failures = null; try diff --git a/tests/AcDream.App.Tests/Rendering/PrivateEntityViewportRendererResidencyTests.cs b/tests/AcDream.App.Tests/Rendering/PrivateEntityViewportRendererResidencyTests.cs new file mode 100644 index 00000000..afc837e1 --- /dev/null +++ b/tests/AcDream.App.Tests/Rendering/PrivateEntityViewportRendererResidencyTests.cs @@ -0,0 +1,225 @@ +using System.Numerics; +using AcDream.App.Rendering; +using AcDream.App.Rendering.Wb; +using AcDream.Core.World; + +namespace AcDream.App.Tests.Rendering; + +/// +/// #443: private paperdoll/examination entities must cross the mesh-upload +/// barrier before the shared viewport publishes them. These tests exercise +/// the renderer's slot directly, without requiring a GPU device. +/// +public sealed class PrivateEntityViewportRendererResidencyTests +{ + [Fact] + public void FirstEntityIsNotPublishedUntilEveryDrawableMeshIsReady() + { + const ulong first = 0x0100_0001u; + const ulong second = 0x0100_0002u; + var adapter = new RecordingMeshAdapter { ReadyIds = { first } }; + var textures = new RecordingTextureLifetime(); + var slot = CreateSlot(adapter, textures); + WorldEntity entity = Entity(first, second); + + slot.Set(entity); + + Assert.True(slot.HasPending); + Assert.Null(slot.Entity); + Assert.False(slot.PrepareForDraw()); + Assert.Null(slot.Entity); + Assert.Equal(1, adapter.ReferenceCount(first)); + Assert.Equal(1, adapter.ReferenceCount(second)); + + adapter.ReadyIds.Add(second); + + Assert.True(slot.PrepareForDraw()); + Assert.False(slot.HasPending); + Assert.Same(entity, slot.Entity); + + slot.Dispose(); + Assert.Equal(0, adapter.TotalReferences); + Assert.Equal(1, textures.ReleaseCount); + } + + [Fact] + public void ReplacementKeepsActiveEntityUntilCandidateIsReady() + { + const ulong first = 0x0100_0011u; + const ulong second = 0x0100_0012u; + var adapter = new RecordingMeshAdapter { ReadyIds = { first } }; + var textures = new RecordingTextureLifetime(); + var slot = CreateSlot(adapter, textures); + WorldEntity active = Entity(first); + WorldEntity candidate = Entity(second); + slot.Set(active); + Assert.True(slot.PrepareForDraw()); + + slot.Set(candidate); + + Assert.True(slot.HasPending); + Assert.Same(active, slot.Entity); + Assert.False(slot.PrepareForDraw()); + Assert.Same(active, slot.Entity); + Assert.Equal(1, adapter.ReferenceCount(first)); + Assert.Equal(1, adapter.ReferenceCount(second)); + + adapter.ReadyIds.Add(second); + + Assert.True(slot.PrepareForDraw()); + Assert.Same(candidate, slot.Entity); + Assert.Equal(0, adapter.ReferenceCount(first)); + Assert.Equal(1, adapter.ReferenceCount(second)); + Assert.Equal(1, textures.ReleaseCount); + + slot.Dispose(); + Assert.Equal(0, adapter.TotalReferences); + } + + [Fact] + public void InPlaceMeshChangeIsDetectedAtTheDrawBarrier() + { + const ulong first = 0x0100_0021u; + const ulong second = 0x0100_0022u; + var adapter = new RecordingMeshAdapter { ReadyIds = { first } }; + var slot = CreateSlot(adapter, new RecordingTextureLifetime()); + WorldEntity entity = Entity(first); + slot.Set(entity); + Assert.True(slot.PrepareForDraw()); + + // Appraisal synchronization and chargen animation can mutate the same + // retained clone without calling Set again. + entity.MeshRefs = [new MeshRef((uint)second, Matrix4x4.Identity)]; + + Assert.False(slot.PrepareForDraw()); + Assert.True(slot.HasPending); + Assert.Same(entity, slot.Entity); + Assert.Equal(1, adapter.ReferenceCount(first)); + Assert.Equal(1, adapter.ReferenceCount(second)); + + adapter.ReadyIds.Add(second); + + Assert.True(slot.PrepareForDraw()); + Assert.False(slot.HasPending); + Assert.Equal(0, adapter.ReferenceCount(first)); + Assert.Equal(1, adapter.ReferenceCount(second)); + + slot.Dispose(); + } + + [Fact] + public void NewerCandidateReleasesSupersededPendingOwner() + { + const ulong first = 0x0100_0031u; + const ulong second = 0x0100_0032u; + var adapter = new RecordingMeshAdapter(); + var slot = CreateSlot(adapter, new RecordingTextureLifetime()); + + slot.Set(Entity(first)); + slot.Set(Entity(second)); + + Assert.True(slot.HasPending); + Assert.Equal(0, adapter.ReferenceCount(first)); + Assert.Equal(1, adapter.ReferenceCount(second)); + + slot.Dispose(); + Assert.Equal(0, adapter.TotalReferences); + } + + [Fact] + public void UnresolvedPartOverrideDoesNotBlockResolvedDrawableMeshes() + { + const ulong drawable = 0x0100_0041u; + const ulong overrideId = 0x0100_0042u; + var adapter = new RecordingMeshAdapter { ReadyIds = { drawable } }; + var slot = CreateSlot(adapter, new RecordingTextureLifetime()); + WorldEntity entity = Entity( + [drawable], + [new PartOverride(3, (uint)overrideId)]); + + slot.Set(entity); + + Assert.True(slot.PrepareForDraw()); + Assert.Same(entity, slot.Entity); + Assert.Equal(1, adapter.ReferenceCount(overrideId)); + + slot.Dispose(); + Assert.Equal(0, adapter.TotalReferences); + } + + [Fact] + public void ClearReleasesBothActiveAndPendingOwners() + { + const ulong first = 0x0100_0051u; + const ulong second = 0x0100_0052u; + var adapter = new RecordingMeshAdapter { ReadyIds = { first } }; + var textures = new RecordingTextureLifetime(); + var slot = CreateSlot(adapter, textures); + slot.Set(Entity(first)); + Assert.True(slot.PrepareForDraw()); + slot.Set(Entity(second)); + + slot.Set(null); + + Assert.Null(slot.Entity); + Assert.False(slot.HasPending); + Assert.Equal(0, adapter.TotalReferences); + Assert.Equal(1, textures.ReleaseCount); + + slot.Dispose(); + Assert.Equal(1, textures.ReleaseCount); + } + + private static PrivateEntityViewportRenderer.EntitySlot CreateSlot( + IWbMeshAdapter adapter, + IEntityTextureLifetime textures) => + new(adapter, textures, ownerLocalId: 0xDA11_D012u, "#443 test viewport"); + + private static WorldEntity Entity(params ulong[] meshIds) => + Entity(meshIds, []); + + private static WorldEntity Entity( + IReadOnlyList meshIds, + IReadOnlyList partOverrides) => new() + { + Id = 0xDA11_D012u, + ServerGuid = 0xDA11_D011u, + SourceGfxObjOrSetupId = 0x0200_0001u, + Position = Vector3.Zero, + Rotation = Quaternion.Identity, + MeshRefs = meshIds + .Select(static id => new MeshRef((uint)id, Matrix4x4.Identity)) + .ToArray(), + PartOverrides = partOverrides, + }; + + private sealed class RecordingTextureLifetime : IEntityTextureLifetime + { + public int ReleaseCount { get; private set; } + + public void ReleaseOwner(uint localEntityId) => ReleaseCount++; + } + + private sealed class RecordingMeshAdapter : IWbMeshAdapter + { + private readonly Dictionary _references = []; + + public HashSet ReadyIds { get; } = []; + public int TotalReferences => _references.Values.Sum(); + + public int ReferenceCount(ulong id) => _references.GetValueOrDefault(id); + + public bool IsRenderDataReady(ulong id) => ReadyIds.Contains(id); + + public void IncrementRefCount(ulong id) => + _references[id] = ReferenceCount(id) + 1; + + public void DecrementRefCount(ulong id) + { + int current = ReferenceCount(id); + if (current <= 0) + throw new InvalidOperationException("reference underflow"); + _references[id] = current - 1; + } + } +}