diff --git a/docs/architecture/retail-divergence-register.md b/docs/architecture/retail-divergence-register.md index 634a1850..4604f683 100644 --- a/docs/architecture/retail-divergence-register.md +++ b/docs/architecture/retail-divergence-register.md @@ -63,7 +63,7 @@ accepted-divergence entries (#96, #49, #50). --- -## 2. Adaptation (AD) — 70 active rows (AD-91 filed 2026-08-13 at the #390 port — the display-change clamp covers floating chats too, which retail leaves unclamped/strandable; AD-90 filed 2026-08-13 at the #389 fix round — retail's smartbox aspect runs through the `Render.AspectRatio` preference (`ComputeAspectForViewport @0x0054f150`), exactly raw w/h at its default, which is what acdream assumes; AD-89 RETIRED same-day 2026-08-13 — the SmartboxFOV port landed (#389): `RetailFieldOfView` + `CameraController.SetGameFov` now apply retail's `gameFOV/(aspect−0.1)` law with the 90°-degrees option semantics, and the invented 60° camera constants are deleted; AD-88 filed 2026-08-13 at the #385 dropdown fix — the vendor category dropdown keeps G5's fixed 6-row scrollable window although its authored popup ListBox is edge-docked, the condition that arms retail's `RecalculatePopupSize` size-to-content resize; classification UNCLEAR pending a retail side-by-side (ISSUES #386); AD-87 filed 2026-08-12 at Campaign FA slice FA6 — the allegiance-swear half of the two-bot headless gate is written+wired but `AllegianceGateEnabled=false` (disabled by default), unverified end-to-end over the wire because ACE returns nothing to the `0x001D` swear (ISSUES #384); the FELLOWSHIP two-session gate passed live and ships as FA6's automated proof; AD-86 filed 2026-08-12 at Campaign FA slice FA5, item 4 — ACE's deliberate zeroing of officers/officer titles/MOTD/MOTD-set-by/name-last-set-time/lock/approved-vassal/timeOnline/allegianceAge, dropped past acdream's own parse layer to match retail's own no-widget presentation; AD-85 filed 2026-08-12 at Campaign FA slice FA5 — the Allegiance page's numeric-only fields and its three local confirmation dialogs' unsubstituted-verbatim-or-bare-name text, the same unported `StringInfo` gap AD-81 filed for Fellowship; AD-84 filed 2026-08-12 at Campaign FA slice FA5 — the Swear button's missing "target is a player" gate, the same class as AD-83's Recruit-button gap; AD-83 filed 2026-08-12 at the Campaign FA slice FA4 fix round (mechanism MUST-FIX 5) — the Recruit button's missing "target is a player" gate, previously an inline comment not a row; AD-82 filed 2026-08-12 at the Campaign FA slice FA4 fix round (mechanism MUST-FIX 4/5) — the invented leader-tint/selection-tint colors, the name-text-only row click target, and the page-local (not generic-`UiTemplateListBox`) world→panel selection sync; AD-81 filed 2026-08-12 at Campaign FA slice FA4 — the fellowship roster/create-flow text-composition gap (unported `StringInfo` variable substitution + `ACCharGenData::FormatName`); AD-80 filed 2026-08-12 at Campaign FA slice FA4, D5 — the panel's retail-exact XP-share percentage display versus the currently-targeted ACE server's slightly different actual grant; AD-79 filed 2026-08-12 at Campaign FA slice FA3, D1 — the social panel's Friends/Squelch page action buttons (add/remove friend, appear offline, squelch add/remove/clear) are honest INERT, no wire implemented this campaign; AD-78 filed 2026-08-11 at Campaign OP's gate-2 follow-up (user-directed, verbatim "mark all options that are not implemented now, so I can clearly see what is not implemented") — the shared store-only-caption-dimming convention across the Character/Config option tabs and Configure Keyboard; AD-77 filed 2026-08-11 at the Campaign OP OP3 review-fix round — the client-wide floating-only `gmPanelUI` host divergence (retail also exposes a docked `0x21000017` host) the plan's §5 delegated to the OP3 dual review, scoped to every main panel not just Options; AD-76/AD-75/AD-74 filed 2026-08-11 at Campaign OP slice OP3 — the Options panel's Exit to Character Selection "behaves as Exit Game" adaptation (D6), the Urgent Assistance/Report Abuse dead-URL interface-text short-circuit (D5), and In-Game Help Files' asset-missing inert button (D5); AD-73 filed 2026-08-11 at the Campaign OP OP2 rework — `UiTabPanel`'s dormant-until-`ActivateTabBehavior()` activation model, replacing retail's unconditional per-instance tab-table wiring, so the four already-shipped Type-8 hosts keep their existing controller-owned switching without a double-driver race; AD-72 filed 2026-08-08 at the Slice 5.3 review corrections — `VendorPricing`'s double-precision narrowing versus retail's x87 extended precision, same class as AD-33; AD-65 RETIRED and AD-69 FILED 2026-08-07 at Campaign S S4 — the away-arm now snaps per retail @0x00509c50, while AD-66's byte-confirmed sibling landing is WITHHELD pending #341's measurement-anomaly apparatus, and AD-69 records the seam-frame dist gap the same pass discovered; AD-56 RESTORED 2026-08-07 — the a8a7d64b revert had collaterally DELETED it, the inverse of the AD-55 zombie it also created; its plumb-fall-freeze condition is live again since TS-4’s real retirement at Slice 2B; AD-55 RE-RETIRED 2026-08-07 — its 2026-07-30 retirement at 252e8068 was collaterally resurrected by the a8a7d64b revert of the unrelated TS-4 commit; the code kept the cos(10°) fix throughout; AD-68 filed 2026-08-07 at the #338 closure — the async-residency placeholder mover shape (0.4/0.4 steps + capsule) has no retail counterpart because retail loads synchronously; AD-67 filed 2026-08-07 at the #32 closeout — the narrowed `SetContactPlane` keeps its per-write `ContactPlaneCellId`, which retail writes only at `init_contact_plane`; AD-49 filed 2026-08-06 at the #334 fix — the BSP part-array flood runs its outdoor cell rectangle at seed time rather than only from retail’s residency-gated walk, keeping both registration floods on one residency rule; AD-64 filed 2026-08-05 at the C5b architecture review's D1 fix — AD-60's W2 wire-cell REACHABILITY decision is expressed once per host because the two hosts run parallel non-shared inbound routes; the committed VALUE is single-sourced at `RuntimeEntityObjectLifetime.CommitWireCellRebucket`, and unification is filed as #324; AD-60 CORRECTED the same day — its surviving-channel enumeration presented "the local force path, the missile arm" as exhaustive when the entire no-window host belonged in it; AD-1 RETIRED 2026-08-05, C5a deletion sweep — the legacy outdoor demote/restore lift this row described was `PhysicsEngine.Resolve`'s own body, deleted with zero production callers; AD-42 DELETED 2026-08-04, C4 route 3 — its last surviving citation, the headless portal-arrival resync's two-call Resolve/ResolvePlacement split, was retired by the canonical `RuntimeAcceptedPositionDriveController` portal arm; AD-2 amended same route with the deferred-place timing adaptation, the T8 tolerated-overwrite note, and the leash-anchor nuance; AD-63 filed 2026-08-04, cancelled-park presentation rollback — the rollback restores every presentation registration the park's Withdraw removed EXCEPT the player's selection, which is user intent rather than a projection; AD-62 filed 2026-08-03, C4 route 2 round 2 — a deferred ForcePosition retired without committing is not re-applied and its ack is not sent; AD-61 filed 2026-08-02, C3c review round 1 — the #270 settle compression now covers the local player; AD-59/AD-60 filed 2026-08-02, continuation-executor slice) +## 2. Adaptation (AD) — 71 active rows (AD-92 filed 2026-08-13 at the #376/#388 fix round — highest-refresh-for-WxH selection + refuse-and-log invalid fullscreen requests, versus retail's pass-through-and-error `ForceDisplayResolution`; AD-91 filed 2026-08-13 at the #390 port — the display-change clamp covers floating chats too, which retail leaves unclamped/strandable; AD-90 filed 2026-08-13 at the #389 fix round — retail's smartbox aspect runs through the `Render.AspectRatio` preference (`ComputeAspectForViewport @0x0054f150`), exactly raw w/h at its default, which is what acdream assumes; AD-89 RETIRED same-day 2026-08-13 — the SmartboxFOV port landed (#389): `RetailFieldOfView` + `CameraController.SetGameFov` now apply retail's `gameFOV/(aspect−0.1)` law with the 90°-degrees option semantics, and the invented 60° camera constants are deleted; AD-88 filed 2026-08-13 at the #385 dropdown fix — the vendor category dropdown keeps G5's fixed 6-row scrollable window although its authored popup ListBox is edge-docked, the condition that arms retail's `RecalculatePopupSize` size-to-content resize; classification UNCLEAR pending a retail side-by-side (ISSUES #386); AD-87 filed 2026-08-12 at Campaign FA slice FA6 — the allegiance-swear half of the two-bot headless gate is written+wired but `AllegianceGateEnabled=false` (disabled by default), unverified end-to-end over the wire because ACE returns nothing to the `0x001D` swear (ISSUES #384); the FELLOWSHIP two-session gate passed live and ships as FA6's automated proof; AD-86 filed 2026-08-12 at Campaign FA slice FA5, item 4 — ACE's deliberate zeroing of officers/officer titles/MOTD/MOTD-set-by/name-last-set-time/lock/approved-vassal/timeOnline/allegianceAge, dropped past acdream's own parse layer to match retail's own no-widget presentation; AD-85 filed 2026-08-12 at Campaign FA slice FA5 — the Allegiance page's numeric-only fields and its three local confirmation dialogs' unsubstituted-verbatim-or-bare-name text, the same unported `StringInfo` gap AD-81 filed for Fellowship; AD-84 filed 2026-08-12 at Campaign FA slice FA5 — the Swear button's missing "target is a player" gate, the same class as AD-83's Recruit-button gap; AD-83 filed 2026-08-12 at the Campaign FA slice FA4 fix round (mechanism MUST-FIX 5) — the Recruit button's missing "target is a player" gate, previously an inline comment not a row; AD-82 filed 2026-08-12 at the Campaign FA slice FA4 fix round (mechanism MUST-FIX 4/5) — the invented leader-tint/selection-tint colors, the name-text-only row click target, and the page-local (not generic-`UiTemplateListBox`) world→panel selection sync; AD-81 filed 2026-08-12 at Campaign FA slice FA4 — the fellowship roster/create-flow text-composition gap (unported `StringInfo` variable substitution + `ACCharGenData::FormatName`); AD-80 filed 2026-08-12 at Campaign FA slice FA4, D5 — the panel's retail-exact XP-share percentage display versus the currently-targeted ACE server's slightly different actual grant; AD-79 filed 2026-08-12 at Campaign FA slice FA3, D1 — the social panel's Friends/Squelch page action buttons (add/remove friend, appear offline, squelch add/remove/clear) are honest INERT, no wire implemented this campaign; AD-78 filed 2026-08-11 at Campaign OP's gate-2 follow-up (user-directed, verbatim "mark all options that are not implemented now, so I can clearly see what is not implemented") — the shared store-only-caption-dimming convention across the Character/Config option tabs and Configure Keyboard; AD-77 filed 2026-08-11 at the Campaign OP OP3 review-fix round — the client-wide floating-only `gmPanelUI` host divergence (retail also exposes a docked `0x21000017` host) the plan's §5 delegated to the OP3 dual review, scoped to every main panel not just Options; AD-76/AD-75/AD-74 filed 2026-08-11 at Campaign OP slice OP3 — the Options panel's Exit to Character Selection "behaves as Exit Game" adaptation (D6), the Urgent Assistance/Report Abuse dead-URL interface-text short-circuit (D5), and In-Game Help Files' asset-missing inert button (D5); AD-73 filed 2026-08-11 at the Campaign OP OP2 rework — `UiTabPanel`'s dormant-until-`ActivateTabBehavior()` activation model, replacing retail's unconditional per-instance tab-table wiring, so the four already-shipped Type-8 hosts keep their existing controller-owned switching without a double-driver race; AD-72 filed 2026-08-08 at the Slice 5.3 review corrections — `VendorPricing`'s double-precision narrowing versus retail's x87 extended precision, same class as AD-33; AD-65 RETIRED and AD-69 FILED 2026-08-07 at Campaign S S4 — the away-arm now snaps per retail @0x00509c50, while AD-66's byte-confirmed sibling landing is WITHHELD pending #341's measurement-anomaly apparatus, and AD-69 records the seam-frame dist gap the same pass discovered; AD-56 RESTORED 2026-08-07 — the a8a7d64b revert had collaterally DELETED it, the inverse of the AD-55 zombie it also created; its plumb-fall-freeze condition is live again since TS-4’s real retirement at Slice 2B; AD-55 RE-RETIRED 2026-08-07 — its 2026-07-30 retirement at 252e8068 was collaterally resurrected by the a8a7d64b revert of the unrelated TS-4 commit; the code kept the cos(10°) fix throughout; AD-68 filed 2026-08-07 at the #338 closure — the async-residency placeholder mover shape (0.4/0.4 steps + capsule) has no retail counterpart because retail loads synchronously; AD-67 filed 2026-08-07 at the #32 closeout — the narrowed `SetContactPlane` keeps its per-write `ContactPlaneCellId`, which retail writes only at `init_contact_plane`; AD-49 filed 2026-08-06 at the #334 fix — the BSP part-array flood runs its outdoor cell rectangle at seed time rather than only from retail’s residency-gated walk, keeping both registration floods on one residency rule; AD-64 filed 2026-08-05 at the C5b architecture review's D1 fix — AD-60's W2 wire-cell REACHABILITY decision is expressed once per host because the two hosts run parallel non-shared inbound routes; the committed VALUE is single-sourced at `RuntimeEntityObjectLifetime.CommitWireCellRebucket`, and unification is filed as #324; AD-60 CORRECTED the same day — its surviving-channel enumeration presented "the local force path, the missile arm" as exhaustive when the entire no-window host belonged in it; AD-1 RETIRED 2026-08-05, C5a deletion sweep — the legacy outdoor demote/restore lift this row described was `PhysicsEngine.Resolve`'s own body, deleted with zero production callers; AD-42 DELETED 2026-08-04, C4 route 3 — its last surviving citation, the headless portal-arrival resync's two-call Resolve/ResolvePlacement split, was retired by the canonical `RuntimeAcceptedPositionDriveController` portal arm; AD-2 amended same route with the deferred-place timing adaptation, the T8 tolerated-overwrite note, and the leash-anchor nuance; AD-63 filed 2026-08-04, cancelled-park presentation rollback — the rollback restores every presentation registration the park's Withdraw removed EXCEPT the player's selection, which is user intent rather than a projection; AD-62 filed 2026-08-03, C4 route 2 round 2 — a deferred ForcePosition retired without committing is not re-applied and its ack is not sent; AD-61 filed 2026-08-02, C3c review round 1 — the #270 settle compression now covers the local player; AD-59/AD-60 filed 2026-08-02, continuation-executor slice) Recent retirements: AD-3/AD-4 retired 2026-07-31 by exact active/per-candidate visible-cell availability, full-catalog containment-root validation, and the @@ -186,6 +186,7 @@ readiness/requeue adaptation. See | AD-88 | **Filed 2026-08-13 at the #385 dropdown fix (classification: UNCLEAR).** The vendor category dropdown ships G5's fixed 6-row scrollable popup window, but its authored popup ListBox (`0x21000043/0x10000350`) is edge-docked on all four sides (L=T=R=B=1, measured by menuprobe3 `OptionsPanelLiveMountProbeTests.ProbeMenuPopupSizingAndTextStyle`) — the exact authored condition that arms retail `UIElement_Menu::RecalculatePopupSize @0x0046caf0`, which resizes the popup to the ListBox's summed content height, uncapped (`0x0046e5f4..0046e66c`). The Config option-menus' identical docked shape now drives `UiMenu.PopupSizeToContent=true` (#385); vendor deliberately keeps `false`. | `src/AcDream.App/UI/Layout/VendorUiController.cs` (its UiMenu wiring leaves `PopupSizeToContent` at the class-default false) | The G5 vendor-gate retail screenshot was read as a ~6-row-with-scrollbar look and the vendor connected gate USER-PASSED on that shape — reworking a user-gated surface on decomp inference alone would invert the retail-oracle rule. The two pieces of evidence conflict; the row records the conflict rather than silently picking a side. | If retail actually opens the category popup full-height, our vendor dropdown shows a 6-row scroll window where retail shows every category at once — visible at any vendor with >6 categories. If retail truly shows 6 rows, the mechanism question (why the docked ListBox does not trigger RecalculatePopupSize there) is unanswered and could mislead the next dropdown port. | docs/ISSUES.md #386 (the retail side-by-side to run + the two candidate resolutions); #385 (the Config fix that exposed the conflict) | | AD-90 | **Filed 2026-08-13 at the #389 mechanism-review fix round (finding M1).** Retail's smartbox divisor aspect is not raw width/height: `RenderDevice::ComputeAspectForViewport @0x0054f150` yields `(w/h) × m_DisplayAspectRatio × 0.75`, with `m_DisplayAspectRatio` fed by the registered `Render.AspectRatio` preference. At that preference's DEFAULT (4:3) the factor is exactly 1.0f and the expression collapses to raw w/h — which is what acdream uses. acdream carries no AspectRatio preference at all. Also folded in: retail's `SetFOVRad` gate arithmetic ACCEPTS NaN (x87 unordered-compare quirk) where acdream's port rejects it — unreachable in practice, deliberately not reproduced (mechanism review M3). | `src/AcDream.App/Rendering/RetailFieldOfView.cs` (class doc names this row) | Bit-exact at retail's registered default; the preference existed for 2003-era stretched-CRT correction with no modern counterpart. Reproducing it would add a user knob retail itself defaulted away. | A retail user who had changed `Render.AspectRatio` saw framing acdream cannot reproduce; anyone porting FOV behavior from a capture made with a non-default AspectRatio preference will measure a mismatch against our law. | `RenderDevice::ComputeAspectForViewport @0x0054f150`; `Render::SetFOVRad @0x0054b2d0`; consumer `D3DXMatrixPerspectiveFovLH @0x0059ab71`; docs/research/2026-08-13-389-fov-mechanism-review.md | | AD-91 | **Filed 2026-08-13 at the #390 port.** acdream's display-change clamp covers ALL registered floating windows; retail's does not — every retail floaty overrides `MoveTo` with the clamp `x = max(0, min(x, parentW − selfW))` EXCEPT `gmFloatyChatUI` (floating chats 2–4), which has no clamp and can genuinely strand off-screen on a resolution change (decomp finding, `docs/research/2026-08-13-retail-ui-display-change.md`). The display block's product requirement ("UI windows must stay reachable on resolution change", the 2026-08-13 /goal) overrides the exception. | `src/AcDream.App/UI/RetailWindowLayoutPersistence.cs` (`ClampAllToScreen` — clamps every attached handle, floating chats included) | User-directed reachability beats reproducing a retail defect-shaped gap; the clamp math itself is retail's own, applied uniformly. | A retail-parity comparison that deliberately strands a floating chat window will find acdream rescuing it where retail leaves it lost. | `UIElementManager::RefreshEvent @0x0045C530`; `UIElement::UpdateForParentSizeChange @0x00462640`; the per-floaty `MoveTo` clamp overrides; docs/research/2026-08-13-retail-ui-display-change.md | +| AD-92 | **Filed 2026-08-13 at the #376/#388 review fix round (blast M6 / mechanism M4).** Two switcher adaptations with no retail counterpart: (1) the fullscreen refresh rate is the monitor's HIGHEST for the picked WxH — retail passed the device mode's own refresh as-is (`Device::ForceDisplayResolution`); (2) an invalid/unsupported fullscreen request is a logged refusal that leaves the window unchanged — retail attempted the switch and surfaced the device error. The persisted-flag divergence a refusal leaves behind is ISSUES #392. | `src/AcDream.App/Settings/DisplayModeSwitching.cs` (`TryFindRefreshRate`, the refusal paths); `src/AcDream.App/Settings/RuntimeSettingsTargets.cs` (`Apply`'s refused-mode logging) | Highest-refresh is strictly better on modern variable-refresh panels (retail predates them); refuse-and-log is #388's own no-crash requirement. | A capture comparing retail's exact chosen refresh for a mode will differ; a server/tooling flow expecting an error dialog on an invalid mode sees a console line instead. | `Device::ForceDisplayResolution @gmClient::Init 0x004047af`; docs/research/2026-08-13-376-388-{mechanism,blast}-review.md | --- diff --git a/docs/research/2026-08-13-376-388-blast-review.md b/docs/research/2026-08-13-376-388-blast-review.md new file mode 100644 index 00000000..be43ba15 --- /dev/null +++ b/docs/research/2026-08-13-376-388-blast-review.md @@ -0,0 +1,497 @@ +# Blast-radius review — `e56aa511` (#376 + #388: real fullscreen mode switching, state-aware display apply) + +**Reviewer lens:** BLAST RADIUS (who else touches the changed surfaces, and who +read the values they used to write). Report only — no edits made. +**Date:** 2026-08-13 +**Base:** `e56aa511`, worktree `.claude/worktrees/eloquent-hugle-42119e`, +branch `claude/latest-commits-cb0c8f`. + +The commit is correct about the thing it set out to fix: a raw `Size` write +against a fullscreen GLFW window is genuinely gone, the mode switch is genuinely +native, and the failure paths are genuinely no-throw. Everything below is about +the *other* edges those two changes now sit on. + +--- + +## Verdict summary + +| # | Surface | Verdict | +|---|---|---| +| M1 | Every Config-tab display row re-runs the fullscreen switch; enter has no idempotence guard; sliders apply per drag tick | **MUST-FIX** | +| M2 | Validation source ≠ switch target (catalog monitor vs primary monitor; UI fallback vs validator fallback) | **MUST-FIX** | +| M3 | #388's *maximized* half is unfixed, and the only un-maximize in the codebase was deleted — but the issue is marked DONE | **MUST-FIX** | +| M4 | Refused/failed enter leaves `fullscreen: true` persisted + checkbox ticked, with no surface but a console line | **MUST-FIX** | +| M5 | Two `GlfwDisplayModeSwitcher` instances → the windowed-position memory is dead on the boot-fullscreen exit path (gate §D5.3 asserts the opposite) | **MUST-FIX** | +| M6 | No divergence-register row for the deviations this commit introduces | **MUST-FIX (process)** | +| W1 | Pacing monitor-refresh read happens *before* the switch, and `StateChanged` (the signal the deleted `WindowState` write used to fire) is now dead | WATCH | +| W2 | `IsFullscreen` is the one unguarded native call in a "never throws" class | WATCH | +| W3 | Windowed exit with no parseable resolution falls back to the *fullscreen* size | WATCH | +| W4 | `glfwSetWindowMonitor` dispatches Silk callbacks synchronously, inside the quiescence lock, inside the update phase | WATCH (verified safe, undocumented) | +| W5 | Hard-kill while fullscreen now leaves a real display-mode change to Windows to unwind | WATCH | +| W6 | `DisplayModeCatalog.InstallFromWindow` is unguarded, and a silent non-install disables fullscreen entirely | WATCH | +| A1 | Startup `ApplyDisplay` ordering vs `InstallFromWindow` | **CLEAN** | +| A2 | Readers of `_window.WindowState` across `src/` | **CLEAN** (there are none) | +| A3 | `RetailWindowLayoutPersistence` resolution key shared fullscreen/windowed | **CLEAN** (correct: same client extent) | +| A4 | #390 edge-detector sequence for enter and exit | **CLEAN** | +| A5 | Tests / `InspectingDisplayWindowTarget` | **CLEAN** | +| A6 | Headless / no-window hosts | **CLEAN** (never reach this code) | + +--- + +## Axis 1 — callers of `SilkRuntimeDisplayWindowTarget` / `IRuntimeDisplayWindowTarget` + +Exactly **two production construction sites**, both in `AcDream.App`, both +inside `GameWindow.OnLoad`'s single composition pipeline run: + +| Site | Path | Instance lifetime | +|---|---|---| +| `src/AcDream.App/Rendering/GameWindow.cs:1296` | startup — `RuntimeSettingsStartupTargets` → `ApplyDisplay` | process | +| `src/AcDream.App/Composition/SessionPlayerComposition.cs:334` | live save — `RuntimeSettingsTargets` → `ApplyDisplayWindowState` | process (composition runs once at `OnLoad`, not per generation) | + +Consumers of the interface: `RuntimeSettingsStartupTargets.ApplyDisplay` +(`RuntimeSettingsTargets.cs:194`) and +`RuntimeSettingsTargets.ApplyDisplayWindowState` (`RuntimeSettingsTargets.cs:424`). +No headless / no-window host reaches either — `AcDream.Headless` references only +`AcDream.Runtime`, and `SessionPlayerDependencies.Window` is a non-nullable +`IWindow` supplied only by `GameWindow` (`SessionPlayerComposition.cs:41`, +`GameWindow.cs:1445`). **CLEAN.** + +### Callers relying on the OLD behaviour + +- *"`Size` write while fullscreen persists a windowed size for later."* No + consumer depended on this. A resolution pick while fullscreen is now a real + mode switch, and the later windowed exit takes its size from + `display.Resolution` (`RuntimeSettingsTargets.cs:141`), which is the same + value the old `Size` write would have parked. **CLEAN.** +- *"`WindowState.Fullscreen` borderless."* Nothing in `src/` reads window state + (see Axis 2). **CLEAN** — with one exception, the *maximized* case, which is + M3 below. + +### M5 — two switcher instances, one live position memory + +`GlfwDisplayModeSwitcher._windowedPosition` (`DisplayModeSwitching.cs:47`) is +per-instance and defaults to `(60, 60)`. It is written only inside +`TryEnterFullscreen` (`DisplayModeSwitching.cs:90-96`). + +Boot-fullscreen flow: + +1. Phase 3 (`SettingsDevToolsComposition.cs:53` → `ApplyStartup` → + `ApplyDisplay`) enters fullscreen through the **startup** target's switcher. + That instance captures the real windowed position. +2. Phase 8 (`SessionPlayerComposition.cs:334`) builds the **live** target with a + fresh switcher whose `_windowedPosition` is still `(60, 60)`. +3. User unticks Full Screen → live path → `TryLeaveFullscreen` restores to + `(60, 60)` at the fullscreen resolution. + +On a 1920×1080 desktop that is a 1920×1080 client area placed at (60,60) — title +bar and bottom edge off-screen. The gate script's own acceptance text for §D5 +step 3 says the client must return "positioned where it was before entering +fullscreen"; for the §D4 boot-fullscreen path it will not. + +Cheapest correct fix: one shared switcher (or one shared placement record) rather +than one per target instance. + +--- + +## Axis 2 — readers of `_window.WindowState` + +`grep '\.WindowState|WindowState\.'` over `src/**/*.cs` returns **zero** reads of +`IWindow.WindowState` outside the deleted lines. Nothing keys window chrome, +screenshots, input mapping, or the L1 capability probes off it: + +- Screenshots: `PrivatePresentationRenderer.cs:81` takes `input.ViewportWidth/Height`, + seeded from `_window.Size` at `GameWindow.cs:1575`. Size-driven, not state-driven. +- Input: cursor mode is driven by camera mode only + (`CameraPointerInputController.cs:445-447`); nothing consults fullscreen. +- #348's native cursor cache takes the GLFW window handle + (`GameWindow.cs:1026`), which `glfwSetWindowMonitor` does not change. +- L1 probes (`GraphicalHostPlatformServices`, the bindless/MDI/SSBO gates) read + no window state. + +**CLEAN** on the read side. The one consumer of the *event*: + +### W1 — the `StateChanged` signal is now dead, and pacing reads the monitor too early + +`SilkWindowCallbackBinding.cs:153` wires `IWindow.StateChanged` → +`DisplayFramePacingController.OnWindowStateChanged` +(`DisplayFramePacingController.cs:132`) → `RefreshActiveMonitor()`. Silk raises +`StateChanged` from its own `WindowState` setter and its iconify/maximize +callbacks; a native `glfwSetWindowMonitor` bypasses all of them. So the deleted +`_window.WindowState = Fullscreen` write was, incidentally, the thing that +re-read the active monitor's refresh rate after a fullscreen transition. That is +gone. + +Compounding it, the startup path refreshes the monitor **before** the switch: + +``` +RuntimeSettingsTargets.cs:194-201 + _pacing.RefreshActiveMonitor(); // desktop mode, e.g. 60 Hz + _pacing.ApplyPreference(display.VSync); + _displayWindow.Apply(display); // switches the display to 1920x1080@300 +``` + +and the live path (`ApplyDisplayWindowState`, `RuntimeSettingsTargets.cs:424-434`) +never touches pacing at all. With VSync **off** and capped rendering, +`FramePacingPolicy.Resolve` (`FramePacingPolicy.cs:26-29`) caps the software +pacer at the cached Hz — which after a fullscreen enter is the pre-switch desktop +refresh. The only thing that rescues it is the incidental `Move` callback +(`SilkWindowCallbackBinding.cs:152`) firing because the window jumped to the +monitor origin — which does not fire if the window was already at that position. + +Rated WATCH, not MUST-FIX, because VSync defaults on (`DisplaySettings.cs:92`) +and the VSync branch ignores the refresh value. Fix is one line: refresh the +active monitor *after* `_displayWindow.Apply`, on both paths. + +### W2 — `IsFullscreen` can throw out of a "never throws" class + +`DisplayModeSwitching.cs:54-62` calls `Glfw.GetApi().GetWindowMonitor(handle)` +with no `try`, while both `Try*` methods around it are wrapped. The class doc +promises "every failure is a no-throw (bool, reason) result". `Apply` calls +`IsFullscreen` on the windowed branch (`RuntimeSettingsTargets.cs:134`), which is +the branch that runs on every single Config-tab save. Low probability +(`glfwGetWindowMonitor` on a live handle does not error), but the contract is +stated and not honoured. + +--- + +## Axis 3 — `DisplaySettings.Fullscreen` consumers + +Complete reader set (`grep Fullscreen --include=*.cs src`): + +| Reader | Site | +|---|---| +| Startup apply | `RuntimeSettingsTargets.cs:116` (via `ApplyDisplay`, `:199`) | +| Live apply | `RuntimeSettingsTargets.cs:116` (via `ApplyDisplayWindowState`, `:426`) | +| Config tab "Full Screen" row | `ConfigOptionsPageController.cs:650-651` | +| Persistence | `SettingsStore.cs:82` (read), `:640` (write) | + +`RetailWindowLayoutPersistence` does **not** read it — see A3. + +### Fullscreen boot path, end-to-end + +1. `GameWindow.Run` creates the window unconditionally at 1280×720 + (`GameWindow.cs:752-754`). +2. `OnLoad` → `DisplayModeCatalog.InstallFromWindow(_window!)` + (`GameWindow.cs:1246`). +3. Composition phase 3 builds `SilkRuntimeDisplayWindowTarget(_window!)` + (`GameWindow.cs:1296`) and `SettingsDevToolsCompositionPhase.Compose` runs + `ApplyStartup` (`SettingsDevToolsComposition.cs:53`) → + `RuntimeSettingsController.ApplyStartup:216` → `ApplyDisplay` → + `Apply(display)`. +4. Fullscreen branch → catalog check → `TryEnterFullscreen` → + `glfwSetWindowMonitor`. +5. GLFW dispatches the framebuffer-size callback synchronously; the viewport + target arms `VulkanGraphicsContext.RequestRecreate` + (`VulkanHostInputCameraCompositionFactory.cs:136`) and the camera aspect + updates (`FramebufferResizeController.cs:125-127`). Both targets are already + bound by phase 2, so the mid-`OnLoad` event lands on live owners. + +Functionally sound. The problem is what happens when step 4 says no. + +### M4 — flag/reality divergence has no surface + +The Config toggle persists first, applies second +(`ConfigOptionsPageController.cs:651` → `RuntimeSettingsController.cs:386-392`). +Both refusal paths (`RuntimeSettingsTargets.cs:120-125` "not an offered mode", +`:126-128` "failed") only `Console.WriteLine`. Consequences: + +- `settings.json` keeps `fullscreen: true` while the client is windowed. +- The checkbox reads `bindings.LoadDisplay().Fullscreen` + (`ConfigOptionsPageController.cs:650`) → renders **ticked** against a windowed + client. +- Every subsequent launch refuses again. The client boots windowed forever with + the box ticked and no visible explanation. The user's only feedback is a + console line they will not see in a normal launch. + +This matters because the refusal is *reachable in ordinary use*, not just in +pathological setups — see M2. At minimum the refusal should reset the persisted +flag (or the row should read live state rather than the stored flag) so that what +the panel shows is what the client is. + +Note the interaction with #377's documented recovery ("hand-edit settings.json +back to false"): the new refusal path re-creates exactly that stuck-flag shape, +minus the crash. + +--- + +## Axis 4 — the #390 interaction (`2153bee2`) + +`RetailUiRuntime.Draw` (`RetailUiRuntime.cs:620-652`) is a two-step edge detector +on the per-frame screen size. That size is `_window.Size`, captured once per +render at `GameWindow.cs:1575` and threaded through +`RenderFrameInput.ViewportWidth/Height` → +`PrivatePresentationRenderer.cs:72-76` → `RetainedGameplayUiFrame.Render` → +`Draw`. + +**Enter (1280×720 → 1920×1080).** The toggle is handled in the *update* phase +(this is the same seam the #389 review deliberately chose over the render-phase +preview seam — `RuntimeSettingsTargets.cs:427-433`), so the native switch +completes before the frame's render: + +- frame N render: `_window.Size` = 1920×1080 ≠ `_lastScreenSize` → one + `ClampAllToScreen()` (`RetailWindowLayoutPersistence.cs:102-129`, no store I/O, + `_restoring` suppresses per-move saves), `_screenSizeSettling = true`. +- frame N+1: size repeats → one `RestoreAll(saveBack: false)` for key + `"1920x1080"`. + +One clamp, one reload, no transient size (the intermediate 1280×720 frame never +renders — the switch is synchronous within the update). **CLEAN.** + +**Exit.** `TryLeaveFullscreen` is called with the *same* `display.Resolution` +(`RuntimeSettingsTargets.cs:141`) — unticking Full Screen does not change the +resolution setting. So the screen size does not change, the edge detector sees +nothing, and there is no clamp/reload. Correct, and consistent: the layout key is +unchanged. **CLEAN.** + +**A3 — the shared layout key.** `ResolutionKey` is `"{W}x{H}"` with no fullscreen +discriminator (`RetailWindowLayoutPersistence.cs:304-305`). Fullscreen 1920×1080 +and windowed 1920×1080 have the *same client extent*, so sharing the key is the +right answer, not a collision. **CLEAN.** + +**No storm risk from the detector itself** — but see M1, which can produce a +native-call storm upstream of it. + +--- + +## Axis 5 — tests + +- No test in `tests/` asserts the old `WindowState` write. The only `WindowState` + hits are `ChatWindowState`, `RetainedWindowState`, `CreateWindowState`, and + `DisplayFramePacingControllerTests.cs:65`'s `OnWindowStateChanged(default)` + smoke call — none coupled to the display apply. +- `InspectingDisplayWindowTarget` + (`tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs:1072`) is a + fake `IRuntimeDisplayWindowTarget`; the interface is unchanged, so all 7 uses + stay valid. +- `GameWindowSlice8BoundaryTests.cs:298/331` asserts on `GameWindow.cs` source + text only — unaffected. +- The 5 new state-machine facts + (`RuntimeSettingsControllerTests.cs:191-283`) cover exactly the five branches + and are honest tests (they assert `surface.Writes == 0` on the fullscreen paths, + which is the real invariant). + +**CLEAN.** Two gaps worth noting rather than fixing-as-bugs: + +- No test pins the **idempotence** of a repeated fullscreen apply (M1). A + `FakeModeSwitcher` already records `Calls`; asserting `["enter:1920x1080"]` + after two identical applies would have caught it. +- No test covers `Fullscreen = true` + unparseable resolution + (`RuntimeSettingsTargets.cs:118-119`, silent return). + +--- + +## Axis 6 — the catalog dependency and construction order + +**A1 — ordering: CLEAN.** Two facts make it safe: + +1. The offered-mode check is a **lazy** lambda, not a captured snapshot: + `spec => Rendering.DisplayModeCatalog.Resolutions?.Contains(spec) == true` + (`RuntimeSettingsTargets.cs:83`). Construction order relative to + `InstallFromWindow` is therefore irrelevant; only *apply* order matters. +2. `DisplayModeCatalog.InstallFromWindow(_window!)` is at `GameWindow.cs:1246`, + before `GameWindowCompositionPipeline.Run` at `:1249`; the startup apply is + phase 3 (`SettingsDevToolsComposition.cs:53`). Install strictly precedes apply. + +A legitimate fullscreen boot is **not** refused for ordering reasons. But it can +be refused for two other reasons, which is the real finding: + +### M2 — "an offered mode is supported by construction" is false along two axes + +The commit's central safety claim is that validating against `DisplayModeCatalog` +makes the unsupported-mode crash class unreachable. That holds only if the +offered set is a subset of the *switch target's* mode list. Two ways it isn't: + +**(a) Different monitors.** The catalog is curated from `window.Monitor` — the +monitor the window currently sits on (`DisplayModeCatalog.cs:53`). The switch +targets `glfwGetPrimaryMonitor()` (`DisplayModeSwitching.cs:77`), and the refresh +lookup enumerates *that* monitor's modes (`DisplayModeSwitching.cs:144-156`). On a +multi-monitor machine with the client on a secondary display, a mode that is +offered (secondary supports it) can be absent from the primary's list → +`"mode WxH is not in the monitor's mode list"` → silent refusal (M4). It fails +safe rather than crashing, but the invariant the design leans on is not there. + +**(b) Different fallbacks at the two seams.** When no catalog installs +(`window.Monitor` null, or `Curate` returns empty — `DisplayModeCatalog.cs:54,68`): + +- the dropdown falls back to the 6-entry static ladder + (`RetailUiRuntime.cs:2429` → `ConfigOptionsPageController.cs:641` + `availableResolutions ?? DisplaySettings.AvailableResolutions`), so the user + sees six selectable resolutions; +- the validator falls back to **nothing offered** + (`RuntimeSettingsTargets.cs:83`, `Resolutions?.Contains(...) == true` on null → + `false`). + +So on any host where the catalog does not install, the Full Screen checkbox is a +permanent silent no-op while the dropdown still advertises six modes. The two +seams must share one fallback. + +**(c) The likely everyday case.** `DisplaySettings.Default.Resolution` is +`"1280x720"` (`DisplaySettings.cs:90`). If the monitor does not enumerate a +1280×720 mode (common on high-refresh panels that expose a short mode list), a +fresh user who simply ticks Full Screen at the default resolution gets nothing — +no fullscreen, no message. Likewise any settings.json carrying a pre-#391 +resolution (`800x600`, `1024x768` — both selectable in earlier builds, per the +OP6 note at `ConfigOptionsPageController.cs:625-633`) now boots windowed forever +with the flag ticked. + +--- + +## M1 — the fullscreen enter is not idempotent, and every Config row re-runs it + +This is the highest-blast-radius finding, because it comes in through a surface +the commit never looked at. + +`storeOnly` on the Config rows is **only a caption-dim marker** +(`ConfigOptionsPageController.cs:136-152`) — it does not suppress the apply. +Every Graphics row's `apply:` lambda calls `bindings.SaveDisplay(...)` +(`ConfigOptionsPageController.cs:587, 643, 651, 659, 671, 678, 685, 693, 738, +746, 754, 766, 774, 780`), which routes +`RuntimeSettingsController.SaveDisplay:386-392` → +`ApplyDisplayWindowState` → `SilkRuntimeDisplayWindowTarget.Apply`. + +And `Apply`'s fullscreen branch has **no idempotence guard** +(`RuntimeSettingsTargets.cs:116-129`) — unlike the windowed branch, which still +has `if (haveResolution && (_window.Size.X != width || ...))` at `:147`. The old +code had `if (_window.WindowState != desired)` at the deleted `:83`; that guard +was removed with the rest of the block and nothing replaced it. + +So, while `fullscreen: true`: + +- toggling *any* store-only graphics row (MultiPassAlpha, BuildingDetailTextures, + LandscapeDrawDistance, TextureFiltering, …) fires a full + `glfwSetWindowMonitor(handle, primary, 0, 0, W, H, refresh)`; +- **dragging a slider** does it *per drag tick*. `FloatOptionRow.SetCurrentValue` + is documented as "the drag-tick entry point… applies it live IMMEDIATELY" + (`OptionPageModel.cs:232-240`), and `slider.ScalarChanged` calls it directly + (`ConfigOptionsPageController.cs:1044`). Field of View (`:587`), Screen + Brightness (`:671`) and Degrade Distance (`:693`) are all sliders on this tab. + +Per drag tick that means: a full `glfwGetVideoModes` enumeration +(`DisplayModeSwitching.cs:148`), a `glfwSetWindowMonitor`, and one +`display: fullscreen mode switch WxH@R` console line +(`DisplayModeSwitching.cs:99-100`) — a permanent evidence line turned into a +per-frame log storm. + +What saves it from being a visible mode-change storm on Windows is GLFW's +*internal* early-out (`setVideoModeWin32` returns before `ChangeDisplaySettingsEx` +when the chosen mode equals the current one). Correctness of a user-visible +behaviour should not rest on an undocumented internal of a third-party library — +especially on the exact path whose failure mode was "the client dies mid-session". + +The gate script's own §D6 step 1 tells the user to "drag sliders in between" +while fullscreen, so this is directly in the acceptance path. + +**Fix shape:** guard the enter the way the windowed branch guards its write — +remember the last applied `(W, H, monitor)` or query the current window monitor + +mode, and return early when the request already holds. + +--- + +## M3 — #388's maximized half is unfixed, and the only un-maximize was deleted + +#388's own title and filing include the maximized case: *"fullscreen/maximized +windows silently ignore resolution picks"*, with the evidence being five +consecutive picks producing no framebuffer event at `window was 2056x1290` +(`docs/ISSUES.md:126, 158-165`). The commit marks #388 **DONE** +(`docs/ISSUES.md:127`) having addressed only the fullscreen half. + +Worse, the deleted block was the codebase's *only* write of `WindowState` +(Axis 2: there is now zero). Under the new `Apply`, a maximized window takes the +`_modeSwitcher.IsFullscreen == false` path (a maximized window has no attached +monitor) and receives a plain `_window.Size = ...` +(`RuntimeSettingsTargets.cs:147-154`) — precisely the write the issue documents +as silently ignored. Nothing anywhere restores the window to `Normal` first. + +So: resolution picks on a maximized window are now permanently a no-op, with the +issue closed. Either restore-then-resize in the windowed branch, or reopen the +maximized half of #388 with the evidence carried forward. + +--- + +## M6 — missing divergence-register rows (process) + +`git show e56aa511 --stat` touches no register file. CLAUDE.md's rule is binding: +*"Any commit that introduces a deviation adds its register row IN THE SAME +COMMIT."* At least two deviations ship here, both acknowledged in the code's own +comments: + +1. **Refresh-rate selection.** `TryFindRefreshRate` picks the monitor's + *highest* refresh for the requested WxH (`DisplayModeSwitching.cs:142-156`), + while the class doc states "retail passed the device's mode as-is" + (`DisplayModeSwitching.cs:37-38`). Entering fullscreen therefore changes the + panel's refresh rate as a side effect of a *resolution* pick (the live run + went to 300 Hz). +2. **Silent refusal.** Retail surfaced "Graphics mode not supported"; acdream + refuses to a console line and leaves the preference inconsistent (M4). + +Arguably a third: primary-monitor-only targeting (`DisplayModeSwitching.cs:77`), +which the doc justifies against retail's primary display device — that one is +probably a citation rather than a deviation, but it is the assumption M2(a) +breaks, so it deserves to be written down somewhere auditable. + +IA-22 (#391's curated list) is the natural neighbour row; these are separate +deviations and need their own. + +--- + +## Remaining WATCH items + +**W3 — windowed exit with no parseable resolution uses the fullscreen size.** +`RuntimeSettingsTargets.cs:136-140`: when `display.Resolution` does not parse, the +exit falls back to `_window.Size` — which, while fullscreen, *is* the fullscreen +extent. The window then leaves fullscreen as a desktop-sized window at +`(60, 60)`. Narrow (requires a corrupt/blank resolution string), but the fallback +reads "current windowed size" and means "current fullscreen size". + +**W4 — synchronous native dispatch inside the quiescence lock.** On Win32, +`glfwSetWindowMonitor` dispatches `WM_SIZE`/`WM_MOVE` synchronously, so +`FramebufferResize` and `Move` re-enter `HostQuiescenceGate.Invoke` +(`SilkWindowCallbackBinding.cs:152-154`) on the same thread while the update +callback already holds `_sync`. This is **safe** — `Monitor` is re-entrant +(`HostQuiescenceGate.cs:36-54`) — and the Vulkan side only *arms* recreation +(`VulkanHostInputCameraCompositionFactory.cs:136`), never recreating mid-frame. +Both are load-bearing invariants that nothing records; a comment at the +`SetWindowMonitor` call site would keep a future change from breaking them. + +Also thread-affinity: `glfwSetWindowMonitor` and `glfwGetVideoModes` are +main-thread-only. Both call paths (composition `OnLoad`, update-phase save) are +main-thread today; nothing enforces it. + +**W5 — hard kill while fullscreen.** The project's documented launch workflow +falls back to `Stop-Process -Force` when `CloseMainWindow` times out (CLAUDE.md, +"Logout-before-reconnect"). Previously that left a borderless window behind; +now it abandons a real `ChangeDisplaySettingsEx` mode change for Windows to +unwind. Low risk (CDS_FULLSCREEN changes are process-scoped and revert on exit), +but worth one line in the gate script's black-screen-risk note. + +**W6 — `InstallFromWindow` is unguarded and fails silently.** +`GameWindow.cs:1246` has no `try`, while `SilkDisplayFramePacingSurface` already +documents that `window.Monitor` can throw `GlfwException` during an RDP/console +display-topology handoff (`DisplayFramePacingController.cs:42-49`). A throw there +kills `OnLoad`; a silent early-return (`DisplayModeCatalog.cs:54-69`) now also +silently disables fullscreen entirely (M2b). The two failure modes deserve +different treatment, and neither is currently distinguishable in a log. + +--- + +## Ranked must-fixes + +1. **M1** — guard the fullscreen enter against a no-change request. + `src/AcDream.App/Settings/RuntimeSettingsTargets.cs:116-129`; drivers at + `src/AcDream.App/UI/Layout/OptionPageModel.cs:232-240` and + `src/AcDream.App/UI/Layout/ConfigOptionsPageController.cs:587,671,693`. +2. **M2** — make the offered set and the switch target agree: same monitor + (`DisplayModeCatalog.cs:53` vs `DisplayModeSwitching.cs:77`) and same fallback + (`RuntimeSettingsTargets.cs:83` vs `RetailUiRuntime.cs:2429` → + `ConfigOptionsPageController.cs:641`). +3. **M3** — either fix the maximized half of #388 (restore-to-`Normal` before the + windowed size write, `RuntimeSettingsTargets.cs:147-154`) or reopen it; + `docs/ISSUES.md:126-127` currently claims it closed. +4. **M4** — give a refused/failed enter a surface: reset the persisted flag, or + have the Config row read live state. + `RuntimeSettingsTargets.cs:120-128`, `ConfigOptionsPageController.cs:650-651`. +5. **M5** — share one windowed-placement record between the startup and live + switchers. `DisplayModeSwitching.cs:47`, `GameWindow.cs:1296`, + `SessionPlayerComposition.cs:334`. +6. **M6** — add the divergence-register rows (refresh-rate selection; silent + refusal), `docs/architecture/retail-divergence-register.md`. diff --git a/docs/research/2026-08-13-376-388-mechanism-review.md b/docs/research/2026-08-13-376-388-mechanism-review.md new file mode 100644 index 00000000..6c92c888 --- /dev/null +++ b/docs/research/2026-08-13-376-388-mechanism-review.md @@ -0,0 +1,528 @@ +# Mechanism review — `e56aa511` (#376 + #388): native fullscreen mode switching + state-aware display apply + +**Reviewer lens:** MECHANISM (does the machine do what it claims, on the +platform it runs on). +**Scope:** `src/AcDream.App/Settings/DisplayModeSwitching.cs` (new), +`src/AcDream.App/Settings/RuntimeSettingsTargets.cs` +(`SilkRuntimeDisplayWindowTarget.Apply` rework), and their live +surroundings. +**Date:** 2026-08-13. **Report only — no files were edited.** + +Primary evidence for the Silk/GLFW claims below is IL read directly out of +the pinned packages +(`Silk.NET.GLFW 2.23.0`, `Silk.NET.Windowing.Glfw 2.23.0` under +`%USERPROFILE%\.nuget\packages`), not recollection. Where a claim rests on +IL, the decoded bytes are quoted. + +--- + +## Verdict summary + +| # | Question | Verdict | +|---|---|---| +| 1 | GLFW API correctness (`Glfw.GetApi()`, main thread) | **PASS** on both halves — but the error-handling contract built on top of it is broken (M1) | +| 2 | State-machine holes | **FAIL** — M2 (no idempotence → video-mode storm), M5 (persisted state diverges, one path silent). The `_windowedPosition` clobber the question asked about is correctly guarded. | +| 3 | Silk-vs-native `WindowState` desync | **PASS** — no desync exists; the getter is native-backed and no writer remains. Events fire on both paths. One latent trap (W3). | +| 4 | Exit path / remembered position | **PARTIAL FAIL** — M3: two switcher instances, so the startup-fullscreen → live-exit path never restores the remembered placement. True off-screen stranding is unlikely (W5). | +| 5 | Validation seam | **PARTIAL FAIL** — M4: catalog and switcher enumerate *different monitors*. The catalog-uninstalled (fixture/headless) case refuses cleanly rather than crashing — that half **PASSES**. | +| 6 | #377 regression surface (startup ordering) | **PASS on ordering** — the composition order is safe and the quiescence gate is reentrant. But the commit does not close #377's mechanism; it makes it reachable more often (W1). | + +--- + +## MUST-FIX findings, ranked + +### M1 — `catch (GlfwException)` is dead code on Windows: a failed native switch reports SUCCESS and arms a deferred crash + +`DisplayModeSwitching.cs:103-109` (and `:135-139`) wrap the native calls in +`catch (GlfwException)`, and the interface doc at `:24-25` promises +"False (with a reason) instead of throwing on any failure". The commit +message repeats it: "every failure is a no-throw (bool, reason) result." + +**On Windows that is false.** Silk's process-global GLFW error callback is +installed by `GlfwProvider::GetGlfw` +(`… callvirt Glfw::Init … call Glfw::get_ErrorCallback; callvirt +Glfw::SetErrorCallback`) and its body is `Glfw.<>c::<.cctor>b__143_0`. +Raw IL (51 bytes): + +``` +00: 72 3F160070 ldstr "{0}: {1}" +05: 03 8C 11000002 ldarg.3 ; box (error code) +0B: 02 ldarg.2 (description) +0C: 28 4F00000A call String::Format +11: 73 9E000006 newobj GlfwException::.ctor +16: 25 03 6F A2..06 dup; ldarg.3; set_ErrorCode +1D: 0A stloc.0 (ex) +1E: 7E 40010004 ldsfld Glfw::_isWindows +23: 2C 02 brfalse.s -> 0x27 +25: 06 7A ldloc.0; THROW <- non-Windows only +27: 7E 41010004 ldsfld Glfw::_exceptions +2C: 06 6F 5600000A ldloc.0; callvirt List::Add <- WINDOWS: DEFERRED +32: 2A ret +``` + +i.e. `if (!_isWindows) throw ex; else Glfw._exceptions.Add(ex);`. + +The drain is `Glfw.ThrowExceptions()`, and the only callers in the entire +Silk closure are seven `GlfwWindow` methods: + +``` +GlfwWindow::Create, CoreInitialize, CoreReset, RegisterCallbacks, +GlfwWindow::SetWindowIcon, set_Monitor, GetProcAddress +``` + +(verified by scanning `Silk.NET.Windowing.Glfw`, `Silk.NET.Input.Glfw`, +`Silk.NET.Windowing.Common`, `Silk.NET.Input.Common`, `Silk.NET.Core` — +the last four contain **zero** call sites). None of them is per-frame, and +none of them is on any path this commit takes. `ThrowExceptions` also never +clears the list — its 66-byte body contains `get_Count`, `get_Item(0)`, +`AggregateException`, `throw`, and no `Clear` — so a queued error is +re-raised at *every* later drain. + +**Consequences, in order of severity:** + +1. A failing `glfw.SetWindowMonitor` (`DisplayModeSwitching.cs:98`) — e.g. + "Failed to set video mode: Graphics mode not supported", the exact #388 + error — returns normally. `TryEnterFullscreen` returns `true` and prints + `display: fullscreen mode switch WxH@R` (`:99-100`) for a switch that did + not happen. Every downstream consumer, including the §D5/§D6 gate script + and the "live-verified" evidence line in the commit message, reads that + log as proof of success. **The gate's own oracle can lie.** +2. The queued `GlfwException` detonates at the next `ThrowExceptions()`, + which in this client is `GlfwWindow::CoreReset` — window close. A failed + mode switch mid-session therefore surfaces as an unexplained crash *at + shutdown*, which is exactly the path the project needs graceful for ACE + session cleanup. +3. This also retro-explains #388's "first surfaced as a caught + `settings: display save failed: PlatformError…`, then a second fired as + an UNHANDLED exception": with a never-cleared static list, **the same + exception instance is thrown twice** from two different drains. That + pattern is a signature of this mechanism, not of two independent errors. + +**Fix direction (no workaround):** do not rely on exceptions at all. Verify +the *post-condition* natively inside `TryEnterFullscreen` / +`TryLeaveFullscreen`: after `SetWindowMonitor`, re-read +`glfw.GetWindowMonitor(handle)` and `glfw.GetVideoMode(monitor)` and require +they match the request; return `false` with the observed state otherwise. +That is platform-independent, needs no knowledge of Silk's deferral, and +turns the log line into a measurement instead of an assumption. Draining +`Glfw.ThrowExceptions()` is a *second-choice* option only, and must account +for the never-cleared static list (a stale error from anywhere in the +process would be misattributed to the switch). + +--- + +### M2 — No idempotence guard on the fullscreen branch: every Display-backed Config row re-issues a REAL video-mode change, once per slider drag tick + +`RuntimeSettingsTargets.cs:116-130` — the fullscreen branch calls +`TryEnterFullscreen` **unconditionally**. The windowed branch immediately +below keeps its change guard (`:147`, +`if (haveResolution && (_window.Size.X != width || _window.Size.Y != height))`), +and the code this commit **deleted** had one too +(`if (_window.WindowState != desired) _window.WindowState = desired;`). The +idempotence that existed before this commit was removed on the fullscreen +side only. + +Why that matters here specifically: + +- Every Display-backed Config row funnels through the same apply: + `ConfigOptionsPageController.cs:587, 643, 651, 659, 671, 678, 685, 693, + 738, 746, 754` → `bindings.SaveDisplay(...)` → + `RuntimeSettingsController.SaveDisplay` (`:386-399`) → + `ApplyDisplayWindowState` → `Apply`. +- `FloatOptionRow.SetCurrentValue` applies **live, on every drag tick, not + on release** — stated verbatim in `OptionPageModel.cs:176-181` and relied + on by the Chat tab's fade-while-dragging behaviour. + +So, while fullscreen, dragging Field of View / Screen Brightness / Degrade +Distance / Graphics Performance issues one `glfwSetWindowMonitor` +(→ `ChangeDisplaySettingsEx`) **per mouse-move sample**, each one taking the +monitor through a real mode set + re-sync, each one re-entrantly firing +`FramebufferResize` + `Move` (see W1) and arming a swapchain recreate. Every +non-slider Display row (texture detail, filtering, degrades, quality preset, +VSync) does the same once per click. + +Secondary cost on the same path: `Glfw.GetApi()` is called fresh on every +`IsFullscreen` read (`:60`) and every `Try*` entry (`:76`, `:124`), and +`Glfw::GetApi` is `newobj GlfwLibraryNameContainer → GetLibraryNames → +CreateDefaultContext → newobj Glfw` — a new `DefaultNativeContext` (and a +`LoadLibrary`/`dlopen` refcount) per call, never disposed. That is ~2–3 per +apply, i.e. per drag tick under this bug. + +**Fix:** early-return when already fullscreen at the requested mode. That +needs a current-mode read on the seam (e.g. +`IDisplayModeSwitcher.TryGetCurrentMode(out int w, out int h)`), which the +fake can implement trivially — and a test asserting a same-state re-apply +issues zero `enter:` calls (`FakeModeSwitcher` already records them, so this +gap was one assertion away from being caught). + +--- + +### M3 — Two `SilkRuntimeDisplayWindowTarget` instances: the remembered windowed placement is lost exactly on the startup-fullscreen → live-exit path + +There are two production construction sites, each building its **own** +`GlfwDisplayModeSwitcher` with its own `_windowedPosition` field +(`DisplayModeSwitching.cs:47`, initial value `(60, 60)`): + +- `src/AcDream.App/Rendering/GameWindow.cs:1296` — the **startup** target + (`RuntimeSettingsStartupTargets`), which runs `ApplyDisplay` during + composition phase 3. +- `src/AcDream.App/Composition/SessionPlayerComposition.cs:334` — the + **live** target (`RuntimeSettingsTargets`), which runs + `ApplyDisplayWindowState` on every Config save. + +Sequence that breaks: `settings.json` has `fullscreen: true` → **instance A** +captures the real windowed position at `:90-96` and enters fullscreen → +user later unticks Full Screen in the Config tab → **instance B** runs +`TryLeaveFullscreen`, and its `_windowedPosition` has never been written, so +the window is placed at the `(60, 60)` literal. + +On a single monitor that is merely wrong-but-harmless; on multi-monitor it +teleports the client to the **primary** monitor's top-left regardless of +where it was. It also makes the commit message's claim ("the windowed +placement is remembered for the exit path") and gate-script step §D5 +("positioned where it was before entering fullscreen", +`docs/research/2026-08-13-display-block-test-script.md`) false for that +ordering — and *only* for that ordering, so a tester who toggles fullscreen +on and off within one session will not reproduce it. **The gate must +explicitly exercise launch-fullscreen → untick.** + +**Fix:** one process-wide switcher (or hoist the remembered placement into a +single owner both targets borrow). + +--- + +### M4 — Catalog and switcher enumerate DIFFERENT monitors, so "an offered mode is supported by construction" does not hold + +The whole crash-class argument rests on the catalog and the switcher agreeing +about which display's mode list is authoritative. They do not: + +- `DisplayModeCatalog.InstallFromWindow` (`Rendering/DisplayModeCatalog.cs:53`) + enumerates **`window.Monitor`**. Silk's `GlfwWindow::get_Monitor` (IL: + `GetWindowMonitor` → else walk `GlfwMonitorEnumerable` for the monitor whose + bounds contain the window centre → else `GetPrimaryMonitor`) means at + `OnLoad` this is *the monitor the window happens to be on*. +- `GlfwDisplayModeSwitcher.TryEnterFullscreen` (`:77`) enumerates and + switches on **`GetPrimaryMonitor()`**. + +Two failure shapes on a heterogeneous multi-monitor desktop: + +1. A mode offered from the secondary's list is absent from the primary's → + `TryFindRefreshRate` fails (`:84-88`) → fullscreen refused. The user ticks + the box, nothing happens, `settings.json` says `fullscreen: true` + (see M5). +2. Even on success, entering fullscreen moves the client to the *primary* + monitor. That may be retail-faithful (`Device::ForceDisplayResolution` + drove the primary display device, per the class doc), but it is a + behavioural deviation from "fullscreen the window where it is" and is not + in the divergence register. + +**Fix:** pick one monitor authority and use it at both ends (catalog + +switcher). If "always primary" is kept as the retail-faithful choice, the +catalog must enumerate the primary too, and a +`docs/architecture/retail-divergence-register.md` row must record the +window-jumps-to-primary behaviour. + +--- + +### M5 — Every fullscreen refusal/failure leaves `settings.json` inconsistent with reality; one path is entirely silent + +`RuntimeSettingsController.SaveDisplay` **persists first, applies second** +(`:390` then `:392`). `Apply`'s three fullscreen exits never revert: + +| Path | Line | Logged? | Persisted state after | +|---|---|---|---| +| Resolution unparseable | `:118-119` | **no log at all** | `fullscreen: true`, window windowed | +| Mode not offered | `:120-125` | yes (`refused — not an offered mode`) | `fullscreen: true`, window windowed | +| Switch failed | `:126-128` | yes (`failed (…) — staying windowed`) | `fullscreen: true`, window windowed | + +The unparseable case is the one the review brief asked about, and it is the +worst of the three: it produces **no `display:` line whatsoever**, so the +gate script's "any refused/failed switch logs a `display: … failed/refused` +line" acceptance criterion (§D6 step 1) cannot be met and the tester has +nothing to read. The Config checkbox then reads `true` from storage forever +and every subsequent launch silently re-refuses. + +A second accuracy defect lives at `:128`: when the *re-entry* attempt fails +while the window is **already fullscreen**, the message says "staying +windowed", which is factually wrong. + +**Fix:** log the unparseable case; make the failure message report the actual +resulting state; and decide deliberately whether a refusal should revert the +persisted `Fullscreen` (retail has a confirmation flow — `SetConfirmChange` +— already noted as unported in `ConfigOptionsPageController.cs:623`). + +--- + +## Per-question findings + +### Q1 — GLFW API correctness + +**`Glfw.GetApi()` is the right instance. PASS.** `Glfw::GetApi` IL is +`newobj GlfwLibraryNameContainer → SearchPathContainer::GetLibraryNames → +Glfw::CreateDefaultContext → newobj Glfw`: a *new managed wrapper* over the +*same native library*. GLFW's state (init flag, window list, monitor list, +error callback) lives in the native module, which the OS loader returns as +one instance per process — so a second wrapper drives the same GLFW. + +The `GraphicalWindowBackendSelection.cs:135-138` comment ("A separate +`Glfw.GetApi()` instance would receive the hint but would not own the window +backend's process-global GLFW state") is about **init-hint ordering**, not +about later calls: `GlfwProvider.UninitializedGLFW` is itself literally +`new Lazy(Glfw.GetApi)` (`GlfwProvider::.cctor` → `<>O::<0>__GetApi`), +so it is the same kind of object. Using `GlfwProvider.UninitializedGLFW.Value` +there matters because Silk must later `Init()` *that* instance; it does not +imply later `GetApi()` calls are wrong. `GlfwCursorCache.TryCreate` +(`Rendering/GlfwCursorCache.cs:47`) already established this in production +(#348). + +**Caveat (W4):** `GlfwCursorCache` caches its `Glfw` for the object's +lifetime; `GlfwDisplayModeSwitcher` calls `Glfw.GetApi()` on **every** +property read and method entry (`:60`, `:76`, `:124`). Each call allocates a +`Glfw` + `DefaultNativeContext` and takes a native-library refcount that is +never released (`NativeApiContainer.Dispose` is never called). Cache one +instance in the field, as the #348 precedent does. + +**Thread: PASS.** `Program.cs` constructs `GameWindow` and calls +`window.Run()` on the process main thread (`Program.cs:142`); +`GameWindow.Run` calls `Window.Create` (`GameWindow.cs:761`) and +`_window.Run()` (`:785`) on that same thread; Silk's loop and all callbacks +(Load/Update/Render/FramebufferResize) run on the calling thread. Both the +startup `ApplyDisplay` (composition, inside `OnLoad`) and the live +`ApplyDisplayWindowState` (Config-tab click handling in the update phase) +therefore execute on the GLFW main thread. `glfwSetWindowMonitor`, +`glfwGetVideoModes` and `glfwGetWindowPos` are all main-thread-only, and all +three are satisfied. + +### Q2 — State-machine hole enumeration + +`haveResolution` = `TryParseResolution` succeeded (`:113-114`); +`offered` = `_isOfferedMode("{w}x{h}")`; `fs` = `_modeSwitcher.IsFullscreen`. + +| # | target | fs | haveRes | offered | switcher result | Outcome | Assessment | +|---|---|---|---|---|---|---|---| +| 1 | FS | no | yes | yes | ok | enters fullscreen | correct | +| 2 | FS | no | yes | yes | fail | logs, stays windowed | correct **but see M1** (on Windows "fail" is not observable) | +| 3 | FS | no | yes | no | — | logs refusal, stays windowed | correct; **M5** (settings now lie) | +| 4 | FS | no | **no** | — | — | **silent return** | **M5** — no log, settings lie | +| 5 | FS | **yes** | yes | yes | ok | redundant real mode set | **M2** — storm | +| 6 | FS | **yes** | yes | yes | fail | logs "staying windowed" **while fullscreen** | **M5** (wrong message) | +| 7 | FS | **yes** | yes | no | — | logs refusal, remains fullscreen at old mode | acceptable; message is accurate | +| 8 | FS | **yes** | no | — | — | silent return, remains fullscreen | benign, but silent | +| 9 | win | yes | yes | — | ok | native exit at picked size + `_windowedPosition` | correct; **M3** (wrong position across instances), **W5** (size==desktop) | +| 10 | win | yes | yes | — | fail | logs, remains fullscreen | correct | +| 11 | win | yes | **no** | — | ok | exit at **current fullscreen size** (`:136-140` reads `_window.Size`, which under native fullscreen is the *mode* size) | window client = desktop size → frame overflows desktop (**W5**) | +| 12 | win | no | yes | — | — | size write iff changed (`:147`) | correct, guarded | +| 13 | win | no | no | — | — | no-op | correct | + +**On the specific idempotency sub-question:** the guard at `:90-96` is +**correct**. `_windowedPosition` is only captured when +`glfw.GetWindowMonitor(handle) is null`, so re-entering fullscreen while +already fullscreen cannot clobber it with `(0,0)`. GLFW itself tolerates +`glfwSetWindowMonitor` with the same monitor (it re-applies the mode); the +problem is not correctness of a single repeat but the *rate* of repeats +(M2). + +### Q3 — Silk-vs-native desync + +**No desync exists. PASS.** `GlfwWindow::get_CoreWindowState` is +native-backed: + +``` +_glfw.GetWindowAttrib(_glfwWindow, Iconified) -> Minimized +_glfw.GetWindowAttrib(_glfwWindow, Maximized) -> Maximized +_glfw.GetWindowMonitor(_glfwWindow) != null -> Fullscreen +else -> Normal +``` + +So after a native `SetWindowMonitor`, `_window.WindowState` correctly reports +`Fullscreen`. Silk's `_extendedState`-style caching only applies before +`IsInitialized`. + +**No readers to misbehave.** `grep` over `src/` and `tests/` finds **zero** +`WindowState =` writers left after this commit, and the only consumer is +`DisplayFramePacingController.OnWindowStateChanged(WindowState _)` +(`Rendering/DisplayFramePacingController.cs:132`), which discards the value +and calls `RefreshActiveMonitor()`. UI/picking read `IWindow.Size` +(`Composition/InteractionUiRuntimeSources.cs:559`) and the render path reads +`IWindow.FramebufferSize` (`Rendering/RetailPViewPassExecutor.cs:27`, +`Rendering/Gpu/Vk/VulkanGraphicsContext.cs:261, 406`) — both are live native +reads (`get_FramebufferSize` = `GetFramebufferSize`), both correct under +native fullscreen. There is no screenshot path that reads `WindowState`. + +**Events fire on both paths, and directly.** Silk raises them straight from +the GLFW callback with **no queueing** — +`GlfwWindow::b__94_2` is +`ldsfld FramebufferResize; newobj Vector2D; callvirt Invoke`, and +`b__94_0` (pos) is `UpdatePosition; ldsfld Move; Invoke`. Since +`glfwSetWindowMonitor` calls `SetWindowPos`, the WM_SIZE / WM_MOVE handlers +run synchronously inside it, so `FramebufferResize` and `Move` are delivered +**re-entrantly**, on both enter and exit. (Confirmed safe: see W1/W8.) + +**`StateChanged` does NOT fire** for a monitor change — `b__94_6`/`b__94_7` +are the iconify/maximize callbacks and are the only sites that call +`UpdateState` + raise `StateChanged`. `pacing.OnWindowStateChanged` therefore +never runs for our switch; `pacing.OnWindowMoved` covers it instead +(`SilkWindowCallbackBinding.cs:152`). No action needed — just do not build +anything new on `StateChanged`. + +**W3 (latent trap):** `GlfwWindow::set_CoreWindowState` is the only writer of +Silk's `_nonFullscreenPosition` / `_nonFullscreenSize`. Bypassing it means +those stay stale forever. If any future code sets `WindowState = Normal` +while natively fullscreen, Silk calls `glfwRestoreWindow`, which for a +fullscreen window restores the *video mode on the same monitor* — it does +**not** leave fullscreen. Worth a one-line comment in +`DisplayModeSwitching.cs` so the next author does not reach for the Silk +setter as a "simpler" exit. + +### Q4 — The exit path + +- **`(60,60)` default:** reachable in production, via M3 (not via the + "never captured" path the question hypothesised — startup entry *does* + capture, just on the other instance). +- **Off-screen stranding proper:** unlikely. `glfwSetWindowMonitor(NULL, …)` + restores the monitor's original video mode, so a position captured + pre-fullscreen is still valid for the restored desktop. `(60,60)` is always + on-screen on the primary. +- **W5 (real, milder):** the exit uses the *picked resolution* as the + **client** size. The catalog's Defaults value **is the desktop mode** + (`DisplayModeCatalog.cs:39-43`), so the common case produces a windowed + client exactly the size of the desktop — its frame and title bar then + overflow the work area, and row 11 of the Q2 table (unparseable resolution) + produces the same via `_window.Size`. Recommend clamping the restored + placement/size to the monitor work area + (`glfwGetMonitorWorkarea`) on the exit path. +- **#390 interaction — confirmed, the UI clamp does NOT cover the OS + window.** `RetailWindowLayoutPersistence.ClampAllToScreen` + (`src/AcDream.App/UI/RetailWindowLayoutPersistence.cs:102-129`) clamps each + attached retail-UI window into `ValidScreenSize()` — the client area — via + `handle.MoveTo`. It never touches the native window. So #390 keeps the + *panels* reachable inside whatever client rect exists; it cannot rescue a + client rect that itself overflows the desktop. That is the right layering; + it just means W5 needs its own fix rather than leaning on #390. + +### Q5 — Validation seam + +- **Are valid sub-desktop fullscreen modes wrongly refused?** Not by the + curation itself. `DisplayModeCatalog.Curate` (`:94-144`) keeps any monitor + mode that fits the desktop, is ≥1280 wide and is 16:9/16:10/21:9/32:9 + within ±2.5% — plus the desktop mode unconditionally. Those are all genuine + fullscreen targets, and `TryFindRefreshRate` re-checks against the live + GLFW mode list, so nothing supported-and-offered is refused **on a + single-monitor machine**. On multi-monitor, **M4** breaks it. +- **Catalog-uninstalled (fixture/headless):** refuses cleanly, no crash. + `DisplayModeCatalog.Resolutions` is `null`, and + `RuntimeSettingsTargets.cs:83` is + `spec => DisplayModeCatalog.Resolutions?.Contains(spec) == true` — a + null-conditional whose `null == true` is `false`. Fullscreen entry is + refused with a log; nothing dereferences null. **PASS.** +- **Nit (not blocking):** `Contains` is `Enumerable.Contains` over an + `IReadOnlyList` (ordinal, O(n≈10)) — fine. The spec string is + rebuilt from the parsed ints (`$"{width}x{height}"`), so + `"01920x1080"`-style input normalises before lookup. `TryParseResolution` + splits on lowercase `'x'` only (`:165`) — pre-existing. +- **W7:** the catalog is captured once at `OnLoad` + (`GameWindow.cs:1246`) and never refreshed; monitor hot-plug or a topology + change leaves it stale, and (with M4) increasingly wrong. + +### Q6 — #377 regression surface at startup + +**The ordering is safe. PASS.** Verified chain inside `OnLoad`: + +1. `AcquirePlatform()` → `VulkanGraphicsContext.Acquire` — swapchain created + at the 1280x720 startup size (`GameWindow.cs:1219-1238`, `:752-757`). +2. `DisplayModeCatalog.InstallFromWindow(_window!)` — `GameWindow.cs:1246`, + **before** the pipeline, so the startup fullscreen entry is validated + against a real catalog. +3. Composition **phase 1** binds the framebuffer targets: + `FramebufferResize.BindViewport` (`HostInputCameraComposition.cs:204`) and + `BindCamera` (`:317`). +4. Composition **phase 3** runs `Settings.ApplyStartup` + (`SettingsDevToolsComposition.cs:53`) → `ApplyDisplay` → native switch. + +So the re-entrant `FramebufferResize` fired from inside +`glfwSetWindowMonitor` finds both targets bound — +`FramebufferResizeController.Resize` (`:109-129`) is null-conditional +throughout, and the Vulkan viewport target only **arms** a frame-boundary +swapchain recreate (`VulkanHostInputCameraCompositionFactory.cs:38, 122`) +rather than doing GPU work re-entrantly. Had step 4 preceded step 3, the +resize would have been silently dropped ("late binding never replays an +earlier resize transition", `FramebufferResizeController.cs:123-124`) and +the client would have rendered a 1280x720-aspect image stretched over the +fullscreen surface — the #387 symptom, resurrected at startup. **This +ordering is load-bearing and undocumented; it deserves a comment at +`GameWindow.cs:1246` or a boundary test.** + +Re-entrancy itself cannot deadlock: `HostQuiescenceGate.Invoke` uses `lock` +(`Rendering/HostQuiescenceGate.cs:39, 49`), and `Monitor` is reentrant on the +same thread — the class doc already calls this out. + +**W1 (the residual, and the honest answer to "could this resurrect the old +access violation"): yes, the mechanism is still reachable — more often than +before.** The re-entrant `Move` event runs +`pacing.OnWindowMoved` → `RefreshActiveMonitor()` → +`SilkDisplayFramePacingSurface.TryGetActiveMonitorRefreshHz` → +`_window.Monitor?.VideoMode.RefreshRate` +(`DisplayFramePacingController.cs:35, 121-128`) **from inside +`glfwSetWindowMonitor`, mid-mode-transition**. `GlfwWindow::get_Monitor` +resolves through `GetWindowMonitor` → `GetMonitors` → `new GlfwMonitor` and +`GlfwMonitor.VideoMode` → `GetVideoMode` — which is precisely #377's crash +site (`0xC0000005 in Glfw.GetVideoMode`, "reading a monitor +mid-mode-transition", `docs/ISSUES.md` §#377). Its `catch (GlfwException)` +guard (`:42-49`) does not help: per **M1** GLFW errors do not throw on +Windows, and an access violation is not a managed exception at all. + +This is not a *new* class — Silk's own `WindowState = Fullscreen` setter also +called `SetWindowMonitor` — but before this commit the fullscreen write was +guarded by `if (_window.WindowState != desired)` and therefore rare. With +**M2**, it now runs on every Display-tab save and every slider drag tick. +Recommendation: suppress the pacing monitor refresh for the duration of a +deliberate mode switch (a re-entrancy latch around +`SetWindowMonitor`, then one explicit `RefreshActiveMonitor()` after it +returns) — that is a correctness fix for a re-entrancy hazard, not a +symptom-hiding guard. + +--- + +## Test-coverage gaps (all five new tests pass on their own facts) + +`tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs:157-288` +covers: validated switch / never-size-write, unoffered refusal, failed-switch +usability, native exit, plain windowed write. Missing, each one assertion +away with the existing `FakeModeSwitcher`: + +1. `Fullscreen=true` while `IsFullscreen=true` **at the same mode** → expect + `Assert.Empty(switcher.Calls)` (**M2**). +2. `Fullscreen=true` with an unparseable `Resolution` → currently a silent + `return`; assert whatever the decided behaviour is (**M5**). +3. Enter-then-exit round trip on **one** switcher instance, asserting the + position is round-tripped (would not catch **M3**, which is a composition + fact — that one needs a construction-site assertion or a single-owner + refactor). +4. `Fullscreen=false` while `IsFullscreen=true` with an unparseable + resolution → asserts the row-11 fallback reads the surface size. + +## Gate-script gaps + +`docs/research/2026-08-13-display-block-test-script.md` §D4–§D6 is otherwise +well-shaped (the black-screen-risk flagging is right). Add: + +- §D5 must include **launch fullscreen → untick Full Screen** as a distinct + step from **tick → untick in one session**; only the former exposes **M3**. +- §D6 must include **drag a slider (Field of View) while fullscreen** — the + **M2** storm. +- §D6's "any refused/failed switch logs a `display: … failed/refused` line" + is not currently guaranteed (**M1** makes failure invisible, **M5** case 4 + logs nothing). Treat that acceptance line as unmet until M1/M5 land. + +## Register / bookkeeping + +- No `docs/architecture/retail-divergence-register.md` row accompanies this + commit. At minimum the **always-switch-the-primary-monitor** behaviour + (**M4**) is a deviation ("retail's `ForceDisplayResolution` drove the + primary display device" is asserted in the class doc but the *window* also + relocates), and the CLAUDE.md rule is that a deviation ships with its row + in the same commit. +- On Wayland (`GraphicalDisplayProtocol.Wayland`), a client cannot change the + display mode; `glfwSetWindowMonitor` will report success without switching. + Slice L is parked, so this is documentation-only today — one line in the + `GlfwDisplayModeSwitcher` class doc. diff --git a/src/AcDream.App/Settings/DisplayModeSwitching.cs b/src/AcDream.App/Settings/DisplayModeSwitching.cs index 4776923d..85833040 100644 --- a/src/AcDream.App/Settings/DisplayModeSwitching.cs +++ b/src/AcDream.App/Settings/DisplayModeSwitching.cs @@ -20,6 +20,12 @@ internal interface IDisplayModeSwitcher /// monitor attached). bool IsFullscreen { get; } + /// The active fullscreen mode's WxH, or null when windowed. + /// The apply's idempotence guard (review M2: every Display-backed Config + /// row applies per change — sliders per DRAG TICK — so a same-mode + /// re-apply must be a no-op before any native call). + (int Width, int Height)? CurrentFullscreenMode { get; } + /// Switches the window to exclusive fullscreen at the given /// mode. False (with a reason) instead of throwing on any failure — /// a settings apply must never crash the client (#388). @@ -32,19 +38,38 @@ internal interface IDisplayModeSwitcher /// /// The native GLFW implementation. Uses the SAME handle path the #348 -/// cursor cache proved (IWindow.Native.Glfw) and the primary -/// monitor — retail's ForceDisplayResolution likewise drove the -/// primary display device. The refresh rate for a mode is the highest the -/// monitor reports for that WxH (retail passed the device's mode as-is). -/// Callers validate the requested WxH against DisplayModeCatalog -/// BEFORE calling — an offered mode is supported by construction, which is -/// what makes the old "Graphics mode not supported" crash class -/// unreachable from the dropdown. +/// cursor cache proved (IWindow.Native.Glfw) and the WINDOW'S OWN +/// monitor (IWindow.Monitor's index into the GLFW monitor array — +/// the same monitor DisplayModeCatalog enumerated, so the offered +/// list and the switch target agree; review M4 caught the earlier +/// primary-monitor mismatch). The refresh rate for a mode is the highest +/// the monitor reports for that WxH. Callers validate the requested WxH +/// against DisplayModeCatalog BEFORE calling. +/// +/// Error model (review M1 — load-bearing): on Windows, Silk's +/// GLFW error callback does NOT throw — it QUEUES the exception on a static +/// list that detonates much later (window close), which is exactly the +/// two-stage crash #388 originally recorded. `catch (GlfwException)` is +/// therefore dead code on this platform; success is judged by the NATIVE +/// POST-CONDITION (does the window have / not have a monitor afterwards), +/// never by the absence of an exception. The catches stay only for the +/// non-Windows platforms where Silk does throw. /// internal sealed unsafe class GlfwDisplayModeSwitcher : IDisplayModeSwitcher { private readonly IWindow _window; - private (int X, int Y) _windowedPosition = (60, 60); + + // Q1 nit: one cached wrapper (a Glfw.GetApi() per call allocates and + // takes a native library refcount) — the same pattern GlfwCursorCache + // uses. + private static readonly Lazy Api = new(Glfw.GetApi); + + // Review M3/M5: the remembered windowed placement is PROCESS state, not + // per-instance state — the startup target and the live-save target are + // two instances over the one window, and a fullscreen boot must exit + // through whichever instance the untick reaches. Static, single-window + // process (the same write-once shape DisplayModeCatalog uses). + private static (int X, int Y) _windowedPosition = (60, 60); public GlfwDisplayModeSwitcher(IWindow window) { @@ -55,9 +80,40 @@ internal sealed unsafe class GlfwDisplayModeSwitcher : IDisplayModeSwitcher { get { - WindowHandle* handle = Handle(); - if (handle is null) return false; - return Glfw.GetApi().GetWindowMonitor(handle) is not null; + try + { + WindowHandle* handle = Handle(); + if (handle is null) return false; + return Api.Value.GetWindowMonitor(handle) is not null; + } + catch (GlfwException) + { + // Never throws on Windows (queued instead) — this guard is + // for the throwing platforms; a monitor query must never + // take the caller down (#377 family). + return false; + } + } + } + + public (int Width, int Height)? CurrentFullscreenMode + { + get + { + try + { + WindowHandle* handle = Handle(); + if (handle is null) return null; + Glfw glfw = Api.Value; + Silk.NET.GLFW.Monitor* monitor = glfw.GetWindowMonitor(handle); + if (monitor is null) return null; + Silk.NET.GLFW.VideoMode* mode = glfw.GetVideoMode(monitor); + return mode is null ? null : (mode->Width, mode->Height); + } + catch (GlfwException) + { + return null; + } } } @@ -73,11 +129,11 @@ internal sealed unsafe class GlfwDisplayModeSwitcher : IDisplayModeSwitcher try { - Glfw glfw = Glfw.GetApi(); - Silk.NET.GLFW.Monitor* monitor = glfw.GetPrimaryMonitor(); + Glfw glfw = Api.Value; + Silk.NET.GLFW.Monitor* monitor = ResolveWindowMonitor(glfw, handle); if (monitor is null) { - error = "no primary monitor"; + error = "no monitor"; return false; } @@ -96,14 +152,22 @@ internal sealed unsafe class GlfwDisplayModeSwitcher : IDisplayModeSwitcher } glfw.SetWindowMonitor(handle, monitor, 0, 0, width, height, refresh); + + // Review M1: judge by the native post-condition, not by the + // absence of an exception (Windows queues GLFW errors). + if (glfw.GetWindowMonitor(handle) is null) + { + error = "the mode switch did not take (GLFW reports no monitor attached)"; + return false; + } + Console.WriteLine( $"display: fullscreen mode switch {width}x{height}@{refresh}"); return true; } catch (GlfwException ex) { - // #388: a failed switch is a logged failure the caller reverts - // from, never a crash. Validation makes this path exceptional. + // Non-Windows platforms throw; Windows never reaches here. error = ex.Message; return false; } @@ -121,13 +185,20 @@ internal sealed unsafe class GlfwDisplayModeSwitcher : IDisplayModeSwitcher try { - Glfw glfw = Glfw.GetApi(); + Glfw glfw = Api.Value; if (glfw.GetWindowMonitor(handle) is null) return true; // already windowed glfw.SetWindowMonitor( handle, null, _windowedPosition.X, _windowedPosition.Y, width, height, 0); + + if (glfw.GetWindowMonitor(handle) is not null) + { + error = "the window is still fullscreen (GLFW reports a monitor attached)"; + return false; + } + Console.WriteLine( $"display: left fullscreen to windowed {width}x{height}"); return true; @@ -139,8 +210,30 @@ internal sealed unsafe class GlfwDisplayModeSwitcher : IDisplayModeSwitcher } } + /// The monitor the window belongs to: the attached monitor when + /// fullscreen, else the window's own monitor per Silk's assignment + /// (index into the GLFW monitor array — the SAME monitor + /// DisplayModeCatalog.InstallFromWindow enumerated), falling back + /// to the primary. + private Silk.NET.GLFW.Monitor* ResolveWindowMonitor(Glfw glfw, WindowHandle* handle) + { + Silk.NET.GLFW.Monitor* attached = glfw.GetWindowMonitor(handle); + if (attached is not null) return attached; + + int? index = _window.Monitor?.Index; + if (index is int i && i >= 0) + { + Silk.NET.GLFW.Monitor** monitors = glfw.GetMonitors(out int count); + if (monitors is not null && i < count) + return monitors[i]; + } + return glfw.GetPrimaryMonitor(); + } + /// The monitor's highest refresh rate for an exact WxH, or - /// false when the monitor does not report the mode at all. + /// false when the monitor does not report the mode at all. (Register + /// row AD-92: retail passed the device mode's own refresh as-is; + /// highest-for-WxH is a deliberate adaptation.) private static bool TryFindRefreshRate( Glfw glfw, Silk.NET.GLFW.Monitor* monitor, int width, int height, out int refresh) { diff --git a/src/AcDream.App/Settings/RuntimeSettingsTargets.cs b/src/AcDream.App/Settings/RuntimeSettingsTargets.cs index 6683ba48..c5b77d05 100644 --- a/src/AcDream.App/Settings/RuntimeSettingsTargets.cs +++ b/src/AcDream.App/Settings/RuntimeSettingsTargets.cs @@ -45,13 +45,20 @@ internal interface IRuntimeChatOpacityTarget void Apply(float defaultOpacity, float activeOpacity); } -/// The one window property the display apply touches — a narrow +/// The window properties the display apply touches — a narrow /// seam so the #388 state machine is testable without faking all of /// (same idiom as FakePacingSurface's /// surface). internal interface IWindowedSizeSurface { Vector2D Size { get; set; } + + /// #388 blast M3: a maximized window silently ignores a Size + /// write — the apply un-maximizes first (the deleted + /// WindowState = Normal write used to do this incidentally). + bool IsMaximized { get; } + + void Restore(); } internal sealed class SilkWindowSizeSurface(IWindow window) : IWindowedSizeSurface @@ -64,6 +71,10 @@ internal sealed class SilkWindowSizeSurface(IWindow window) : IWindowedSizeSurfa get => _window.Size; set => _window.Size = value; } + + public bool IsMaximized => _window.WindowState == WindowState.Maximized; + + public void Restore() => _window.WindowState = WindowState.Normal; } internal sealed class SilkRuntimeDisplayWindowTarget : IRuntimeDisplayWindowTarget @@ -76,11 +87,14 @@ internal sealed class SilkRuntimeDisplayWindowTarget : IRuntimeDisplayWindowTarg : this( new SilkWindowSizeSurface(window), new GlfwDisplayModeSwitcher(window), - // #391's catalog is the validation source: an offered mode is - // supported by construction. With no catalog installed - // (fixture/uninitialised hosts) nothing is "offered", so - // fullscreen entry is refused rather than guessed. - spec => Rendering.DisplayModeCatalog.Resolutions?.Contains(spec) == true) + // #391's catalog is the validation source. With no catalog + // installed, the dropdown falls back to the static preset + // ladder — the validator must fall back to the SAME list + // (blast M2: an asymmetric fallback made Full Screen a permanent + // silent no-op on catalog-less hosts). The switcher's own + // monitor-mode-list check remains the hard guard either way. + spec => (Rendering.DisplayModeCatalog.Resolutions + ?? DisplaySettings.AvailableResolutions).Contains(spec)) { } @@ -116,6 +130,19 @@ internal sealed class SilkRuntimeDisplayWindowTarget : IRuntimeDisplayWindowTarg if (display.Fullscreen) { if (!haveResolution) + { + // Mechanism M5: never a SILENT bail-out — §D6's acceptance is + // "any refused/failed switch logs a line". + Console.WriteLine( + $"display: fullscreen refused — unparseable resolution '{display.Resolution}'"); + return; + } + // Mechanism/blast M2: idempotence BEFORE any native work — every + // Display-backed Config row applies per change (sliders per drag + // tick), and only this guard keeps those from re-issuing a real + // display-mode change per mouse sample. + if (_modeSwitcher.CurrentFullscreenMode is (int curW, int curH) + && curW == width && curH == height) return; if (!_isOfferedMode.Invoke($"{width}x{height}")) { @@ -125,7 +152,7 @@ internal sealed class SilkRuntimeDisplayWindowTarget : IRuntimeDisplayWindowTarg } if (!_modeSwitcher.TryEnterFullscreen(width, height, out string? error)) Console.WriteLine( - $"display: fullscreen {width}x{height} failed ({error}) — staying windowed"); + $"display: fullscreen {width}x{height} failed ({error}) — window state unchanged (#392 tracks the persisted-flag divergence)"); return; } @@ -146,6 +173,11 @@ internal sealed class SilkRuntimeDisplayWindowTarget : IRuntimeDisplayWindowTarg if (haveResolution && (_window.Size.X != width || _window.Size.Y != height)) { + // Blast M3: a maximized window ignores Size writes — restore + // first (the deleted WindowState=Normal write did this + // incidentally; now it is explicit and only-when-needed). + if (_window.IsMaximized) + _window.Restore(); // #387 evidence line (permanent): the resolution-pick write path. Console.WriteLine( $"display: resolution pick {width}x{height} " + diff --git a/tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs b/tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs index a955de6f..7ae6e36d 100644 --- a/tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs +++ b/tests/AcDream.App.Tests/Settings/RuntimeSettingsControllerTests.cs @@ -160,16 +160,26 @@ public sealed class RuntimeSettingsControllerTests { public Silk.NET.Maths.Vector2D Size { get; set; } = new(1280, 720); public int Writes { get; private set; } + public bool IsMaximized { get; set; } + public int Restores { get; private set; } + Silk.NET.Maths.Vector2D IWindowedSizeSurface.Size { get => Size; set { Size = value; Writes++; } } + + public void Restore() + { + IsMaximized = false; + Restores++; + } } private sealed class FakeModeSwitcher : IDisplayModeSwitcher { public bool IsFullscreen { get; set; } + public (int Width, int Height)? CurrentFullscreenMode { get; set; } public bool EnterSucceeds { get; set; } = true; public List Calls { get; } = []; @@ -177,7 +187,11 @@ public sealed class RuntimeSettingsControllerTests { Calls.Add($"enter:{width}x{height}"); error = EnterSucceeds ? null : "injected failure"; - if (EnterSucceeds) IsFullscreen = true; + if (EnterSucceeds) + { + IsFullscreen = true; + CurrentFullscreenMode = (width, height); + } return EnterSucceeds; } @@ -186,10 +200,74 @@ public sealed class RuntimeSettingsControllerTests Calls.Add($"leave:{width}x{height}"); error = null; IsFullscreen = false; + CurrentFullscreenMode = null; return true; } } + [Fact] + public void DisplayApply_SameFullscreenMode_IsANoOp_BeforeAnyNativeWork() + { + // Review M2: sliders apply per drag tick; without this guard every + // tick while fullscreen re-issued a real display-mode change. + var surface = new FakeSizeSurface(); + var switcher = new FakeModeSwitcher + { + IsFullscreen = true, + CurrentFullscreenMode = (1920, 1080), + }; + var target = new SilkRuntimeDisplayWindowTarget( + surface, switcher, _ => true); + + target.Apply(DisplaySettings.Default with + { + Fullscreen = true, + Resolution = "1920x1080", + }); + + Assert.Empty(switcher.Calls); + Assert.Equal(0, surface.Writes); + } + + [Fact] + public void DisplayApply_UnparseableResolutionWhileFullscreen_RefusesWithoutCalls() + { + var surface = new FakeSizeSurface(); + var switcher = new FakeModeSwitcher(); + var target = new SilkRuntimeDisplayWindowTarget( + surface, switcher, _ => true); + + target.Apply(DisplaySettings.Default with + { + Fullscreen = true, + Resolution = "garbage", + }); + + Assert.Empty(switcher.Calls); + Assert.Equal(0, surface.Writes); + } + + [Fact] + public void DisplayApply_MaximizedWindowedPick_RestoresBeforeTheSizeWrite() + { + // Blast M3: a maximized window silently ignores Size writes; the + // deleted WindowState=Normal write used to un-maximize incidentally. + var surface = new FakeSizeSurface { IsMaximized = true }; + var switcher = new FakeModeSwitcher(); + var target = new SilkRuntimeDisplayWindowTarget( + surface, switcher, _ => true); + + target.Apply(DisplaySettings.Default with + { + Fullscreen = false, + Resolution = "1600x900", + }); + + Assert.Equal(1, surface.Restores); + Assert.Equal(1, surface.Writes); + Assert.False(surface.IsMaximized); + } + [Fact] public void DisplayApply_FullscreenPick_IsAValidatedModeSwitch_NeverASizeWrite() {