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>
497 lines
26 KiB
Markdown
497 lines
26 KiB
Markdown
# 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`.
|