From f2d7562c867a4c6c96bcf935b17cb184e8e4e994 Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 6 Sep 2026 17:09:46 +0200 Subject: [PATCH] =?UTF-8?q?chore(plugin-ui):=20review=20cleanup=20?= =?UTF-8?q?=E2=80=94=20hermetic=20memo=20tests,=20shelf=20button=20anchors?= =?UTF-8?q?,=20outline=20pin,=20bounded=20miss=20cache;=20file=20#486/#487?= =?UTF-8?q?;=20correct=20#461?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Split the two hermetic RetailMarkupIconResolver memoization tests (and their counting fakes) out of the Lane=InstalledDat class into a new untagged RetailMarkupIconResolverMemoizationTests.cs so CI's portable filter (Lane!=InstalledDat) actually runs them. - PluginSidePanel: move the entry button's Anchors = AnchorEdges.None from the Add() call site into PluginShelfButton's own constructor (same comment carried over) so a second construction path cannot miss it. - UiRectOutlinePainterOrderTests: assert the back panel's border segment carries exactly 4 quads (24 vertices, FloatsPerVertex each) so a partial outline cannot pass the painter-order check. - RetailMarkupIconResolver: document the type as UI-thread-only (every caller is a draw-time icon source) and bound the MISS cache to 256 entries with FIFO eviction — HIT entries stay unbounded (bounded by the DAT's own surface count already). New test proves the 257th distinct miss evicts the first (re-probe count rises); verified failing first against the un-bounded code (Expected 258, Actual 257) before restoring the fix. - docs/plugin-ui-markup.md: split the icon-binding row's failure mode into Build-time (missing property only — the binder never checks CLR type) vs. draw-time (a resolved value that cannot convert to a number throws from the draw, not from Build). - docs/ISSUES.md: filed #486 (credits picture scroll frozen by the per-draw anchor pass) and #487 (radar compass tokens candidate, same mechanism, unconfirmed); corrected #461's causality — the graceful logout/reveal-cancel log lines are printed by LiveSessionController.Tick's catch -> StopAfterFailure -> StopCore AFTER the motion-update exception, then it rethrows, so the logout is a consequence of the crash, not its cause; real chain is the #462 stalled login-reveal materialization leaving PlayerMovementController in RuntimeOwnedDormant outside its SetPosition ground phase when an inbound 0xF74C arrives. - Plan doc: recorded the three fix-round commits' verdicts (all PASS) and the Smoke-plugin cleanup commit SHA in the Review ledger, plus a pointer to the two newly filed issues. Verified: dotnet build AcDream.slnx -c Release (0/0), targeted filter 85/0/0, full App suite 7364 passed / 97 skipped / 36 failed (36 pre-existing InstalledDat/Manual/Linux-only failures, unchanged by name from baseline; net +1 passed test from the new eviction test). Co-Authored-By: Claude Fable 5.1 --- docs/ISSUES.md | 113 ++++++++- .../2026-09-06-plugin-shelf-and-dat-icons.md | 11 + docs/plugin-ui-markup.md | 4 +- src/AcDream.App/UI/IMarkupIconResolver.cs | 56 ++++- src/AcDream.App/UI/PluginSidePanel.cs | 16 +- ...tailMarkupIconResolverInstalledDatTests.cs | 140 ----------- ...etailMarkupIconResolverMemoizationTests.cs | 221 ++++++++++++++++++ .../UI/UiRectOutlinePainterOrderTests.cs | 13 ++ 8 files changed, 416 insertions(+), 158 deletions(-) create mode 100644 tests/AcDream.App.Tests/UI/RetailMarkupIconResolverMemoizationTests.cs diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 7f375347..387440cd 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -39,6 +39,65 @@ confirmed closed by the owner, 11 need a focused live gate, and 43 are safe to remain closed. See [`docs/research/2026-08-28-owner-closed-issue-validity-audit.md`](research/2026-08-28-owner-closed-issue-validity-audit.md). +## #486 — Credits picture scroll is frozen by the per-draw anchor pass + +**Status:** OPEN — found 2026-09-06 by the Opus review of `47eb2d575` (the +plugin-shelf anchor fix), while confirming no other controller shares the +same mechanism. +**Severity:** LOW (credits screen only) +**Component:** `src/AcDream.App/UI/Layout/CreditsUiController.cs` + +**Description.** `AddPicture` (~331-345) creates each scrolling `UiPanel` +picture with the DEFAULT `Left|Top` anchors (never `AnchorEdges.None`), and +`ScrollPictures` (~293-297) writes `picture.Top` every tick to move it up the +screen. `UiElement.ApplyAnchor` (`UiElement.cs` ~829-856) runs for every +anchored child on every draw: it captures the Left/Top/Width/Height margins +ONCE on the first draw, then re-applies that frozen snapshot on every later +draw — silently overwriting whatever `ScrollPictures` had just written. After +the first draw, the picture's position can no longer actually change, so the +credits scroll freezes. + +`_textArea.Top` (~208-209) has the same write pattern but a different +mechanism gap: `_textArea` is the IMPORTED-layout flavour (its +`UiElement.LayoutPolicy` is set, from `ImportedLayout`/`LayoutImporter`), so +`ApplyAnchor` takes the `LayoutPolicy.Apply` branch instead of the anchor- +margin capture — but nothing ever calls `ResetAnchorCapture()`/ +`UiLayoutPolicy.Rebase` after `ScrollPictures` writes `Top`, so the imported +layout's own baseline goes stale the same way. + +**Fix direction:** `Anchors = AnchorEdges.None` on `AddPicture`'s +programmatically-repositioned children (the controller is their sole layout +owner, exactly the reasoning `PluginSidePanel` already documents for its own +runtime-positioned children); `ResetAnchorCapture()`/`Rebase` on `_textArea` +after each deliberate `Top` write for the imported-layout element. Precedent: +`PluginSidePanel` (commit `47eb2d575`), `UiItemList.cs:170-172`, +`MapPageController.cs:235-249` (the same fix already landed for other +runtime-repositioned imported/programmatic elements). + +## #487 — Radar compass tokens may be pinned by the anchor pass (candidate) + +**Status:** OPEN — CANDIDATE, found 2026-09-06 by the Opus review of +`47eb2d575`, not confirmed by a live gate. +**Severity:** LOW +**Component:** `src/AcDream.App/UI/Layout/RadarController.cs` + +**Description.** `ApplyPresentation` (~130-144) repositions the four +imported compass-letter tokens (`token.Element.Left`/`.Top`) on every +heading change, and `CreateToken` (~185-194) never marks them as runtime- +positioned — same imported-layout (`LayoutPolicy`) shape as +`MapPageController.PrepareIcon`'s F1 finding, which needed the same fix. + +**Symptom if real:** compass letters (N/E/S/W or similar) that never +rotate with the player's heading, staying pinned at their first-draw +position — the same class of bug as #486, just on the radar's imported +tokens instead of the credits screen's programmatic pictures. + +**Not established:** whether `ApplyAnchor`'s snapshot-freeze actually bites +here — it depends on whether these tokens' `LayoutPolicy`/anchor state ends +up captured before `ApplyPresentation`'s first write, which needs the +owner's eyes on a live heading change to confirm. Filed as a candidate +rather than assumed broken. + ## #485 — Gitea portable CI races console capture and selects Vulkan-only tests **Status:** DONE — correction implemented and locally verified, 2026-09-06; @@ -732,11 +791,55 @@ movement controller cannot be mutated.` at `PlayerMovementController.EnsureConfi then the crash — a `0xF74C`/`0xF625`-class motion update for the local player arrived after the controller was retired by the logout, and the inbound route still forwards it to the controller. -**Fix direction:** the inbound motion route must consult the controller's lifetime (the same -sealed/retired state `EnsureConfigurationMutable` throws on) and drop local-player motion once the -session is logging out — the J5.4/J5.7 terminal ledger owns that state; retail discards inbound -movement for a character that has left the world. Add the ordered-teardown test: logout confirmed → -a late `UpdateMotion` for the local player → no throw, no mutation. +**Corrected root cause (2026-09-06).** The original "logout → crash" framing above has the causality +backwards. Verified against `.claude/worktrees/peaceful-blackburn-5333f0/logs/selfgate-20260903-165745-g3c/` +(`client.log`, `client.err.log`) and `tools/overhaul-selfgate/route-g3c.txt` in that same worktree: +`LiveSessionController.Tick` (`LiveSessionController.cs:715-751`) runs `_operations.Tick(scope.Session)` +inside a `try` at line 730; THAT call is what throws (the `WorldSession.ProcessDatagram` → `OnMotion` → +`EnsureConfigurationMutable` chain above). The `catch (Exception tickError)` at line 735 calls +`StopAfterFailure(tickError)` (line 737), which runs `StopCore()` (line 1001) — `StopCore` is what +prints `[session] graceful logout requested`/`confirmed` (`WorldSession.cs:3759/3761`) and cancels the +Login reveal — and only THEN does `StopAfterFailure` rethrow, unwinding out through `GameWindow.OnUpdate` +to the unhandled-exception crash. So the log's `[session] graceful logout requested/confirmed` and +`[world-reveal] event=cancel` lines are a CONSEQUENCE of the already-thrown exception (the crash-recovery +path's own teardown attempt), not its trigger — nothing about logging out caused this crash. + +The real chain: the login reveal into `0xA9B40176` stalled (#462: `ready=True materialized=False` for +180 s — `client.log:204-205` never advance to `materialized=True`). The local player's +`PlayerMovementController` therefore never left `PlayerMovementControllerPublicationLifecycle.RuntimeOwnedDormant` +(`PlayerMovementController.cs:135`) — the one path that clears it, +`RuntimeLocalPlayerPhysicsPublicationState`'s dormant-activation dispatch +(`RuntimeSetPositionState.TryApplyDormantLocalActivationCommit`, bracketed by +`PlayerMovementController.BeginDormantSetPositionGroundPhase`/`EndDormantSetPositionGroundPhase` — +`RuntimeLocalPlayerPhysicsPublicationState.cs:590-625`), never ran because it is gated on the same +reveal materialization that never completed. The route's `wait world-visible` step then timed out and +the script's next verb (`command /teleloc ...`) made ACE send a `0xF74C` for the player. That reached +`LiveEntityNetworkUpdateController.OnMotion` → `PlayerMovementController.SetLastMoveWasAutonomous` → +`EnsureConfigurationMutable` (`PlayerMovementController.cs:1038-1049`), whose predicate allows mutation +only for `StandalonePublished`/`CandidatePreparing`/`RuntimePublished`, OR `RuntimeOwnedDormant` while +`_dormantSetPositionGroundPhase` is true (i.e. actively inside that one bracketed activation window) — +by elimination, a `RuntimeOwnedDormant` controller OUTSIDE that window is the only lifecycle state a +live `_controller` can be in here, and it throws. + +Retail's `CPhysics::SetObjectMovement @0x00509690` stores the autonomous byte and calls +`unpack_movement` as soon as the object exists — there is no dormant window at all. acdream's accepted +equivalent for exactly this "motion arrived before the entity finished materializing" shape already +exists for every OTHER entity: `RuntimeEntityObjectLifetime.TryApplyMotion` +(`RuntimeEntityObjectLifetime.cs:1568-1606`) checks `TryGetPendingInitialResidence` for the guid and, if +the entity still has one, enqueues the motion as a deferred continuation +(`EnqueueDormant(..., RuntimeInitialCreateContinuationKind.Movement, ...)`) instead of applying it +immediately or throwing. The dormant LOCAL player bypasses this retention entirely: its motion is routed +through the player-specific `LiveEntityNetworkUpdateController.OnMotion` → `PlayerMovementController` +path, not through the guid-keyed `RuntimeEntityObjectLifetime.TryApplyMotion` retention every other +entity already gets. + +**Fix direction:** route an accepted local-player motion that arrives while the controller is dormant +into that SAME kind of retention (replayed once `TryApplyDormantLocalActivationCommit` actually +activates the controller), never a drop guard and never a silent no-op — a workaround here would just +trade a crash for silently losing a real wire motion. Test: dormant controller + inbound player +`0xF74C` → no throw, and the motion is applied after activation completes (not dropped). This defect is +reachable in ordinary play only through the #462 stalled-materialization window, so fix #462 alongside +it — without #462, the dormant window this bug lives in should not exist in the first place. ## #460 — Shutdown hangs windowless at 100 % of one core after a close request that follows an aborted automation script (Nanto pose) diff --git a/docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md b/docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md index b83e748d..81ae919b 100644 --- a/docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md +++ b/docs/plans/2026-09-06-plugin-shelf-and-dat-icons.md @@ -303,3 +303,14 @@ all four places, list icon column aligned with rows. same commit; see `chore(plugins): remove the Smoke gate plugin; MossTank shelf icon 0x06002C41`. MossTank's own panels defaulting to `StartVisible = true` remains open — same look, still out of scope here. +- **Fix-round commits + closing cleanup (2026-09-06):** `47eb2d575` (shelf + children must not anchor — PASS, connected gate confirmed the `<`/`>` + toggle survives collapse/reflow), `761a7519f` (retained-UI rect outlines + composite in painter order — PASS, connected gate confirmed the MossTank + border no longer draws over the inventory paperdoll), `ce05c4fb0` (Slice B + residuals — shelf icon sink without magenta, validated icon bindings, + negative ids, memoized DID resolves — PASS; its one SHOULD-FIX carryover, + the memoization tests sitting in a `Lane=InstalledDat` class where CI never + ran them, is this session's cleanup item 1), and the cleanup commit + `ece210418` (Smoke gate plugin removal, referenced by message above). + Latent anchor-pass owners filed by this cleanup pass: #486, #487. diff --git a/docs/plugin-ui-markup.md b/docs/plugin-ui-markup.md index 055d048e..77030479 100644 --- a/docs/plugin-ui-markup.md +++ b/docs/plugin-ui-markup.md @@ -70,7 +70,7 @@ check those four against the markup by eye. | `list items`, `menu items` | Throws | `IEnumerable` | | `list colors` | **Silent** if omitted (no color override); throws if present but mistyped | `IEnumerable` **or** `IEnumerable` (shared `BindUintList`) | | `list icons` (Slice B) | Throws if present but mistyped; omitting it entirely means no icon column at all. A negative `int` element is **silent**: it maps to `0u` (no icon for that row), matching the scalar `did`/`spell`/`item` row above | `IEnumerable` **or** `IEnumerable` | -| ``/`