Save Output resolution as a size, and take any size from the config #1814
No reviewers
Labels
No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
No milestone
No assignees
1 participant
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Hookshot!1814
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/output-res-size"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Requested by Tommy
A screen whose size is not one of the menu's five Output resolutions has no way to get a native frame, and every new size has to claim the next number in
output_res(#1641 and #1670 both took 5). This stores Output resolution as the size itself and accepts any size from the config, as agreed in the #1670 review.What changed
output_resis a size:output_res=3840x2160inhookshot_options.ini, and the same forHOS_OUTPUT_RES=5120x2880.host/hos_main.c's screen shapes). Anything else uses the game's 2048x1152, and the log says why. A size that isn't a preset shows in the menu as its own choice (just its size, like "3440x1440"), in its place in the size order, with a hint saying where it comes from. 2560-wide sizes other than 2560x1440 (2560x1080, 2560x1600) now get the display-size list and BaseResolution edits too:nx2/oot_patches.pychecked only the width.docs/development.mdand the launcher's choices (recomplauncher.json) now use sizes.host/nn_applet.c:hos_output_parse, a sharedini_textthathos_ini_optionnow uses) and the menu (parse_output, a custom slot after the presets, an order sorted by size) follow the same rules.Downgrading: an older build reads
output_res=3840x2160as 3840, an unknown choice, and uses the game's 2048x1152 until it is set again.After #1670: rebased onto main with #1670 merged. 5120x2880 is now an ordinary preset (5), the player's own size is choice 6, the number 5 that #1670's builds save is read as 5120x2880, and this PR's general cap replaces #1670's 5K-only one. #1641's 1280x720 now only needs a preset and its hint.
Testing
Windows, RTX 4090, the standalone build from this branch. Booted headless with
HOS_OUTPUT_RES=5120x2880into a save at Render resolution 100%: 900 MB pool, walked around, no crash, and the menu shows it.tests/test_output_resolution.pyis rewritten for sizes. It covers the old numbers, preset sizes, sizes of the player's own (3440x1440, 2560x1080, 5120x2160, 1366x768 rounded to 1360x768), out-of-range sizes, the handheld profile, the override, the menu order with a custom size in each position, and that what the menu saves reads back as the same choice on both sides.tests/test_rt_pool_minimum.pygets cases above 4K's pixels. Both, plus the menu checks, were compiled with clang and pass.tests/run_all.pygives the same results asmainon this machine.2560x1080: the patch change was checked by applying the same edit to the two translated chunks (
code_011.c,code_013.c) and rebuilding: booted into a save at 2560x1080 with the whole 21:9 frame, no crash. 5120x2880 booted and played again with the bounds in place.After the rebase: booted with an options file saying
output_res=5(from #1670's build): 5120x2880, 900 MB pool, the menu shows the 5120x2880 preset.tests/test_stick_dead_zone.py(the CI failure) now stubsparse_outputand passes.On
mainat51de681the Windows renderer does not link here (present_commit/present_defer_intoare declared invk_framegen.hbut defined nowhere sincea259978, "present ahead re-applied next"), so the last two rebases were checked with the compiled tests and the suite, and the boots above were onb2ed731.Screenshot
Output resolution with
output_res=5120x2880(2560x1440 window), taken before 5K became a preset and the "(your own)" label was dropped; it now reads just "5120x2880":🤖 Generated with Claude Code
is "your own" needed here?
Thanks, this is the direction I hoped for on #1670. The migration reads right: 0 to 4 map to the same sizes on both sides, 5 / garbage / negatives /
3840 x 2160(spaces) all land on the game's own size on both sides, a value that's already a size round-trips, and2048x1152goes through the game's-own path just as 0 did. No options_version bump needed since both forms are read. An older build reads3840x2160as 3840, out of range, so 2048x1152 with no crash, as the description says. No new menu rows, so option ids are unchanged, and both tests were already registered in run_all.py.Two things before I'd approve:
Widths of 2560 with a height other than 1440 don't get the size.
nx2/oot_patches.py(apply_output_size) gates two of its edits onhos_output_size(0) != 2560: the display-size list (... != 2560 && recomp_load32(c, c->x[20] + 8) == 2560) and the BaseResolution scaling (if (hos_output_size(0) != 2560) { x24 = ...; x25 = ... * hos_output_size(1) / 1440; }). Before this PR a width of 2560 always meant 1440. Now2560x1080(a common 21:9 monitor) or2560x1600gets its window textures at the new size from the first edit, but keeps a 2560x1440 frame base and an unscaled 3D base. I'm going from the generated code here, not a run. Gating onw != 2560 || h != 1440in both places (or always applying them) should fix it.The range goes well past what the 900 MB cap was tested for. 5120x2880 fits in 900 MB because its 3D base (3/4 of the output) is about 1.8x 4K's, and 4K's 687 MB pool has room to spare. 7680x4320 is 4x 4K's 3D at Render resolution 100% in the same 900 MB. That's likely the issue 1031 failure (a null target as an area loads), not a graceful "runs short". I'd cap what's accepted at 5120x2880's pixels (or whatever you've actually booted), or fall back for anything above that. The readme line could then stay as it is.
Smaller:
hos_main.ccaps the widened picture at 3840x2160's pixels, so e.g.5120x2160with Ultrawide on gets a ~4.4K-wide picture, not a native frame. The new hint tells people to turn Ultrawide on for non-16:9 sizes, so maybe mention that case (or drop the advice above 4K).640x4320is accepted. hos_main.c clamps the screen shape to 5:4..32:9, so the same bound on the output would be cheap.if (r.value == &opts.output_res) {body insave()is under-indented, and the comment atset_row'sstep_output_choicecall still sayskOutputOrder.#1670 / #1641: this does make the index-5 clash go away. Neither needs a new number any more. The host takes any size, so both reduce to adding a preset to
kOutputsplus its hint (before the "your own" hint, sincekCustomOutputmoves to 6/7). The legacy table inhos_output_parsestays at 0 to 4, because 5 was never saved by a released build. #1670's own 5K pool cap is replaced by this PR's general one. So I'd land this first and rebase #1670 and #1641 onto it. The rebase is mostly mechanical, though the test's hard-codedkLastOutput == 5and{3, 0, 1, 2, 4}order need updating each time.CI: the Builder package and AppImage jobs pass. Python checks and the Windows build were still pending when I looked.
Nit: the body of this
ifis indented at thefor's level, so it reads as if it were outside the loop. Braces on thefor(or one more indent) would help.@ -87,57 +87,95 @@ HLE(_ZN2nn2oe18GetDesiredLanguageEv) {(MKW docked composes 3840 x 2160) */Two things about this range. (1) A width of exactly 2560 with any height other than 1440 passes here, but
nx2/oot_patches.pyskips its display-size-list and BaseResolution edits wheneverhos_output_size(0) == 2560, so 2560x1080 and 2560x1600 end up half-applied (see the review body). (2) 7680x4320 is accepted while the pool is capped at 900 MB, which was only tested at 5120x2880. I'd cap the area at the tested size.Fine for 5120x2880. Above that, the 3D at Render resolution 100% grows with the output (8K is 4x 4K's 3D) while the pool stays at 900 MB, so I'd expect the issue 1031 crash rather than a slowdown. That's another reason to bound the accepted size, not just document it.
Thanks for the quick turnaround. Everything from the last round is addressed:
apply_output_sizeedits innx2/oot_patches.pynow check width and height, so 2560x1080 and 2560x1600 get the display-size list and the BaseResolution scaling. The tests cover both.save()loop has braces now, thekOutputOrdercomment is gone, and the "(your own)" label is dropped.I checked that the host (
hos_output_parse) and the menu (parse_output) follow the same rules (rounding before the bounds, 2048x1152 → the game's own, old numbers 0–5 including #1670's 5 → 5120x2880, 6+ → the game's own) and that what the menu saves reads back as the same choice on both sides. With #1670 merged, folding 5K in as preset 5 and using one general pool cap looks right. #1641's 720p now only needs a preset and a hint, but it is currently conflicting and will need a rebase onto this.CI: the failing jobs are main's own failures (the
present_commit/present_defer_intolink error on Windows and in the AppImage build;test_present_ahead,test_present_semaphores,test_draw_parameters,test_nup_out_of_space,test_wayland_libdecor_hintalso fail on main at51de681). Nothing in them comes from this PR. Mergeable against current main.LGTM.