fix #376/#388 review round: post-condition truth, idempotence, one
position memory, unified monitor, maximized restore; AD-92
Dual-lens Opus review of e56aa511 (reports committed under
docs/research/). The consolidated corrections:
- Mechanism M1 (load-bearing): on Windows, Silk's GLFW error callback
QUEUES exceptions on a static list instead of throwing - they detonate
later at window close, which is exactly #388's original two-stage
crash shape. catch(GlfwException) was dead code here and a failed
SetWindowMonitor "succeeded". Success is now judged by the NATIVE
POST-CONDITION (GetWindowMonitor after the call) on both enter and
exit; the catches remain only for the throwing platforms.
- M2 (both lenses): same-mode fullscreen re-apply is a no-op BEFORE any
native work (new IDisplayModeSwitcher.CurrentFullscreenMode). Every
Display-backed Config row applies per change - sliders per DRAG TICK -
so without this every tick while fullscreen re-issued a real
display-mode change.
- M3/M5 (both): the remembered windowed placement is process state (two
target instances exist - startup and live-save); a fullscreen boot now
exits through either instance to the real placement, not the (60,60)
literal.
- M4 (both): the switcher resolves the WINDOW'S monitor (attached
monitor when fullscreen, else IWindow.Monitor's index into the GLFW
array - the same monitor DisplayModeCatalog enumerated), primary only
as a last resort; the offered-list/switch-target mismatch is gone.
- Blast M2b: the offered-mode validator falls back to the SAME static
ladder the dropdown falls back to - Full Screen is no longer a
permanent silent no-op on catalog-less hosts (the switcher's own
monitor-mode-list check remains the hard guard).
- Blast M3: a windowed pick on a MAXIMIZED window restores it first
(Size writes are silently ignored while maximized; the deleted
WindowState=Normal write used to do this incidentally). New
IWindowedSizeSurface.IsMaximized/Restore.
- Mechanism M5: no silent bail-outs - the unparseable-resolution
fullscreen path logs, and the failure line no longer claims "staying
windowed" when the state is unchanged (#392 noted inline).
- Q1 nit: one cached Glfw wrapper (per-call GetApi allocated + took a
native refcount); IsFullscreen/CurrentFullscreenMode guarded.
- AD-92: highest-refresh-for-WxH + refuse-and-log versus retail's
pass-through-and-error ForceDisplayResolution.
Known-open tail, filed not hidden: #392 (persisted-flag divergence on a
refused enter - needs an apply-result seam); the mechanism report's
pacing-refresh WATCH rides the same seam.
Tests: +3 (same-mode no-op, unparseable-while-fullscreen refusal,
maximized restore-before-write). App suite 4,975/3 skips.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
229242e1fe
commit
ec2a7b0cce
6 changed files with 1257 additions and 28 deletions
File diff suppressed because one or more lines are too long
497
docs/research/2026-08-13-376-388-blast-review.md
Normal file
497
docs/research/2026-08-13-376-388-blast-review.md
Normal file
|
|
@ -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`.
|
||||
528
docs/research/2026-08-13-376-388-mechanism-review.md
Normal file
528
docs/research/2026-08-13-376-388-mechanism-review.md
Normal file
|
|
@ -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>(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::<RegisterCallbacks>b__94_2` is
|
||||
`ldsfld FramebufferResize; newobj Vector2D<int>; 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<string>` (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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue