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

Merged

jarynclouatre merged 2 commits from fix/option-set-list-order into main

2026-11-11 22:22:41 +00:00

jarynclouatre commented

2026-11-11 21:00:36 +00:00

(Migrated from github.com)

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 53bc3d4c plus this change, nx2/build.py --standalone).
Not run in a game with the bridge.

## 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 53bc3d4c plus this change, `nx2/build.py --standalone`). - Not run in a game with the bridge.

Joey-Magnus (Migrated from github.com) requested changes

2026-11-11 21:46:31 +00:00

Joey-Magnus (Migrated from github.com) left a comment

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.

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.

jarynclouatre commented

2026-11-11 21:50:50 +00:00

(Migrated from github.com)

Thanks, both done in bf65dc6c:

tests/test_menu_setting_walk.py: #define _CRT_SECURE_NO_WARNINGS is the fixture's first line, as #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.

Thanks, both done in bf65dc6c: - `tests/test_menu_setting_walk.py`: `#define _CRT_SECURE_NO_WARNINGS` is the fixture's first line, as #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 (Migrated from github.com) approved these changes

2026-11-11 22:17:31 +00:00

hckrman101 (Migrated from github.com) left a comment

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.

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.