MagicBeanDev/Hookshot

Archived

Watch

Fork 0

feat(camera): add adjustable gameplay field of view #2357

Merged

polaco1782 merged 5 commits from feat/adjustable-fov into main

2026-11-11 22:48:30 +00:00

polaco1782 commented

2026-11-11 15:45:31 +00:00

(Migrated from github.com)

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

Add Gameplay → Field of view, from 50% to 150% in steps of 5. 100% (original) is the default; changes apply immediately and save as field_of_view.
Scale the primary gameplay camera's vertical angle before projection and culling are built. Keep the authored angle intact so repeated updates do not compound the adjustment and game zooms remain relative to the selected scale.
Leave other views and scripted events alone. Check the supported build ID and instructions before installing the hook.
Cover the hook, live options, bounds, register preservation, unique option IDs and menu slider behavior with game-free checks.

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

Full game-free suite on the initial clean base 940eb9ef: 400 passed, 0 failed, 21 skipped in 381 s, run sequentially.
After the final rebase onto 463eab1f: 17 focused checks passed, including FOV, camera speed, menus and the intervening upstream changes.
Linux Vulkan renderer rebuilt and linked successfully on 940eb9ef with one build worker. No additional full game rebuild was completed by the agent after rebasing; the author's manual gameplay results are recorded below.
The original pre-rebase full Linux game build passed. Earlier bounded offscreen gameplay on Fedora 44, Ryzen 7 5700X / 32 GB, RX 7800 XT / RADV Mesa 26.1.8 changed 100% → 125% → 100% in Dodongo's Cavern 2F. The wider scene, restored framing, stable HUD placement, fitting menu/help and saved values were inspected locally. Original saves remained unchanged. These gameplay runs predate the current rebase and are not a current-branch gameplay result.

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.

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 - Add **Gameplay → Field of view**, from **50% to 150%** in steps of 5. **100% (original)** is the default; changes apply immediately and save as `field_of_view`. - Scale the primary gameplay camera's vertical angle before projection and culling are built. Keep the authored angle intact so repeated updates do not compound the adjustment and game zooms remain relative to the selected scale. - Leave other views and scripted events alone. Check the supported build ID and instructions before installing the hook. - Cover the hook, live options, bounds, register preservation, unique option IDs and menu slider behavior with game-free checks. ## 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 - Full game-free suite on the initial clean base `940eb9ef`: **400 passed, 0 failed, 21 skipped** in 381 s, run sequentially. - After the final rebase onto `463eab1f`: **17 focused checks passed**, including FOV, camera speed, menus and the intervening upstream changes. - Linux Vulkan renderer rebuilt and linked successfully on `940eb9ef` with one build worker. No additional full game rebuild was completed by the agent after rebasing; the author's manual gameplay results are recorded below. - The original pre-rebase full Linux game build passed. Earlier bounded offscreen gameplay on Fedora 44, Ryzen 7 5700X / 32 GB, RX 7800 XT / RADV Mesa 26.1.8 changed **100% → 125% → 100%** in Dodongo's Cavern 2F. The wider scene, restored framing, stable HUD placement, fitting menu/help and saved values were inspected locally. Original saves remained unchanged. These gameplay runs predate the current rebase and are not a current-branch gameplay result. ## 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. - [Game-free Windows/Linux checks](https://github.com/polaco1782/Hookshot/actions/runs/43101038121) - [Windows Builder and llvm-mingw Vulkan build](https://github.com/polaco1782/Hookshot/actions/runs/43101042910) - [Linux AppImage](https://github.com/polaco1782/Hookshot/actions/runs/43101047414)

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

2026-11-11 20:08:13 +00:00

Joey-Magnus (Migrated from github.com) left a 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).

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).

polaco1782 commented

2026-11-11 21:17:27 +00:00

(Migrated from github.com)

Removed screenshots as Joey-Magnus requested

Removed screenshots as [Joey-Magnus](https://github.com/Joey-Magnus) requested

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

2026-11-11 21:24:52 +00:00

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

Option ID collision: HOS_OPT_FOV = 49 matches #2345's HOS_OPT_WALK_MODE = 49. The PRs touch different hunks of vk_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), which oot_fov.c rejects (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.cpp and nx2/lift.py.

- **Option ID collision:** `HOS_OPT_FOV = 49` matches #2345's `HOS_OPT_WALK_MODE = 49`. The PRs touch different hunks of `vk_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), which `oot_fov.c` rejects (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.cpp` and `nx2/lift.py`.

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

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

Joey-Magnus (Migrated from github.com) left a 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.

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 (Migrated from github.com) reviewed

2026-11-11 22:19:14 +00:00

hckrman101 (Migrated from github.com) left a 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_FOV is now 51, with a comment reserving 49/50 for #2345. The new tests/test_option_ids.py is a good answer to his "check every ID, not two": it compiles the real enums plus the legacy id == N cases in the option lambda, includes the reserved Walk/Item-wheel IDs, is declared with RUN_ALL_ARGS, and proves it fails on 49, 50, 60, 33 and 7. Once #2345 lands with HOS_OPT_WALK_MODE = 49 the 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 changed docs/development.md, gpu/vk/vk_api.h, vk_menu.cpp, vk_menu.h, vk_replay.cpp, nx2/lift.py and readme.md, all files this PR touches. Please rebase and re-run test_oot_fov, test_camera_speed, test_option_ids and 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.md row 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.

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_FOV` is now 51, with a comment reserving 49/50 for #2345. The new `tests/test_option_ids.py` is a good answer to his "check every ID, not two": it compiles the real enums plus the legacy `id == N` cases in the option lambda, includes the reserved Walk/Item-wheel IDs, is declared with `RUN_ALL_ARGS`, and proves it fails on 49, 50, 60, 33 and 7. Once #2345 lands with `HOS_OPT_WALK_MODE = 49` the 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 changed `docs/development.md`, `gpu/vk/vk_api.h`, `vk_menu.cpp`, `vk_menu.h`, `vk_replay.cpp`, `nx2/lift.py` and `readme.md`, all files this PR touches. Please rebase and re-run `test_oot_fov`, `test_camera_speed`, `test_option_ids` and 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.md` row 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.

hckrman101 (Migrated from github.com) approved these changes

2026-11-11 22:42:28 +00:00

hckrman101 (Migrated from github.com) left a 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 = 51 is 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.py is still declared and covers the reserved IDs.
The hook, menu row (slider and stop-at-the-ends handling), option lambda entry, lift.py install 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.md row 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.

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 = 51` is 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.py` is still declared and covers the reserved IDs. - The hook, menu row (slider and stop-at-the-ends handling), option lambda entry, `lift.py` install 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.md` row 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.