MagicBeanDev/Hookshot
Archived
Watch
6
0
Fork
You've already forked Hookshot
 0
Code Issues 125 Pull requests 205 Activity

feat(prompts): a new face or new controls show while the game runs, no restart #2366

Merged
gab9281 merged 11 commits from live-prompts into main 2026-11-11 17:59:27 +00:00
Conversation 14 Commits 11 Files changed 20 +1224 -218
gab9281 commented 2026-11-11 16:13:39 +00:00 (Migrated from github.com)

Since #1321, changing the Button prompts row or a binding on the Controls page only shows in the game's icons after a restart. This PR makes those changes show while the game runs, keeping #1321's rules.

What changes

When the face (the row, or Auto's controller) or a binding changes, host/prompts.c writes the new prompts where the game keeps the ones it loaded from the files #1321 serves:

  • Textures: the ocarina's notes, the Talk ring's letters, and the System page's controller picture with its leader lines. They're written in the memory pool the game makes over Common.bntx and change at once. The System page's picture is drawn once per opening, so it changes when the page is reopened.
  • Layouts: the Talk ring's A B X Y places and the ocarina hint's spacing, written where the game keeps Common.blarc.
  • Icons in texts (dialogue, control hints, menus, song lessons):
    • The game caches each glyph it has drawn under its character code, so a new cmap alone never shows.
    • The font now has its own code for each face, Switch button and pad input (nx2/prompt_assets.py, BIND_BASE U+F000..F54F, free in the game's font).
    • A text's button codes are rewritten to the codes of the current face and controls as the game lays the text out or draws it. This is done by two hooks at the head of the lifted text-pane functions (nx2/oot_patches.py).
  • Menu: the Button prompts row no longer asks for a restart when the game has all three parts loaded. Otherwise it asks, as before (for example under a mod that replaces those files).

#1321's rules are unchanged:

  • Mods come first.
  • The Switch face with the default controls shows the game's own images.
  • Without the prompt files, the game keeps its own icons.
  • Auto falls back to the Switch face when no controller is named.
  • The icons follow hos_binding, the bindings the Controls page sets.

Limit: a text that stays on screen keeps its old icons until the game shows it again: the next dialogue page, the hint shown again, a menu page reopened. The game keeps each text's drawn result until then. Textures change at once.

MKWVK_API_VERSION 17: MkwVkHost.prompts_live tells the menu which parts can change at once. Upstream's 15 (FPS breakdown) and 16 (control bridge, #926) came in on the rebase.

Screenshots

System page, Xbox, then PS5 after a live change and a reopen (ZL on RB reads "Target R1"). Windows build, under Proton:
system

Field: Xbox, then PS5 with A bound to R3 while the game runs. The Talk ring's letters moved at once, and the prompt reads "R3 Talk" (Windows):
field

Impa's lesson, Steam Deck, then a live change to PS5 during her dialogue. The next page has the PS5 icons (Linux):
impa dialogue

The same lesson: Impa's notes, and Link's replay with "Play a note" in PS5 (Linux):
impa lesson

The same on Windows:
impa windows

Tested

  • Linux, in game:
    • Talk ring and field prompt.
    • System page.
    • Bonooru's ocarina grid and hint. The notes on the staff changed PS5 → Xbox while shown.
    • His dialogue in each face.
    • Pause menu pages.
    • Impa's song lesson (dialogue, her notes, Link's replay) after live changes, on the final build.
  • Windows (llvm-mingw cross-build, run under GE-Proton): the same runs for the System page, the field (R3 Talk, the ring) and Impa's lesson. All three parts were found, and every change reached them.
  • Tests: python -I -B tests/run_all.py gives 336 passed, 3 failed, 49 skipped. All 3 fail the same way on upstream main:
    • test_wayland_libdecor_hint (newer clang, -Wunused-but-set-global in its own harness).
    • test_vcore_fs_open and test_vcore_video_acquire (env_on undeclared in their fixture).
    • New: test_prompts_live (putting the game's bytes back, guarded).
    • Extended: test_prompt_font_bind (285 text codes rewritten), test_menu_hooks (the text hooks).
  • Packages: no new files.
    • The AppImage copies nx2, host and gpu/vk whole.
    • The Windows parts take every tracked file under gpu, host, nx2, scripts and third_party.
    • test_appimage_prompt_fonts passes.
  • Review: a code review of the branch led to the last commit:
    • A part found after a change catches up.
    • Auto with no controller is the Switch face, as at the start.
    • Release/acquire on the pool's address.
    • The layouts are found without textures.
    • A font face not readable yet is looked at again.
    • !bind takes only a Switch button.
  • Left as is:
    • A change reads its files on the input thread. That happens only when the row or a binding changes; the font is 33 KB and each face's textures are read once.
    • After the first change, each text drawn is scanned for button codes. The texts are short.

🤖 Generated with Claude Code

Since #1321, changing the Button prompts row or a binding on the Controls page only shows in the game's icons after a restart. This PR makes those changes show while the game runs, keeping #1321's rules. ## What changes When the face (the row, or Auto's controller) or a binding changes, `host/prompts.c` writes the new prompts where the game keeps the ones it loaded from the files #1321 serves: - **Textures:** the ocarina's notes, the Talk ring's letters, and the System page's controller picture with its leader lines. They're written in the memory pool the game makes over `Common.bntx` and change at once. The System page's picture is drawn once per opening, so it changes when the page is reopened. - **Layouts:** the Talk ring's A B X Y places and the ocarina hint's spacing, written where the game keeps `Common.blarc`. - **Icons in texts** (dialogue, control hints, menus, song lessons): - The game caches each glyph it has drawn under its character code, so a new cmap alone never shows. - The font now has its own code for each face, Switch button and pad input (`nx2/prompt_assets.py`, `BIND_BASE` U+F000..F54F, free in the game's font). - A text's button codes are rewritten to the codes of the current face and controls as the game lays the text out or draws it. This is done by two hooks at the head of the lifted text-pane functions (`nx2/oot_patches.py`). - **Menu:** the Button prompts row no longer asks for a restart when the game has all three parts loaded. Otherwise it asks, as before (for example under a mod that replaces those files). #1321's rules are unchanged: - Mods come first. - The Switch face with the default controls shows the game's own images. - Without the prompt files, the game keeps its own icons. - Auto falls back to the Switch face when no controller is named. - The icons follow `hos_binding`, the bindings the Controls page sets. **Limit:** a text that stays on screen keeps its old icons until the game shows it again: the next dialogue page, the hint shown again, a menu page reopened. The game keeps each text's drawn result until then. Textures change at once. `MKWVK_API_VERSION` 17: `MkwVkHost.prompts_live` tells the menu which parts can change at once. Upstream's 15 (FPS breakdown) and 16 (control bridge, #926) came in on the rebase. ## Screenshots System page, Xbox, then PS5 after a live change and a reopen (ZL on RB reads "Target R1"). Windows build, under Proton: ![system](https://raw.githubusercontent.com/gab9281/Hookshot/pr-assets/live-prompts/system_xbox_to_ps5.jpg) Field: Xbox, then PS5 with A bound to R3 while the game runs. The Talk ring's letters moved at once, and the prompt reads "R3 Talk" (Windows): ![field](https://raw.githubusercontent.com/gab9281/Hookshot/pr-assets/live-prompts/field_xbox_to_ps5_r3.jpg) Impa's lesson, Steam Deck, then a live change to PS5 during her dialogue. The next page has the PS5 icons (Linux): ![impa dialogue](https://raw.githubusercontent.com/gab9281/Hookshot/pr-assets/live-prompts/impa_steamdeck_to_ps5.jpg) The same lesson: Impa's notes, and Link's replay with "Play a note" in PS5 (Linux): ![impa lesson](https://raw.githubusercontent.com/gab9281/Hookshot/pr-assets/live-prompts/impa_lesson_ps5.jpg) The same on Windows: ![impa windows](https://raw.githubusercontent.com/gab9281/Hookshot/pr-assets/live-prompts/impa_windows.jpg) ## Tested - **Linux, in game:** - Talk ring and field prompt. - System page. - Bonooru's ocarina grid and hint. The notes on the staff changed PS5 → Xbox while shown. - His dialogue in each face. - Pause menu pages. - Impa's song lesson (dialogue, her notes, Link's replay) after live changes, on the final build. - **Windows (llvm-mingw cross-build, run under GE-Proton):** the same runs for the System page, the field (R3 Talk, the ring) and Impa's lesson. All three parts were found, and every change reached them. - **Tests:** `python -I -B tests/run_all.py` gives 336 passed, 3 failed, 49 skipped. All 3 fail the same way on upstream main: - `test_wayland_libdecor_hint` (newer clang, `-Wunused-but-set-global` in its own harness). - `test_vcore_fs_open` and `test_vcore_video_acquire` (`env_on` undeclared in their fixture). - New: `test_prompts_live` (putting the game's bytes back, guarded). - Extended: `test_prompt_font_bind` (285 text codes rewritten), `test_menu_hooks` (the text hooks). - **Packages:** no new files. - The AppImage copies `nx2`, `host` and `gpu/vk` whole. - The Windows parts take every tracked file under `gpu`, `host`, `nx2`, `scripts` and `third_party`. - `test_appimage_prompt_fonts` passes. - **Review:** a code review of the branch led to the last commit: - A part found after a change catches up. - Auto with no controller is the Switch face, as at the start. - Release/acquire on the pool's address. - The layouts are found without textures. - A font face not readable yet is looked at again. - `!bind` takes only a Switch button. - **Left as is:** - A change reads its files on the input thread. That happens only when the row or a binding changes; the font is 33 KB and each face's textures are read once. - After the first change, each text drawn is scanned for button codes. The texts are short. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
hckrman101 (Migrated from github.com) reviewed 2026-11-11 17:03:27 +00:00
hckrman101 (Migrated from github.com) left a comment

Nice work. Reading the whole thing, the design holds up: one font with every face's icons and a code of its own for each face, button and input (so the glyph cache never shows a stale icon), the game's bytes put back before each new set of runs, and the layouts and font guarded against bytes the game has changed since. MKWVK_API_VERSION goes to 17 for MkwVkHost.prompts_live, and the new field is appended after fps_timing with rx_backend.c's initializer in the same order, so the exe/back end ABI is fine. HOS_OPT_PROMPTS = 46 doesn't collide with main or the open PRs (10, 44). The hooks follow the existing _hook_head pattern and are only applied in the supported build's lift. The touched tests are updated rather than loosened (test_menu_hooks checks all five hooks once and none in another lift). The new test_prompts_live declares RUN_ALL_ARGS and covers put back, run order, out of range and guarded. Nothing generated from the game is committed: the font, textures and blarc are still made on the player's PC by make_game.py.

One thing I'd like before this goes in:

  • The textures' pool is written without a check that it's still there. Once hos_prompts_pool has found it, tex.l.base/lo/hi stay set for the rest of the run, and live_textures → put_runs writes (unguarded) into that range on every change. If the game ever finalizes that pool and the memory goes to something else, a change in the menu would write over unrelated guest memory. The layouts (guarded) and the font (64-byte head check) are both protected against this. The textures are the only part that isn't. A cheap fix: compare the three tex.want samples again before put_runs (and drop base if they differ), or clear tex.l.base in nvnMemoryPoolFinalize when it's that pool. Inline below.

Smaller things (not blocking):

  • The menu only skips the restart prompt when all three parts are found. A build made without textures.bin (no texture tools) never sets the TEXTURES bit, so the row keeps asking for a restart even though everything that was served does change at once. Treating a part that wasn't served as satisfied would fix that.
  • The first change to a face reads its textures.bin and does seeks/reads in Common.bntx/Common.blarc on the thread that is reading the game's npad. You already list this under "Left as is", and it only happens on a change, so fine for now. It'd be good to move it to a worker if anyone reports a hitch.
  • Overlap with open PRs: #2304 also changes build_layouts (the Recomp options button in Common.blarc, served without runs), and #2270 changes build() and the size table path in prompts.c/prompt_assets.py. Both are textual conflicts for whichever lands second. With #2304, find_layouts should still match, since it takes the head and length from the served prompts/Common.blarc.

CI: the appimage failure is the runner failing to resolve archive.ubuntu.com (apt), not this PR. test_vcore_video_acquire / test_vcore_fs_open fail the same way on main. The other Python parts were still running when I looked. The screenshots across Linux and Windows are very helpful, thanks.

Nice work. Reading the whole thing, the design holds up: one font with every face's icons and a code of its own for each face, button and input (so the glyph cache never shows a stale icon), the game's bytes put back before each new set of runs, and the layouts and font guarded against bytes the game has changed since. `MKWVK_API_VERSION` goes to 17 for `MkwVkHost.prompts_live`, and the new field is appended after `fps_timing` with `rx_backend.c`'s initializer in the same order, so the exe/back end ABI is fine. `HOS_OPT_PROMPTS = 46` doesn't collide with main or the open PRs (10, 44). The hooks follow the existing `_hook_head` pattern and are only applied in the supported build's lift. The touched tests are updated rather than loosened (`test_menu_hooks` checks all five hooks once and none in another lift). The new `test_prompts_live` declares `RUN_ALL_ARGS` and covers put back, run order, out of range and guarded. Nothing generated from the game is committed: the font, textures and blarc are still made on the player's PC by `make_game.py`. One thing I'd like before this goes in: - **The textures' pool is written without a check that it's still there.** Once `hos_prompts_pool` has found it, `tex.l.base/lo/hi` stay set for the rest of the run, and `live_textures` → `put_runs` writes (unguarded) into that range on every change. If the game ever finalizes that pool and the memory goes to something else, a change in the menu would write over unrelated guest memory. The layouts (guarded) and the font (64-byte head check) are both protected against this. The textures are the only part that isn't. A cheap fix: compare the three `tex.want` samples again before `put_runs` (and drop `base` if they differ), or clear `tex.l.base` in `nvnMemoryPoolFinalize` when it's that pool. Inline below. Smaller things (not blocking): - The menu only skips the restart prompt when all three parts are found. A build made without `textures.bin` (no texture tools) never sets the TEXTURES bit, so the row keeps asking for a restart even though everything that was served does change at once. Treating a part that wasn't served as satisfied would fix that. - The first change to a face reads its `textures.bin` and does seeks/reads in `Common.bntx`/`Common.blarc` on the thread that is reading the game's npad. You already list this under "Left as is", and it only happens on a change, so fine for now. It'd be good to move it to a worker if anyone reports a hitch. - Overlap with open PRs: #2304 also changes `build_layouts` (the Recomp options button in `Common.blarc`, served without runs), and #2270 changes `build()` and the size table path in `prompts.c`/`prompt_assets.py`. Both are textual conflicts for whichever lands second. With #2304, `find_layouts` should still match, since it takes the head and length from the served `prompts/Common.blarc`. CI: the `appimage` failure is the runner failing to resolve archive.ubuntu.com (apt), not this PR. `test_vcore_video_acquire` / `test_vcore_fs_open` fail the same way on main. The other Python parts were still running when I looked. The screenshots across Linux and Windows are very helpful, thanks.
gpu/vk/vk_menu.cpp
@ -1385,6 +1385,14 @@ void save() {
}
}
/// (hookshot) The Button prompts row: a new face shows at once where the game has every part of the prompts from
hckrman101 (Migrated from github.com) commented 2026-11-11 17:03:27 +00:00

Nit: with a build that has no textures.bin, the TEXTURES bit never gets set, so this still asks for a restart even though the font and layouts change at once. Maybe have prompts_live count a part that wasn't served as live.

Nit: with a build that has no `textures.bin`, the TEXTURES bit never gets set, so this still asks for a restart even though the font and layouts change at once. Maybe have `prompts_live` count a part that wasn't served as live.
host/prompts.c
@ -472,0 +846,4 @@
}
/* the font with the face's icons on these controls (prompts/External.bfotf, cmap4_bind: hos_binding's), decrypted behind
its header (key: the XOR key); bound: how many buttons. NULL none */
hckrman101 (Migrated from github.com) commented 2026-11-11 17:03:27 +00:00

This writes into the pool range found once at startup, with no check that the pool still holds Common.bntx (guarded is 0 for tex.l). Re-checking tex.sample/tex.want here, or clearing tex.l.base when the pool is finalized, would keep a change from writing into memory the game has reused.

This writes into the pool range found once at startup, with no check that the pool still holds Common.bntx (`guarded` is 0 for `tex.l`). Re-checking `tex.sample`/`tex.want` here, or clearing `tex.l.base` when the pool is finalized, would keep a change from writing into memory the game has reused.
gab9281 commented 2026-11-11 17:18:59 +00:00 (Migrated from github.com)

@hckrman101 thanks for the review. Both points are addressed in 10615af4, and the branch is rebased on main:

  • Textures' pool: nvnMemoryPoolFinalize now tells prompts.c when a pool goes away (hos_prompts_pool_gone). If it is the textures' pool, nothing is written there any more, and live_textures checks this again right before put_runs. If the game makes the pool again, it is found again and catches up from the files served at the start. I didn't re-check the samples instead: they are picked only to avoid the start's runs, so after a change to another face they can hold our own writes. test_prompts_live now checks that the finalize reaches the prompts and that only the textures' own pool is dropped.
  • No textures.bin: prompts_live now counts a part that wasn't served as live, since a restart wouldn't change it either. It still returns 0 when no part was found at all.
  • #2270: it landed, and I resolved it in the rebase. A mod's size table is merged with ours as on main; the docs now say "the font", since this branch serves one.
  • The input-thread file reads stay as they are for now, as you said.

The build with these fixes passed the in-game live run on Linux before the rebase: every change reached the textures, layouts and font. The rebased branch is pushed but not yet built or tested; I'll post again once it is.

@hckrman101 thanks for the review. Both points are addressed in 10615af4, and the branch is rebased on main: - **Textures' pool:** `nvnMemoryPoolFinalize` now tells `prompts.c` when a pool goes away (`hos_prompts_pool_gone`). If it is the textures' pool, nothing is written there any more, and `live_textures` checks this again right before `put_runs`. If the game makes the pool again, it is found again and catches up from the files served at the start. I didn't re-check the samples instead: they are picked only to avoid the start's runs, so after a change to another face they can hold our own writes. `test_prompts_live` now checks that the finalize reaches the prompts and that only the textures' own pool is dropped. - **No `textures.bin`:** `prompts_live` now counts a part that wasn't served as live, since a restart wouldn't change it either. It still returns 0 when no part was found at all. - **#2270:** it landed, and I resolved it in the rebase. A mod's size table is merged with ours as on main; the docs now say "the font", since this branch serves one. - The input-thread file reads stay as they are for now, as you said. The build with these fixes passed the in-game live run on Linux before the rebase: every change reached the textures, layouts and font. The rebased branch is pushed but not yet built or tested; I'll post again once it is.
hckrman101 (Migrated from github.com) reviewed 2026-11-11 17:24:19 +00:00
hckrman101 (Migrated from github.com) left a comment

Thanks for the quick turnaround. Compared the rebased patches file by file against 82fa562: apart from your new commit, the differences are main's (Camera speed, memory budget, #2270's size-table merge).

Fixed:

  • No textures.bin: hos_prompts_live_parts now counts a part with no served file (romfs_state == 3) as live, and still returns 0 when nothing was found. Looks right.
  • #2270: resolved cleanly. sizes() matches main byte for byte, and hos_prompts_romfs has main's mod handling. The only change is state moving out to romfs_state so the live check can read it.
  • MKWVK_API_VERSION is still 16 on main, so 17 here is still the right bump. HOS_OPT_PROMPTS = 46 doesn't collide with main's 45/48.
  • Re-creating the pool at the same address works. After hos_prompts_pool_gone clears base, hos_prompts_pool passes its early return again, matches the samples against the served file, and resets part_face[PART_TEXTURES] to the start's, so the next look catches up. Putting back tex.l.ranges writes the served file's bytes, which is what the new pool holds anyway.

Still open: the finalize can land in the middle of a write. hos_prompts_pool_gone runs on the game's thread (in nvnMemoryPoolFinalize), and live_textures/put_runs run on the npad thread with no lock between them. The fix is a check followed by a write, so:

  1. If the finalize comes after the check in live_textures, put_runs still writes the whole set. It does file reads per range, so the window is the length of the disk I/O, and the game can free and reuse the storage right after the finalize returns.
  2. It's slightly worse than before in one case. Live.base is volatile and put_runs re-reads it on every access. If it goes to 0 between the second loop and the third, the third loop's hos_guest_range(l->base + runs[r].off, ...) has no range check and no NULL check, so it would memcpy to guest address off (low pages of the guest window): a crash or a stray write. The first two loops are saved by the a >= l->lo check.

Both are narrow, but closing them is cheap: one lock (an SRWLOCK, as nvn_native.c uses) held by live_textures around the base check and put_runs (plus the part_face[PART_TEXTURES] update), and taken by hos_prompts_pool_gone/hos_prompts_pool around their stores. A finalize then waits only in the rare case a change is in flight. At minimum, have put_runs read base once into a local and range-check the third loop like the others. Inline below.

Earlier nits: the file reads on the npad thread stay as agreed. #2304 is still open and still overlaps build_layouts/prompt_assets.py, so whichever lands second resolves it.

CI: every job is still queued on this head, and you said the rebased branch isn't built or tested in game yet. Happy to approve once the race is closed and that run is in.

Thanks for the quick turnaround. Compared the rebased patches file by file against 82fa562: apart from your new commit, the differences are main's (Camera speed, memory budget, #2270's size-table merge). **Fixed:** - No `textures.bin`: `hos_prompts_live_parts` now counts a part with no served file (`romfs_state == 3`) as live, and still returns 0 when nothing was found. Looks right. - #2270: resolved cleanly. `sizes()` matches main byte for byte, and `hos_prompts_romfs` has main's `mod` handling. The only change is `state` moving out to `romfs_state` so the live check can read it. - `MKWVK_API_VERSION` is still 16 on main, so 17 here is still the right bump. `HOS_OPT_PROMPTS = 46` doesn't collide with main's 45/48. - Re-creating the pool at the same address works. After `hos_prompts_pool_gone` clears `base`, `hos_prompts_pool` passes its early return again, matches the samples against the served file, and resets `part_face[PART_TEXTURES]` to the start's, so the next look catches up. Putting back `tex.l.ranges` writes the served file's bytes, which is what the new pool holds anyway. **Still open: the finalize can land in the middle of a write.** `hos_prompts_pool_gone` runs on the game's thread (in `nvnMemoryPoolFinalize`), and `live_textures`/`put_runs` run on the npad thread with no lock between them. The fix is a check followed by a write, so: 1. If the finalize comes after the check in `live_textures`, `put_runs` still writes the whole set. It does file reads per range, so the window is the length of the disk I/O, and the game can free and reuse the storage right after the finalize returns. 2. It's slightly worse than before in one case. `Live.base` is `volatile` and `put_runs` re-reads it on every access. If it goes to 0 between the second loop and the third, the third loop's `hos_guest_range(l->base + runs[r].off, ...)` has no range check and no NULL check, so it would `memcpy` to guest address `off` (low pages of the guest window): a crash or a stray write. The first two loops are saved by the `a >= l->lo` check. Both are narrow, but closing them is cheap: one lock (an SRWLOCK, as `nvn_native.c` uses) held by `live_textures` around the base check and `put_runs` (plus the `part_face[PART_TEXTURES]` update), and taken by `hos_prompts_pool_gone`/`hos_prompts_pool` around their stores. A finalize then waits only in the rare case a change is in flight. At minimum, have `put_runs` read `base` once into a local and range-check the third loop like the others. Inline below. Earlier nits: the file reads on the npad thread stay as agreed. #2304 is still open and still overlaps `build_layouts`/`prompt_assets.py`, so whichever lands second resolves it. CI: every job is still queued on this head, and you said the rebased branch isn't built or tested in game yet. Happy to approve once the race is closed and that run is in.
host/prompts.c
@ -465,3 +509,4 @@
ok ? "" : ": could not be written");
if (ok) part_served(PART_TEXTURES, face, bind);
free(runs);
free(t);
hckrman101 (Migrated from github.com) commented 2026-11-11 17:24:19 +00:00

This check and the put_runs below aren't atomic with respect to hos_prompts_pool_gone on the game's thread. A finalize right after this line still lets the whole set be written into storage the game can now free. A lock held from here through put_runs (and the part_face[PART_TEXTURES] update in hos_prompts_live), and taken in hos_prompts_pool_gone/hos_prompts_pool, would close it.

This check and the `put_runs` below aren't atomic with respect to `hos_prompts_pool_gone` on the game's thread. A finalize right after this line still lets the whole set be written into storage the game can now free. A lock held from here through `put_runs` (and the `part_face[PART_TEXTURES]` update in `hos_prompts_live`), and taken in `hos_prompts_pool_gone`/`hos_prompts_pool`, would close it.
hckrman101 (Migrated from github.com) commented 2026-11-11 17:24:19 +00:00

l->base is re-read here (it's volatile), and unlike the two loops above this one has no range or NULL check. If the finalize clears base between the second loop and this one, it writes at guest address runs[r].off. Reading base once at the top of put_runs (and bailing if it's 0) avoids that even without the lock.

`l->base` is re-read here (it's `volatile`), and unlike the two loops above this one has no range or NULL check. If the finalize clears `base` between the second loop and this one, it writes at guest address `runs[r].off`. Reading `base` once at the top of `put_runs` (and bailing if it's 0) avoids that even without the lock.
hckrman101 (Migrated from github.com) reviewed 2026-11-11 17:24:29 +00:00
host/prompts.c
@ -472,0 +846,4 @@
}
/* the font with the face's icons on these controls (prompts/External.bfotf, cmap4_bind: hos_binding's), decrypted behind
its header (key: the XOR key); bound: how many buttons. NULL none */
hckrman101 (Migrated from github.com) commented 2026-11-11 17:24:29 +00:00

Thanks, clearing base from nvnMemoryPoolFinalize is the right hook. One gap is left: the check in live_textures and the writes in put_runs can still interleave with the finalize on the game's thread. See the new comments at live_textures and in put_runs's last loop.

Thanks, clearing `base` from `nvnMemoryPoolFinalize` is the right hook. One gap is left: the check in `live_textures` and the writes in `put_runs` can still interleave with the finalize on the game's thread. See the new comments at `live_textures` and in `put_runs`'s last loop.
gab9281 commented 2026-11-11 17:28:18 +00:00 (Migrated from github.com)

@hckrman101 good catch. The race is closed in b846ee78:

  • One lock over the textures' pool (tex_lock, an SRWLOCK):
    • hos_prompts_live holds it from looking at tex.l.base, through live_textures/put_runs, to updating part_face[PART_TEXTURES].
    • hos_prompts_pool and hos_prompts_pool_gone take it to set or clear the pool.
    • A finalize that comes while a change is being written now waits for the writes to end, so the game frees the memory only after them. nvnMemoryPoolFinalize calls the hook after its own vmap_lock section, so no other lock is held there.
  • put_runs reads base once into a local and writes nothing when it is 0. Its last loop now checks each run against [lo, hi) and for NULL, like the two loops before it.
  • test_prompts_live checks two more things:
    • The finalize clears the pool while holding tex_lock, and releases it after.
    • put_runs over a range whose memory was let go writes nothing, not even at a run's offset from 0.

The Linux build and the in-game runs of the rebased branch with this commit are going now. I'll post the results here when they're in.

@hckrman101 good catch. The race is closed in b846ee78: - **One lock over the textures' pool (`tex_lock`, an SRWLOCK):** - `hos_prompts_live` holds it from looking at `tex.l.base`, through `live_textures`/`put_runs`, to updating `part_face[PART_TEXTURES]`. - `hos_prompts_pool` and `hos_prompts_pool_gone` take it to set or clear the pool. - A finalize that comes while a change is being written now waits for the writes to end, so the game frees the memory only after them. `nvnMemoryPoolFinalize` calls the hook after its own `vmap_lock` section, so no other lock is held there. - **`put_runs`** reads `base` once into a local and writes nothing when it is 0. Its last loop now checks each run against `[lo, hi)` and for NULL, like the two loops before it. - **`test_prompts_live`** checks two more things: - The finalize clears the pool while holding `tex_lock`, and releases it after. - `put_runs` over a range whose memory was let go writes nothing, not even at a run's offset from 0. The Linux build and the in-game runs of the rebased branch with this commit are going now. I'll post the results here when they're in.
hckrman101 (Migrated from github.com) approved these changes 2026-11-11 17:35:18 +00:00
hckrman101 (Migrated from github.com) left a comment

Thanks, both points from last time are fixed in b846ee7.

  • Race with the finalize: tex_lock is now held from the tex.l.base check through live_textures/put_runs and the part_face[PART_TEXTURES] update, and hos_prompts_pool/hos_prompts_pool_gone take it too, re-checking under the lock. Both hooks run outside vmap_lock in nvn_native.c, so I don't see a lock-order problem.
  • put_runs: base is read once, nothing is written when it's 0, and the last loop now has the same [lo, hi)/NULL check as the other two. The new test cases cover both.

One small nit inline: the first change per face reads textures.bin from disk while the lock is held. Still open from before, also a nit: a build with no textures.bin still asks for a restart in the menu.

Approving the code. Before merging, CI should go green (still pending at this head) and your in-game runs of the rebased branch should be posted.

Thanks, both points from last time are fixed in b846ee7. - Race with the finalize: `tex_lock` is now held from the `tex.l.base` check through `live_textures`/`put_runs` and the `part_face[PART_TEXTURES]` update, and `hos_prompts_pool`/`hos_prompts_pool_gone` take it too, re-checking under the lock. Both hooks run outside `vmap_lock` in `nvn_native.c`, so I don't see a lock-order problem. - `put_runs`: `base` is read once, nothing is written when it's 0, and the last loop now has the same `[lo, hi)`/NULL check as the other two. The new test cases cover both. One small nit inline: the first change per face reads `textures.bin` from disk while the lock is held. Still open from before, also a nit: a build with no `textures.bin` still asks for a restart in the menu. Approving the code. Before merging, CI should go green (still pending at this head) and your in-game runs of the rebased branch should be posted.
host/prompts.c
hckrman101 (Migrated from github.com) commented 2026-11-11 17:35:18 +00:00

Nit: when the textures are the first part to change, setup() and the read_all of textures.bin in live_textures both run with tex_lock held. A finalize on the game's thread then waits for that disk read. Loading tex.bins[face] before taking the lock would keep the time under the lock to the writes alone. It only happens once per face, so this isn't blocking.

Nit: when the textures are the first part to change, `setup()` and the `read_all` of `textures.bin` in `live_textures` both run with `tex_lock` held. A finalize on the game's thread then waits for that disk read. Loading `tex.bins[face]` before taking the lock would keep the time under the lock to the writes alone. It only happens once per face, so this isn't blocking.
hckrman101 (Migrated from github.com) reviewed 2026-11-11 17:35:27 +00:00
host/prompts.c
@ -465,3 +509,4 @@
ok ? "" : ": could not be written");
if (ok) part_served(PART_TEXTURES, face, bind);
free(runs);
free(t);
hckrman101 (Migrated from github.com) commented 2026-11-11 17:35:27 +00:00

Fixed in b846ee7: tex_lock covers the check through the writes and the part_face update, and both pool hooks take it.

Fixed in b846ee7: `tex_lock` covers the check through the writes and the `part_face` update, and both pool hooks take it.
hckrman101 (Migrated from github.com) reviewed 2026-11-11 17:35:29 +00:00
host/prompts.c
@ -465,3 +509,4 @@
ok ? "" : ": could not be written");
if (ok) part_served(PART_TEXTURES, face, bind);
free(runs);
free(t);
hckrman101 (Migrated from github.com) commented 2026-11-11 17:35:29 +00:00

Fixed in b846ee7: base is read once, with a bail-out at 0 and the range/NULL check in this loop too.

Fixed in b846ee7: `base` is read once, with a bail-out at 0 and the range/NULL check in this loop too.
gab9281 commented 2026-11-11 17:35:38 +00:00 (Migrated from github.com)

@hckrman101 the branch is on the latest main (no newer commits upstream) at b846ee78, with the race fix from my last comment. Since main keeps moving, I'm pushing first and testing after:

  • Pushed: b846ee78. It builds on Linux (make_game.py --update).
  • Running now: a full code review of the branch, and the in-game live runs on that build (System page, field, Impa's lesson).

I'll post the review's findings, with what I changed for each, and the in-game results here as soon as they're in.

@hckrman101 the branch is on the latest main (no newer commits upstream) at b846ee78, with the race fix from my last comment. Since main keeps moving, I'm pushing first and testing after: - **Pushed:** b846ee78. It builds on Linux (`make_game.py --update`). - **Running now:** a full code review of the branch, and the in-game live runs on that build (System page, field, Impa's lesson). I'll post the review's findings, with what I changed for each, and the in-game results here as soon as they're in.
gab9281 commented 2026-11-11 17:39:23 +00:00 (Migrated from github.com)

@hckrman101 I ran a full code review of the branch at b846ee78 (the race fix). It found 10 issues. Here is each one and what I did. The fixes are in 1da85707, on the latest main.

Fixed:

# Finding Fix
1 When the game makes the textures' pool again, the put-back ranges still held the last change's runs, not the served file's. After that, a change could leave a start-face block that the new runs don't cover. The served runs are kept (tex.served). A pool made again restores them along with the served face and controls.
2 The layouts' region was found once and never checked again, so a change could write into memory the game had reused. live_layouts checks the file's head at lay.base before writing. If it no longer matches, it drops the region and writes nothing.
3 A part replaced by a mod's own file left romfs_state at 0, so the row kept asking for a restart. A mod's file now sets that part's state to "none", like a part never served.
4 While the menu asked for a restart (a part not found yet), a change still reached the parts that were found, mixing faces until the restart. Until every part is live, a change waits. Nothing changes before the restart, or before the last part is found, and then every part catches up together.
8 The glyph hook accepted a stream of font.n bytes (header included) but compared the head at offset 0, so that case could never match. It compares at +8 then. A same-size font that isn't ours is marked as seen, so it isn't checked again.
9 A part's start state was recorded before its file was written, and taken from whichever file was served first. Each part records the face and controls of its own file, and only once that file is written (part_served).
10 find_layouts dropped a run that a later, overlapping run had partly overwritten, so its bytes were never put back. A byte that a later run (by idx, write_patched's order) covers no longer counts against the run.

Rebase fallout, also fixed: #2270's test_mods_size_table compiles prompts.c whole, and it no longer compiled on this branch. Its hos.h and main.c now stub what the live code reads (guest memory, address regions, the clock, rx_option). Its font moved to prompts/External.bfotf, the single font this branch serves. All 10 of its cases pass.

Left as agreed: the npad-thread file reads (#5 and #6) and the per-draw text scan after the first change (#7). These are the performance points we already discussed; a worker thread can come later if anyone sees a hitch.

Proof: test_prompts_live, test_mods_size_table, test_prompt_font_bind, test_menu_hooks, the input tests and the NVN tests pass. Since main keeps moving, I pushed before the rest: the full suite and the in-game runs on this head come next, and I'll post them here.

@hckrman101 I ran a full code review of the branch at b846ee78 (the race fix). It found 10 issues. Here is each one and what I did. The fixes are in 1da85707, on the latest main. **Fixed:** | # | Finding | Fix | |---|---|---| | 1 | When the game makes the textures' pool again, the put-back ranges still held the last change's runs, not the served file's. After that, a change could leave a start-face block that the new runs don't cover. | The served runs are kept (`tex.served`). A pool made again restores them along with the served face and controls. | | 2 | The layouts' region was found once and never checked again, so a change could write into memory the game had reused. | `live_layouts` checks the file's head at `lay.base` before writing. If it no longer matches, it drops the region and writes nothing. | | 3 | A part replaced by a mod's own file left `romfs_state` at 0, so the row kept asking for a restart. | A mod's file now sets that part's state to "none", like a part never served. | | 4 | While the menu asked for a restart (a part not found yet), a change still reached the parts that were found, mixing faces until the restart. | Until every part is live, a change waits. Nothing changes before the restart, or before the last part is found, and then every part catches up together. | | 8 | The glyph hook accepted a stream of `font.n` bytes (header included) but compared the head at offset 0, so that case could never match. | It compares at `+8` then. A same-size font that isn't ours is marked as seen, so it isn't checked again. | | 9 | A part's start state was recorded before its file was written, and taken from whichever file was served first. | Each part records the face and controls of its own file, and only once that file is written (`part_served`). | | 10 | `find_layouts` dropped a run that a later, overlapping run had partly overwritten, so its bytes were never put back. | A byte that a later run (by `idx`, `write_patched`'s order) covers no longer counts against the run. | **Rebase fallout, also fixed:** #2270's `test_mods_size_table` compiles `prompts.c` whole, and it no longer compiled on this branch. Its `hos.h` and `main.c` now stub what the live code reads (guest memory, address regions, the clock, `rx_option`). Its font moved to `prompts/External.bfotf`, the single font this branch serves. All 10 of its cases pass. **Left as agreed:** the npad-thread file reads (#5 and #6) and the per-draw text scan after the first change (#7). These are the performance points we already discussed; a worker thread can come later if anyone sees a hitch. **Proof:** `test_prompts_live`, `test_mods_size_table`, `test_prompt_font_bind`, `test_menu_hooks`, the input tests and the NVN tests pass. Since main keeps moving, I pushed before the rest: the full suite and the in-game runs on this head come next, and I'll post them here.
gab9281 commented 2026-11-11 17:41:57 +00:00 (Migrated from github.com)

@hckrman101 thanks for the approval. On your two nits:

  • textures.bin read under tex_lock: fixed in d0dcc7f1. hos_prompts_live reads the face's textures.bin (texture_bin) before taking the lock, so a finalize waits only for the writes. put_runs's put-back still reads Common.bntx under the lock, since those bytes go into the pool.
  • No textures.bin asking for a restart: this was fixed in 10615af4, which is in the head you approved. Without the file, build_textures returns 0 (if (!t) return 0;), so hos_prompts_romfs sets the textures' state to 3. hos_prompts_live_parts then counts that part as live, as long as some other part was found. Since 1da85707, a part a mod replaces counts the same way.

Note that the approval was at b846ee78. Since then:

  • 1da85707 adds the fixes from my own review (table in the comment above).
  • d0dcc7f1 adds this nit.

Both are on the latest main. CI and the in-game runs of this head are next; I'll post them here.

@hckrman101 thanks for the approval. On your two nits: - **`textures.bin` read under `tex_lock`:** fixed in d0dcc7f1. `hos_prompts_live` reads the face's `textures.bin` (`texture_bin`) before taking the lock, so a finalize waits only for the writes. `put_runs`'s put-back still reads `Common.bntx` under the lock, since those bytes go into the pool. - **No `textures.bin` asking for a restart:** this was fixed in 10615af4, which is in the head you approved. Without the file, `build_textures` returns 0 (`if (!t) return 0;`), so `hos_prompts_romfs` sets the textures' state to 3. `hos_prompts_live_parts` then counts that part as live, as long as some other part was found. Since 1da85707, a part a mod replaces counts the same way. Note that the approval was at b846ee78. Since then: - 1da85707 adds the fixes from my own review (table in the comment above). - d0dcc7f1 adds this nit. Both are on the latest main. CI and the in-game runs of this head are next; I'll post them here.
hckrman101 (Migrated from github.com) approved these changes 2026-11-11 17:49:55 +00:00
hckrman101 (Migrated from github.com) left a comment

Looked at the three commits since my last review (b846ee7 → d0dcc7f). Still good to go.

  • texture_bin now reads the face's textures.bin before tex_lock, so a finalize on the game's thread only waits for the writes. That was my last nit, thanks. It's only ever called from the input thread (under busy), so reading tex.bins outside the lock is fine.
  • Keeping the served runs and putting them back as tex.l.ranges when the pool is found again reads right, and the malloc failures fall back cleanly.
  • Not changing anything until every part is live (so no half change across faces), the layouts' header check before writing, and the mod's own file counting as "the same whatever the face" all look correct. run_as_served handling later runs written over earlier ones also makes sense.

One small thing, not blocking: live_layouts checks the header and then writes without anything stopping the game from letting that memory go in between, unlike the textures, which now have tex_lock. The window is tiny and the layouts don't go through a pool finalize, so I'm fine with it as is. It might be worth a comment.

CI is still pending on this head, so it's worth a green run before merging.

Looked at the three commits since my last review (b846ee7 → d0dcc7f). Still good to go. - `texture_bin` now reads the face's textures.bin before `tex_lock`, so a finalize on the game's thread only waits for the writes. That was my last nit, thanks. It's only ever called from the input thread (under `busy`), so reading `tex.bins` outside the lock is fine. - Keeping the served runs and putting them back as `tex.l.ranges` when the pool is found again reads right, and the malloc failures fall back cleanly. - Not changing anything until every part is live (so no half change across faces), the layouts' header check before writing, and the mod's own file counting as "the same whatever the face" all look correct. `run_as_served` handling later runs written over earlier ones also makes sense. One small thing, not blocking: `live_layouts` checks the header and then writes without anything stopping the game from letting that memory go in between, unlike the textures, which now have `tex_lock`. The window is tiny and the layouts don't go through a pool finalize, so I'm fine with it as is. It might be worth a comment. CI is still pending on this head, so it's worth a green run before merging.
gab9281 commented 2026-11-11 17:50:45 +00:00 (Migrated from github.com)

@hckrman101 thanks. I added the comment in 3f124e4b and made it more precise in 8a7ea243 (comments only). It's at the layouts' header check in live_layouts and says:

  • unlike the textures' pool, nothing holds the region from the check to the writes;
  • the game frees its resources' regions on its own thread, with no hook here;
  • new runs are still written only where the bytes are the file's (guarded), but the last change's bytes put back are not checked that way.

The in-game runs of b846ee78 on Linux passed: System page, field and Impa's lesson, all shots taken, every change reaching the textures, layouts and font. The full suite and the in-game runs of this head are next. I'll post them with CI.

@hckrman101 thanks. I added the comment in 3f124e4b and made it more precise in 8a7ea243 (comments only). It's at the layouts' header check in `live_layouts` and says: - unlike the textures' pool, nothing holds the region from the check to the writes; - the game frees its resources' regions on its own thread, with no hook here; - new runs are still written only where the bytes are the file's (guarded), but the last change's bytes put back are not checked that way. The in-game runs of b846ee78 on Linux passed: System page, field and Impa's lesson, all shots taken, every change reaching the textures, layouts and font. The full suite and the in-game runs of this head are next. I'll post them with CI.
gab9281 commented 2026-11-11 18:15:18 +00:00 (Migrated from github.com)

@hckrman101 the head changed after your approval: the in-game runs of d0dcc7f1 caught a regression of mine, fixed in 6cb2a3e9.

The regression: in 1da85707, for review point #8, I made the glyph hook look for the font 8 bytes in when the stream's size counts the header. The game keeps the font decrypted at the stream's start in that case too, so on d0dcc7f1 the font was never found. With "a change waits until every part is live", no change showed at all: 0 changes applied in either run. 6cb2a3e9 looks at both places. A font that doesn't match (yet) is checked again at its next glyph, as before 1da85707, so the hook no longer marks a face as seen after a mismatch.

Results on 6cb2a3e9 (Linux, in game):

  • The font, the textures' pool and the layouts are all found (prompts: the font at 0x7fd39770c8: the icons in texts follow at once).
  • live.sh and live3.sh ran with every shot taken and every change applied (30 and 6, as on b846ee78).
  • System page reopened: the PS5 picture with "Target R1".
  • Impa's lesson after a live switch to PS5: her dialogue, her notes and Link's replay with "Play a note" all show PS5.

Tests: the full suite on d0dcc7f1 gave 348 passed, 0 failed, 49 skipped. 6cb2a3e9 changes only the glyph hook, and test_mods_size_table (which compiles prompts.c whole) passes on it.

CI on this head: every job passes except Python source checks (windows-2022, part 1/2). That job fails on test_native_astc_texture_count ('scanf' is deprecated, from #2288), which fails the same way on main (run 43074702922). Main has moved since. I can rebase once that's fixed, or before merging, as you prefer.

@hckrman101 the head changed after your approval: the in-game runs of d0dcc7f1 caught a regression of mine, fixed in 6cb2a3e9. **The regression:** in 1da85707, for review point #8, I made the glyph hook look for the font 8 bytes in when the stream's size counts the header. The game keeps the font decrypted at the stream's start in that case too, so on d0dcc7f1 the font was never found. With "a change waits until every part is live", no change showed at all: 0 changes applied in either run. 6cb2a3e9 looks at both places. A font that doesn't match (yet) is checked again at its next glyph, as before 1da85707, so the hook no longer marks a face as seen after a mismatch. **Results on 6cb2a3e9 (Linux, in game):** - The font, the textures' pool and the layouts are all found (`prompts: the font at 0x7fd39770c8: the icons in texts follow at once`). - `live.sh` and `live3.sh` ran with every shot taken and every change applied (30 and 6, as on b846ee78). - System page reopened: the PS5 picture with "Target R1". - Impa's lesson after a live switch to PS5: her dialogue, her notes and Link's replay with "Play a note" all show PS5. **Tests:** the full suite on d0dcc7f1 gave 348 passed, 0 failed, 49 skipped. 6cb2a3e9 changes only the glyph hook, and `test_mods_size_table` (which compiles `prompts.c` whole) passes on it. **CI on this head:** every job passes except `Python source checks (windows-2022, part 1/2)`. That job fails on `test_native_astc_texture_count` (`'scanf' is deprecated`, from #2288), which fails the same way on main (run 43074702922). Main has moved since. I can rebase once that's fixed, or before merging, as you prefer.
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
hckrman101
Labels
Clear labels   
accessibility
Barrier affecting people with disabilities

  
autorelease: pending

   
autorelease: tagged

   
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
testing wanted

   
wontfix
This will not be worked on

No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
MagicBeanDev/Hookshot!2366
No description provided.