Skip to content

fix(menu): option_set walks the list rows in the arrows' order to the value asked for - #2496

Merged
hckrman101 merged 2 commits into
NotRexed:mainfrom
jarynclouatre:fix/option-set-list-order
Nov 11, 2026
Merged

hckrman101 merged 2 commits into
NotRexed:mainfrom
jarynclouatre:fix/option-set-list-order

Conversation

@jarynclouatre

Copy link
Copy Markdown
Contributor

What was wrong

The control bridge's option_set stepped the Upscaler, Render resolution and Output resolution rows towards the number it was given and stopped at the first step past it. The numbers on these rows are not in the order the arrows step through them:

  • Output resolution: the sizes go by width, so 1920x1080 is choice 3 but comes before 2048x1152 (choice 0). From the default, option_set output_res 3 stepped 0 > 1 > 2 > 4 and saved 5120x1440. From 1920x1080, asking for 0, 1 or 2 changed nothing.
  • Render resolution with DLSS or FSR 4: from Auto, the first step right is Native, which keeps the value. The loop read that as "no further" and stopped, so any render resolution asked for from Auto switched the Upscaler to DLAA (or FSR 4's native AA).
  • Upscaler: the engines step in their own order, so some targets were stepped past.
  • A value no step reaches (output_res 6 with no size of your own) landed on the last size.

The change

menu_setting_set now walks these rows with the arrows' own steps, one way and then the other, until the row holds the value. The Render resolution's Native (judged on the upscaler that runs, as the arrows judge it) counts as its own place, so it is never taken for a value. An Upscaler walk passes other engines, and each snaps the Render resolution to its own steps, so the resolution is snapped once at the end, from where it was, as a single step would. When no step reaches the value, the row is left as it was. Other rows are unchanged. docs/control.md says the same.

Testing

  • New tests/test_menu_setting_walk.py compiles the menu's real walk_to, step_output_choice and the steps in upscaler_rows.h, and walks every start to every target: output sizes with and without a size of your own, Render resolution with DLSS and FSR 4 (Native included) and with a saved DLAA or FSR 4 native AA that cannot run, and the Upscaler with three sets of engines from every render resolution, checking the resolution is kept as one step would keep it. It passes. The cases from before the resolution check fail 176 times on the old loop.
  • test_menu_env_overrides.py, test_output_resolution.py, test_control.py, test_upscaler_rows.py, test_menu_restart.py, test_menu_keys.py pass.
  • The renderer builds on Linux (main 53bc3d4 plus this change, nx2/build.py --standalone).
  • Not run in a game with the bridge.

… value asked for

The Upscaler, Render resolution and Output resolution rows were stepped towards the number asked for and
stopped at the first step past it. Their numbers are not in the arrows' order: from 2048x1152, 1920x1080 (3)
went on to 5120x1440, and from DLSS Auto any Render resolution stopped at Native (DLAA). Walk the row each
way until it holds the value, and leave it as it was when no step reaches it.

@Joey-Magnus Joey-Magnus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The walk is the right fix, and comparing the whole state (the value, Native and the Upscaler) to detect an end stop or a full loop is sound.

The Windows "Python source checks (part 1/2)" failure comes from this PR. tests/test_menu_setting_walk.py fails under clang++ with the Windows CRT, because -Werror turns the deprecation warnings for getenv (host/hos_wide_size.h:36) and sscanf (line 31 of the fixture) into errors. The branch already has #2397, so this isn't the old-base failure. Adding #define _CRT_SECURE_NO_WARNINGS as the fixture's first line, as #2397 did in test_native_astc_texture_count.py, should fix it.

Minor: when no step reaches the value, walk_to steps away and back again. Each set_row call on the way still has its side effects: the Quality preset becomes Custom, and an Upscaler step calls env_release(&opts.resolution). So an unreachable option_set leaves the row as it was but the preset changed, and the reply doesn't say it failed. docs/control.md says such a row is left as it was.

…nged and replies failed; the walk test builds with the Windows CRT
@jarynclouatre

Copy link
Copy Markdown
Contributor Author

Thanks, both done in bf65dc6:

  • tests/test_menu_setting_walk.py: #define _CRT_SECURE_NO_WARNINGS is the fixture's first line, as test: test_native_astc_texture_count builds on Windows, where the CRT marks scanf deprecated #2397 did.
  • An option_set none of the steps reach now puts back everything the walk changed: the options (the Quality preset with them), the rows the environment held, and the restart question's state. It replies failed with "no step reaches N on this PC (left as it was)". menu_setting_set returns -2 for that, and docs/control.md, vk_api.h and control.h say so.

Tested on Linux: test_menu_setting_walk.py (g++) and test_control.py pass, and the renderer builds. The Windows CRT build itself I have not run here; the CI run will show it.

@hckrman101 hckrman101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The walk is the right fix, and bf65dc6 answers both of Joey-Magnus's points.

  • The Windows source-check failure: the fixture now starts with #define _CRT_SECURE_NO_WARNINGS, and all CI jobs are green on this head, including windows-2022 parts 1 and 2 and the llvm-mingw Vulkan build.
  • An option_set no step reaches now puts back the options (the Quality preset with them), the rows the environment held and the restart state, and replies failed (-2 from menu_setting_set, handled in cmd_option_set). docs/control.md, vk_api.h and control.h all say so. That matches what I read in the diff.

The Upscaler case also looks right: the Render resolution is snapped once at the end, from where it was.

Nits only, none blocking:

  • The failure path (restoring the options, env and restart state) is not covered by the new test. The test's set() mirrors the walk but not that restore, so a future edit could drop it unnoticed.
  • The docs/control.md paragraph now has one very long line; worth rewrapping to the file's width.

Not run in a game with the bridge, as the description says. Good to merge from my side.

@hckrman101
hckrman101 merged commit 4087241 into NotRexed:main Nov 11, 2026
13 checks passed
pmwork-personal added a commit to pmwork-personal/Hookshot-pr that referenced this pull request Nov 11, 2026
…and NotRexed#2496)

NotRexed#2119 renamed the Screen section's Ultrawide row to Aspect ratio and changed
Ultrawide menus' help; NotRexed#2481 added HDR tone mapper after HDR contrast. Both
changed rows inside the block this PR moves into sections: the sections keep
main's definitions word for word (every one of main's 81 rows matches), with
HDR tone mapper last in HDR. The readme's table takes main's Aspect ratio and
HDR tone mapper lines in the section order. test_menu_sections.py also pins
Profile, Aspect ratio, Ultrawide menus and HDR tone mapper in their sections.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pmwork-personal added a commit to pmwork-personal/Hookshot-pr that referenced this pull request Nov 11, 2026
…2119, NotRexed#2197, NotRexed#2481, NotRexed#2496)

NotRexed#2177 landed (squashed), so this takes main's sections as they shipped and
keeps this PR's change on top: the Screen, HDR and Overlays rows on the
Display tab. Main's newer rows go with them, word for word: NotRexed#2119's Aspect
ratio and Ultrawide menus text and NotRexed#2481's HDR tone mapper (last in Display's
HDR). Every one of main's 88 rows keeps main's definition; the only changes
are the moved rows' tab and the Overlays heading. draw_options keeps main's
Save my pipeline list line (NotRexed#2447) with the restart line on Display too. The
readme takes main's Menu shortcut text (NotRexed#2197) with six tabs, NotRexed#2177's VRAM
budget line, and main's HDR tone mapper and Field of view lines.
test_menu_sections.py pins Aspect ratio by name and HDR tone mapper in
Display's HDR; test_scene_tonemap_pass.py (NotRexed#2481) finds the tone mapper row on Display.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pmwork-personal added a commit to pmwork-personal/Hookshot-pr that referenced this pull request Nov 11, 2026
NotRexed#2197, NotRexed#2481, NotRexed#2496) into feat/menu-game-tab

Takes NotRexed#2178 as merged with main (cf90c14f) and keeps this PR's change on
top: the Game tab first, where the menu opens. Main's new Gameplay row,
Field of view, goes with the Game rows at the table's top, word for word,
after Game speed. Every one of main's 88 rows keeps main's definition; the
only changes are the moved rows' tab (Display) and the Overlays heading.
The readme takes main's Menu shortcut text (NotRexed#2197) with this PR's tab
sentence, and Field of view among Game's rows. The tab enum, the strip's
titles and the control bridge's names follow the same Game-first order.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants