Repository navigation
Conversation
Confetti3
left a comment
There was a problem hiding this comment.
Thanks for this. Keeping the layout and resolution policy pure (BorderlessLayout.hxx, DisplayPreferences.hxx) with tests, configuring the engine's own render size instead of stretching the window, and reusing the atomic launcher settings store are all the right calls. Reviewed at f93b845. The main thing to change is that links are lost while the new Home screen is open; the rest is smaller.
1. Room, tournament, public room, connect and Discord links are lost while Home is open. The launcher takes the single-instance lock at src/launcher/launcher.cxx:606 and then waits in Home at src/launcher/launcher.cxx:663. A link opened in that time starts a second launcher, which fails Acquire() and tries DeliverJoinLink and the other three. Those mailboxes are only created by the game (src/sf4e/sf4e__NetplayRuntime.hxx:177, :180, :183, :253), so OpenFileMappingW fails, the second launcher shows "already running" and exits, and the first launcher never hears about the link. A --discord-launch start is quieter still: it skips the message (launcher.cxx:636), and ember-discord.exe only starts with the game (launcher.cxx:334), so an accepted invite does nothing visible. Before this PR the lock was held without a game only for a moment at startup; now it is the normal state after every double-click. SlotMailbox<> is a header template, so Home can open the same four mailboxes itself, check Take() once per loop, and on a link leave Home and start with it through the existing SF4E_JOIN_LINK / SF4E_MATCH_LINK / SF4E_PUBLIC_ROOM_LINK / SF4E_CONNECT_LINK handoff (launcher.cxx:679-688). Close them before CreateSF4Process so the game's Open() owns the sections. For Discord, either have the second start set a flag Home watches, or start the game directly when Home is open and a Discord launch arrives. A test in the style of src/tests/join_link_test.cxx, with private section names, would cover a link arriving while Home is listening.
2. A plain double-click now depends on the launcher's D3D9 window. Home goes through RunRecovery, which shows recovery.d3d_failed and returns false when it cannot create its device (src/ui/RecoverySurface.cxx:88), and launcher.cxx:663 then returns without starting the game. Before this PR that player got the game. Please fall through to a normal start when the Home window cannot be created.
3. Windowed sizing uses the window's current DPI. src/sf4e/sf4e__Borderless.cxx:121 expands the physical client rectangle with GetDpiForWindow(window). If the game window is DPI unaware (which the windowed-unaware test variant assumes), that is always 96, while the frame Windows draws is scaled to the monitor, so at 150% or 200% the client comes out smaller than selection.window by the scaled frame and caption. For an aware window it is the DPI of the monitor the window is on now, not the one it is being moved to. The early return at :122 compares outer sizes, so it accepts the wrong result. Borderless and Fullscreen do not reach this line. Please compute the frame for the destination monitor and the window's awareness, then check the actual client rectangle after SetWindowPos. The windowed-unaware test only proves something on a scaled desktop; at 100% it passes either way.
4. A failed window setup leaves the override half applied. Prepare has already changed the adapter, render size, fullscreen flag and refresh (sf4e__Borderless.cxx:186 onwards) before the window work. If PostMessageW fails (:214) it only logs, and if the style or position change fails (:132) only the styles are restored. prepared stays true, so Alt+Enter is still swallowed (:229) although the layout was never installed. These calls failing on the game's own window is unlikely, so this is a robustness point, but the fix is small: on failure, record it, stop intercepting Alt+Enter and stop requesting further applies, so the player keeps the native controls.
5. Utf8 in src/platform/DisplaySettings.cxx:10 duplicates platform::WideToUtf8. It also skips WC_ERR_INVALID_CHARS and ignores the result of the second WideCharToMultiByte. Please use the one in src/platform/Utf8.hxx and delete this.
6. Suggestion: name the launcher screens instead of adding flags. RunRecovery and DrawRecoveryMenu now take home and openDisplay alongside updates and canStart (src/ui/RecoverySurface.hxx:14, src/ui/RecoveryMenu.hxx:32), and launcher.cxx:663 passes five positional booleans. Some combinations make no sense together, and Home now changes row order, title, close text and back hint inside the recovery renderer. A small Home / Recovery / Updates root plus an initial page, keeping the shared window and loop, would read better and keep Display an ordinary child page.
7. Suggestion: one owner for the override's state. The lifecycle is spread over prepared, native.graphics, applied, applying and bars (sf4e__Borderless.cxx:34-37). Holding the plan, the native snapshot and the window outcome in one object would make item 4 a single cancel path and make it clear which thread owns what.
Still to check on a desktop: a room link and a Discord invite opened while Home is up, Windowed at 150% and 200% scaling, a secondary monitor at a different scale, and whether Borderless is sharp on a scaled display with the game window DPI unaware.
|
Cross-check of the engine side of this PR against an independent static read of the Steam The offsets agree. Derived separately, the same function and fields come out:
These four are entries [3], [4], [5] and [12] of a 13-dword display block that starts at
Why proportional is the right scope. The HUD classes have no aspect-ratio logic, and the gameplay screen edge is a fixed distance either side of the camera. Real ultrawide would need both changed, so "16:9 image with bars" matches what the engine can do without touching the simulation. The game's own mode list. At device start the game keeps only display modes with width/height between 1.25 and 1.8. Writing the fields directly sidesteps that for Windowed and Borderless. For exclusive Fullscreen the mode still has to be one the adapter offers, which the PR already restricts to. What To test: the in-game display options. Applying a change in the game's own graphics options copies the framework's values back over the device block (through the function that validates the 13-dword block). To test: DXVK / Wine. One observation from a run today on Windows with DXVK 3.1.1 under the plain game (no Ember): after Alt+Enter from fullscreen back to windowed, the window stayed borderless and could not be returned to a normal window. Blocking Alt+Enter and fixing the mode per launch, as this PR does, avoids that path. Since Ember supports Wine, one run of each mode under DXVK or Proton would be useful; the validation record here is native D3D9 only. Overlap. #74 also edits 🤖 Generated with Claude Code |
|
Correction to my comment above, section "What What the code does
What follows, if the above is right With an override active, the overridden width, height, refresh and fullscreen flag are copied into the framework's settings at boot and written to One thing I could not resolve: at shutdown the display keys are written only when framework How to check on a desktop Note Possible fixes
The rest of the earlier comment (offsets, adapter field, mode filter) I have not found reason to change. 🤖 Generated with Claude Code |
Extract this focused change from the locally tested development build. Keep other features out of the upstream review; the pull request records implementation details, prior integration evidence, and outstanding isolated validation.
f93b845 to
6f23240
Compare
Keep every key and translated value intact while avoiding unrelated PR additions at the shared country-list tail.
Keep every key and translated value intact while avoiding unrelated PR additions at the shared country-list tail.
Adds saved Native, Windowed, Borderless and advertised exclusive-Fullscreen choices, with monitor, render resolution and refresh selection before game startup. Automatic Borderless on 3440×1440 renders a proportional 2560×1440 image with 440-pixel side bars. This does not widen the gameplay camera or change the simulation.
Review dependency
Please review/merge #98 before this PR. Its branch is incorporated here to resolve the shared implementation; the PR still targets
release. Review only this feature relative to #98. The tested path preserves merge history. A simulated squash merge of the prerequisite produced conflicts in this dependent branch: if upstream squashes/rebases it, refresh this branch against the resultingreleasehead before merging. Future edits to shared files also require rechecking.Review changes
Native config correction
FRaccie's correction was checked against the installed Steam build 834219 executable, not assumed from the earlier comment. The existing writer at 0x77d5f0 reads framework +0xe8/+0xec/+0xf0/+0x10c. The framework setter copies the 13-dword display block, and the brightness path can feed it overridden device values.
The existing upstream Main::WriteDisplayConfig hook now temporarily supplies the captured native width, height, refresh and fullscreen values while saving, then restores the live override. It composes with upstream's RenderingThread protection and leaves unrelated graphics options alone. Restoring D3D fields on Destroy is no longer described as config-write protection. The adapter field at +0x08 is named adapterOrdinal.
Rebased on release cd4daa5, preserving replay launch/handoff, current crash reporting, render capture and current platform hooks. Detailed boundaries and manual checks are in DISPLAY_VALIDATION.md.
Shared launcher lifecycle
Home consumes the common
LauncherLinksowner introduced by #98. The separateLauncherHomeLinksimplementation is removed, so startup updates, recovery and Home share mailbox lifetime and validation. Home keeps its received destination in the launcher's pending state; it reaches the existing launch environment after display preferences have been applied. The shared IPC regression lives with #98; this PR retains the startup-only UI-failure fallback regression.Validation of the current revision
PR head:
9e5e4ced611b98fe58fac7a8334069142f7bb52d. The SDK-free Windows x86 RelWithDebInfo Launcher, Updater and Sidecar, together with the pinned x64 networking helper, built successfully in the combined integration tree02a982a42db7d710aae3babb590b71907a6104e4.This is focused validation of these changes and their integration, not a new full development-gate receipt. Discord SDK-enabled builds, native game/controller sessions, clean-machine installation and local Linux execution remain outside this run.
Integration and merge order
Checked against release
cd4daa5c2fb19ee5a3ecd14d0e2d9cbee753aa4f. All 136 pairs among the 17 current contribution branches merge cleanly. Three complete merge orders produce the same tree02a982a42db7d710aae3babb590b71907a6104e4. The seven revised branches were also compared with all 15 other open developer branches (105 comparisons): these fixes introduce no new conflicting pairs or additional conflict files relative to their previous heads.This PR already overlapped #95, #89, #88, #76, #37 and #35 before these fixes. Those existing overlaps remain and must be reconciled with the actual target branch if that work lands first. Published heads were checked; unpushed work cannot be assessed. The integration results preserve branch history; after a squash/rebase merge of a shared prerequisite, refresh affected dependent branches and rerun their checks before merging.
Remaining acceptance
Real accepted Discord invitations and native game link startup from Home; Windows desktop scaling at 150%/200% and different scales across monitors; in-game graphics-option/save/quit verification; visible focus/sharpness/exclusive mode behavior; and DXVK/Wine/Proton. The full Discord SDK is not available on this machine. The new display text still uses the feature's English fallback in non-English catalogs and needs localization review.
These results establish the code and fixture changes above, not universal driver or native-game acceptance.