Repository navigation
Conversation
…freshes The Insert menu's Frame generation gets Auto (its value 1, the step it skipped so far). Instead of a whole multiplier, each game frame is given the display's refreshes that fall in its interval (on the game's timeline, a refresh period apart), and each of them a frame made at its place between the two game frames: 45 fps on a 60 Hz display makes one frame for every three of the game's (60 shown a second, at 3/4, 1/2 and 1/4), on 120 Hz 120; at 60 fps on 60 Hz none are made. The display thread shows each at its refresh's time, one game frame later. The sets' frames are made once for the most a game frame may be given (the refresh over 30 fps, 2 to 4). The fixed multipliers are unchanged. The refresh is the window's display's: on Linux SDL's (SDL_GetDisplayForWindow and SDL_GetCurrentDisplayMode; SDL2's SDL_GetWindowDisplayIndex), asked on the main thread once a second and handed over in the window bridge's new refresh_mhz (MKWVK_WINDOW_VERSION 2; a version 1 bridge still starts); on Windows the window's monitor's (EnumDisplaySettingsW). An unknown refresh counts as 60 Hz. tests/test_framegen_auto.py runs the real functions over refresh rates and frame rates; tests/test_framegen_gate.py now counts Auto as asking for interpolation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013mj4MdWTJo5ht4ShZX8Vpp
hckrman101
left a comment
There was a problem hiding this comment.
Nice idea, and the plumbing mostly checks out. What I verified:
- The SDL3 path reads
refresh_rateat offset 20 ofSDL_DisplayMode(displayID, format, w, h, pixel_density, refresh_rate), which is right. The SDL2 struct and theSDL_GetWindowDisplayIndex/SDL_GetCurrentDisplayMode(int, mode*) == 0calls also match, and both lookups are optional, so an older SDL still starts. - Asking SDL on the display thread and handing the value over through an atomic is the right split.
refresh_nowis set beforerenderer_mainstarts, soensure_sizesees a real rate on the first frame. - The bridge version handling is fine: a v1 bridge is still accepted, and
refresh_mhzis only read whenversion >= 2. - Slot count is bounded by
min(fg.mult, kMaxMult), soset->frame[k]never goes past the framesensure_sizeallocated. The Auto early return keeps the same barriers and state updates as the fixed path, and a set withshown == 0is still released.framegen_made_imagestays consistent, because the game's frame can only be the last slot. - Removing the skip of value 1 in
set_rowandmenu_initis all the menu needed. No preset uses 1, and there's no newHOS_OPT_*id or API version bump to collide with. - The new test compiles the real
auto_cap/multiplier/auto_slots, which I like.
Main issues:
-
Auto at a steady 60 fps on 60 Hz interpolates almost every frame and drops some of the game's frames.
game[k]is only true when a refresh slot lands exactly on the game frame's timestamp (t > 0.999). The test hits that because it uses exact integer frame times. With real present jitter (the pacer comment says frames arrive 12 to 28 ms apart), slots drift aroundnow. I re-ran theauto_slotsarithmetic at 60/60 with only ±0.2 ms of jitter: about 95% of shown frames were synthesized (median t ≈ 0.99), and about 14% of game frames got no refresh at all, with their neighbour getting 2. At ±1 ms it's 99% synthesized. So the "60 fps on 60 Hz: none" row in the description doesn't hold in practice. Auto pays for flow + synth on every frame and shows interpolated frames, with their artefacts, where the real frame was available. It also isn't equivalent to Off when the game already matches the display. Snapping slots within some tolerance ofnowto the game's frame (say, within a quarter of a refresh) would fix the common case. The same goes for slots just afterlast, which could reuse the previous frame instead of a near-0 synth. -
The overlay's "game N" undercounts with Auto.
menu_note_frameis only called inside the show loop, so a game frame withshown == 0isn't counted. That happens whenever the game is at or above the display rate, and per (1) also at 60/60 with jitter. The overlay then reports roughly the refresh rate instead of the game's real rate, while the description says each game frame is counted once. Counting game frames where the job is queued, independent of how many refreshes it got, would fix it. -
The PR conflicts with main in
vk_framegen.cpp,vk_main.cppandvk_menu.cpp. Main now waits withwait_until_near()instead ofsleep_for, the bridge check sits next toprep_window = w, andset_rowhasstep_output_choice/stops_at_ends. None of these are hard, but the display-loop hunk needs to keepwait_until_near. Also heads-up: #1088 edits the samevk_windowinitializer indisplay_sdl.c.
Smaller:
- Windows (untested, as you said):
dmDisplayFrequencyis an integer, and many 59.94 Hz modes report 59. Auto would then make 59 frames a second for a ~60 Hz display, which means a repeated refresh roughly every second. Linux SDL3 gives the fractional rate, so it's only a Windows issue.DwmGetCompositionTimingInfo(rateRefresh) orQueryDisplayConfiggives the exact rational rate. - With Auto, the Frame pacing row (and
RX_VK_PACE=0) no longer has any effect, becausetimedignorespace. That's fine, but a word in the help text would save confusion. - The comment above
framegen_wantedstill says "2x and up is the interpolation".
| } | ||
| bool flow = false; | ||
| for (int k = 0; k < n; k++) { | ||
| set->game[k] = t[k] > 0.999f; |
There was a problem hiding this comment.
This only matches when a slot lands exactly on now, which in practice only happens when the timeline restarts. With normal frame jitter at 60/60, nearly every slot ends up at t ≈ 0.95 to 0.99 and gets synthesized, and some game frames get no slot at all (see the review body). Could slots within a tolerance of now (e.g. (now - at[k]) < period / 4) use the game's frame, and skip flow when all of them do?
| menu_note_frame(k == j.set->made); | ||
| // (the overlay's game rate: once a game frame, with its last refresh. Auto may show no frame of the game's own; | ||
| // a game frame given no refresh (the game faster than the display) is not counted) | ||
| menu_note_frame(timed ? k == shown - 1 : k == j.set->made); |
There was a problem hiding this comment.
When shown == 0 this loop never runs, so that game frame isn't counted toward "game N". With Auto at or above the display rate, the overlay then shows about the refresh rate rather than the game's. Maybe count the game frame once per job outside the loop (only the shown counter needs to be per present).
| const int64_t step = interval / shown; | ||
| for (int k = 0; k <= j.set->made; k++) { | ||
| int64_t due = pace ? next_show : 0; | ||
| const bool timed = j.set->shown >= 0; // (Auto: each frame at its own refresh's time) |
There was a problem hiding this comment.
Minor: with timed the pace setting is ignored entirely, so the Frame pacing row does nothing under Auto. That's fine behaviour, but worth mentioning in the row's help text.
| dm.dmSize = sizeof dm; | ||
| if (main_window && GetMonitorInfoW(MonitorFromWindow(main_window, MONITOR_DEFAULTTONEAREST), &mi) && | ||
| EnumDisplaySettingsW(mi.szDevice, ENUM_CURRENT_SETTINGS, &dm) && dm.dmDisplayFrequency > 1) // (0 and 1: the default) | ||
| mhz = (int)dm.dmDisplayFrequency * 1000; |
There was a problem hiding this comment.
dmDisplayFrequency is whole Hz, and 59.94 Hz modes often report 59 here. Auto would then pace 59 frames a second on a ~60 Hz display, which gives one repeated refresh about every second. DwmGetCompositionTimingInfo (rateRefresh numerator/denominator) gives the exact rate. At minimum, treating 59 as 59.94 would cover the common case.
Conflicts: the display loop keeps wait_until_near, the bridge check keeps prep_window = w (and takes versions 1 and 2), set_row keeps step_output_choice / stops_at_ends (Auto's 1 is no longer skipped). Review (hckrman101): - A refresh within a quarter period of the game's frame shows that frame itself, and the timeline is pulled an eighth of the way to it. At 60 fps on 60 Hz with jitter (and 59.94/60.06 Hz drift) no frame is made and none dropped; other rates keep the display's count. The test now runs with jitter. - The overlay's game rate counts each Auto game frame once, given a refresh or not (menu_note_frame(true, 0)). - Windows: a whole-Hz rate one under a multiple of 6 (59, 119, 143...) is taken as its /1.001 rate. - Frame pacing's help says Auto paces itself; framegen_wanted's comment names Auto. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013mj4MdWTJo5ht4ShZX8Vpp
|
Thanks for the careful review, all points taken:
Smaller:
Played on Linux (RX 7900 XTX, RADV, 120 Hz) on current main plus this PR: the game runs at 45 to 60 fps, and Auto holds a steady 120. |
… Auto The Frame generation row takes main's hint scheme: Auto's hint says what it makes and that Frame pacing does not apply there (main skipped the value 1). menu_note_frame keeps the shown count beside menu_note_present_sizes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013mj4MdWTJo5ht4ShZX8Vpp
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, the fixes look good. One thing blocks it though: the Windows Vulkan back end check fails to compile vk_framegen.cpp (details inline, it's the near macro from windows.h).
The rest, re-checked:
- Snapping: I re-ran the slot logic on my side for a minute of frames, at 30/45/59.94/60/90/120 fps on 59.94/60/60.06/120/144/165 Hz with 0, ±0.2 and ±1 ms jitter. At 60 fps on ~60 Hz, 0% of shown frames are made and every game frame gets a refresh. Shown/s always matches the refresh (or the cap of 4 per game frame at 30 fps on 144/165 Hz), and nothing gets dropped below the display's rate. The pull is first order with a 1/8 gain, so it doesn't oscillate, and it stays locked through 60 vs 59.94 drift.
- Overlay: fixed, the game frame is now counted once per job.
- Windows refresh: the 59 -> 59.94 heuristic covers the common case. Fine by me, as long as the PR says it's untested on Windows.
- Pacing / comments: OK. The note on the Frame pacing row from 119c981 went away when you merged main's hints, but the Auto hint on Frame generation says it now, which is enough.
- Merge:
wait_until_near,prep_window,stops_at_ends/step_output_choiceand the new hints are all kept correctly.
Once Windows builds I'm happy to approve.
| // frames none (their neighbour two). The timeline is pulled an eighth of the way to the frame, so that a game at the | ||
| // display's rate (or a whole fraction of it) stays on it while the two clocks drift apart, and other rates hardly | ||
| // change how many frames the display is given | ||
| const int64_t near = period / 4; |
There was a problem hiding this comment.
This breaks the Windows build: windows.h (minwindef.h) has #define near (empty), so this becomes const int64_t = period / 4; (CI: vk_framegen.cpp:132:24: error: expected unqualified-id, plus 133 and 136). Renaming it (snap, within, ...) fixes it. Linux didn't catch it because the test fixture doesn't include windows.h.
What
Frame generation gets Auto in the Insert menu (its value 1, the step the row skipped so far). Auto makes a frame for each of the display's refreshes and no more:
The fixed 2x to 8x are unchanged.
How
auto_slots).tbetween the two game frames.fg_synthalready takes anyt. A refresh att = 1gets the game's own frame.OutSet::due), instead of spreading the frames evenly over the game frame.The refresh rate comes from the display the window is on, and an unknown refresh counts as 60 Hz:
SDL_GetDisplayForWindow+SDL_GetCurrentDisplayMode(SDL2:SDL_GetWindowDisplayIndex+SDL_GetCurrentDisplayMode). Both are looked up optionally, are asked on the main thread once a second, and are handed to the renderer through a newrefresh_mhzin the window bridge (MKWVK_WINDOW_VERSION2; a version 1 bridge still starts).EnumDisplaySettingsWfor the window's monitor, cached for a second.Why
With fixed multipliers, a game running between the display's rate and half of it either makes too many frames or too few:
Auto follows the display, also when the same install is played on a TV one day and a monitor the next.
Tested
RX 7900 XTX, RADV (Mesa 26.2.6), Linux (Hyprland, native Wayland), v2.0.7 plus this change, Kokiri Forest:
EnumDisplaySettingsWpath is written but not built or run; I have no Windows machine. The Windows builder CI will compile it.tests/test_framegen_auto.pyruns the realauto_cap,multiplierandauto_slotsover refresh rates and game rates. It covers 45 fps on 60 Hz (60 shown, three of four made at 1/4 to 3/4), 60 on 60, 75 on 60, 45 on 120, 30 on 240 (capped) and a restarted timeline. It also checks thatdisplay_refresh_mhzis declared outside the unnamed namespace: an earlier draft declared it inside, and the shared library linked with the symbol undefined.tests/test_framegen_gate.pynow counts Auto as asking for interpolation.Not covered
🤖 Generated with Claude Code