Repository navigation
feat(camera): add adjustable gameplay field of view - #2357
Conversation
411308a to
42a9866
Compare
Joey-Magnus
left a comment
There was a problem hiding this comment.
The core of this is nicely done. The hook sits after ldr s0, [x19, #0x13c] (nx2/oot_fov.py:11-13). The scaled angle only flows into the projection store (str s0, [x20, #0xa0]) and the half-angle terms, and the camera's own +0x13c is never written. So zooms stay relative and repeated updates don't compound. The build-ID and instruction-word checks before installing, the LookAtCamera vtable and CameraMgr ownership check, the event-camera skip, and the clamp to 5–140° are all sensible. host/oot_fov.c is picked up by nx2/build.py's directory listing, and a64_vset64 comes from the lifted a64.h. On the PR's head I ran test_oot_fov, test_camera_speed and the menu_* checks, and they pass. No other site in nx2/oot_*.py targets main+0x4c52f8, and oot_fov.install appends to lifter.after rather than overwriting it. Adding an enum to vk_api.h doesn't touch MkwVkHost/MkwVkRenderer, so no API bump is needed.
blocking docs/screenshots/fov-125.png, fov-menu.png, fov-original.png (≈3.9 MB together): these are screenshots of the game committed into the repository. Nothing else in the tree carries game imagery. CONTRIBUTING allows screenshots in issues and PR descriptions, but it lists the screenshots of runs among what stays on your PC. Once merged, they live in the history forever. Please drop them from the branch and attach the images to the PR description instead (drag them into GitHub's editor, which gives user-attachments links), as other PRs do. The description's raw.githubusercontent.com links to your fork can then go too.
minor docs/worklist.md:36: the row for this feature goes into Open, but this PR does the work. The worklist says that when a PR merges, its row moves to Fixed, waiting for a release with the PR. As it stands, merging leaves an owned, finished item in Open for someone to clean up. Put it in the Fixed table (with this PR's link), or drop it.
should fix readme.md:343: "It works with Ultrawide and keeps the game's zoom changes relative to your choice". Per the PR body, ultrawide wasn't exercised in game. The code suggests it composes, since oot_wide replaces the ratio at +0x140 and this scales the vertical angle at +0x13c, but the readme shouldn't claim it until someone has run it. Also note that #2119 renames that row to "Aspect ratio". Whichever lands second will need to update this sentence.
question host/oot_fov.c:22-32: first-person views (C-up look, slingshot/bow/hookshot aiming) presumably also go through camera zero. Were they checked at 50% and 150%? A changed vertical angle there changes where the reticle maps on screen, and aiming is listed as not tested. Even one short run aiming the slingshot at both ends would cover the riskiest interaction.
minor: CI has only run the title check so far; the other workflows are waiting for approval. The branch also conflicts with main in docs/tested-configurations.md (rows added on top since).
Keep the gameplay camera angle independent of the selected aspect ratio and preserve authored zoom changes. Cover hook guards, live options, bounds, register preservation and menu compatibility. Keep screenshots out of the branch history, remove the completed worklist entry, and qualify untested ultrawide coverage following review.
42a9866 to
387a1a3
Compare
|
Removed screenshots as Joey-Magnus requested |
Joey-Magnus
left a comment
There was a problem hiding this comment.
- Option ID collision:
HOS_OPT_FOV = 49matches #2345'sHOS_OPT_WALK_MODE = 49. The PRs touch different hunks ofvk_api.h/vk_replay.cpp, so git won't flag it. #2345's check comes first in the option lambda, so FOV would read Walk mode (0/1), whichoot_fov.crejects (50-150), and FOV silently stops working. The FOV test only checks against PROMPTS and CAMERA_SPEED, so it won't catch this. Please renumber (51 looks free; check #2261's 60) and make the test check uniqueness across all IDs. - The earlier blocker still stands: three screenshots (~3.9 MB) committed under
docs/screenshots/. - It conflicts with main in eight files, including
vk_menu.cpp,vk_replay.cppandnx2/lift.py.
Record the PR author report that aiming works in the 50%-150% FOV range, including ultrawide mode, and remove the outdated untested wording. Keep the earlier automated run scoped to its own coverage.
Joey-Magnus
left a comment
There was a problem hiding this comment.
Thanks for removing the screenshots. docs/screenshots/ and the worklist row are gone from the branch, it now merges cleanly with main, and the readme only claims the aiming and ultrawide results you reported.
The blocking point is still open. gpu/vk/vk_api.h:44 is still HOS_OPT_FOV = 49, and #2345 is still open with HOS_OPT_WALK_MODE = 49, HOS_OPT_WALK_SPEED, so it uses 49 and 50. Once both are merged, whichever check comes first in the option lambda answers 49. If it's Walk mode, FOV reads 0 or 1, oot_fov.c rejects that, and FOV silently does nothing. Git won't flag it because the hunks don't overlap, and the PR description says 49 was kept on purpose. Please move FOV to 51 or higher and update tests/test_oot_fov.py:54 and :77. Also have the test check that every HOS_OPT_* value in vk_api.h is unique, rather than comparing against two.
CI hasn't run beyond the title check, so the workflows need a maintainer's approval before merge.
hckrman101
left a comment
There was a problem hiding this comment.
Re-reading this after Joey-Magnus's two rounds: the technical side looks good to me, and the points he raised are resolved. What is left is mechanical.
Resolved since his reviews
- The screenshots and the worklist row are gone from the branch.
- The ID collision:
HOS_OPT_FOVis now 51, with a comment reserving 49/50 for #2345. The newtests/test_option_ids.pyis a good answer to his "check every ID, not two": it compiles the real enums plus the legacyid == Ncases in the option lambda, includes the reserved Walk/Item-wheel IDs, is declared withRUN_ALL_ARGS, and proves it fails on 49, 50, 60, 33 and 7. Once #2345 lands withHOS_OPT_WALK_MODE = 49the test still passes (the reserved list is skipped for names that exist). - The readme now says the aiming/ultrawide result is the author's report, and the first-person aiming question is covered by that report (50% to 150%). It doesn't say which weapon or scene, so a short note from a second tester would still be welcome, but I would not block on it.
Still open
- The PR conflicts with main again (
mergeable: dirty). Main has since changeddocs/development.md,gpu/vk/vk_api.h,vk_menu.cpp,vk_menu.h,vk_replay.cpp,nx2/lift.pyandreadme.md, all files this PR touches. Please rebase and re-runtest_oot_fov,test_camera_speed,test_option_idsand the menu checks afterwards. When you do, re-check that 51 is still unused on main and in the open PRs (60 is #2261's). - CI: only the title check has run here. The three fork runs in the description are for the earlier code revision, so the upstream workflows still need a maintainer's approval after the rebase.
- Nit: the
docs/tested-configurations.mdrow for the author's report is almost all "Not recorded". It is honest, but it adds little as a tested configuration; fine to keep or drop.
I have no new findings on the hook itself beyond Joey's read (build-ID and instruction-word guard, lifter.after appended, camera's own +0x13c untouched). Merge decision stays with NotRexed, and Joey's change request is for him to clear or keep; from my side only the rebase is outstanding.
Preserve HDR tone-mapper option 47 and the Aspect ratio text while keeping FOV at option 51. Focused checks: 17 passed. Full game-free suite: 384 passed, 5 failures also reproduced on untouched main, 72 skipped.
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for the rebase. The PR now merges cleanly with main (it picked up the HDR tone-mapper row, ID 47, and camera speed, ID 48, without trouble), so the one thing I had left open is done.
Checked at this head:
HOS_OPT_FOV = 51is still unused on main (it has 0-10, 16-22, 24-28, 32-35, 40-43, 45-48) and in every open PR that adds an option ID (#2345 uses 49/50, #2261 uses 60, #2459 uses 29/30, #2307 uses 8192+, #1560 adds 44).tests/test_option_ids.pyis still declared and covers the reserved IDs.- The hook, menu row (slider and stop-at-the-ends handling), option lambda entry,
lift.pyinstall and the readme/development rows are intact after the merge. Joey-Magnus's blockers (screenshots, ID collision, worklist row) stay resolved, as in my earlier review.
Note for whoever merges: #2530 adds HOS_OPT_KEYBOARD = 49, which collides with #2345's Walk mode (49). That is not this PR's problem, but 49 is now claimed twice among the open PRs.
Still to do, not on you:
- CI: only the title check has run on this head (pending). The upstream workflows need a maintainer's approval, and the fork runs in the description are for the earlier code revision.
- Nit: the
docs/tested-configurations.mdrow for the aiming report is almost all "Not recorded"; fine to keep or drop. - The merge decision, and clearing Joey-Magnus's change request, stay with NotRexed.
Players currently have no independent control over the gameplay field of view. This adds a live, saved FOV control while keeping the selected display aspect ratio and HUD layout.
What changed
field_of_view.Review changes
The feature is based on upstream
main(463eab1f), with a documentation follow-up recording the author's manual gameplay validation. The screenshot-only commit is absent from the incoming history, all three screenshot files are absent from the branch, and the image section and raw-image links have been removed from this description. No replacement image attachments were added.The completed feature's worklist self-entry is removed. The README now records successful aiming tests in the 50% to 150% range, including with ultrawide enabled, as reported by the PR author. Upstream's prompts and camera-speed controls are preserved: FOV uses option ID 49, distinct from prompts (46) and camera speed (48). Neither renderer interface is changed, so no API-version bump is needed.
Validation
940eb9ef: 400 passed, 0 failed, 21 skipped in 381 s, run sequentially.463eab1f: 17 focused checks passed, including FOV, camera speed, menus and the intervening upstream changes.940eb9efwith one build worker. No additional full game rebuild was completed by the agent after rebasing; the author's manual gameplay results are recorded below.Manual gameplay validation
The PR author tested aiming in the 50% to 150% FOV range, including with ultrawide mode enabled, and reports that it works fine. These are author-reported manual gameplay results. The report does not specify individual weapons, scenes, build IDs or hardware details.
CI
All three fork workflows passed for code revision
387a1a33; the follow-up changes documentation only. Upstream workflows still require maintainer approval.