WIP: feat(input): hold-to-toggle for any bound button #137
No reviewers
Labels
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!137
Loading…
Reference in a new issue
No description provided.
Delete branch "simonwjackson/Longshot:feat/input-hold-to-toggle"
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?
Why
Players who cannot keep a button held need another way to shield, target or aim. Hold-to-toggle makes one press hold a bound button and the next release it.
Carried over from https://github.com/NotRexed/Hookshot/pull/2886.
What changed
toggle_<button>=0|1. Plus, Minus, directions and note-only rows cannot toggle.docs/tested-configurations.md.Flags and settings
toggle_<button>=0/1HOS_INPUT_BACKGROUND=0/1/ Controller in backgroundMeasurements
main @
f3fe1763ddwas merged in commit68b95c1(no rebase or force-push).this PR @
3582a10e01; the new Linux game build usesbc0c1292d8ce377c1d6fbcc6dc44372b8f6f74ec. Later commits change only tests and recorded evidence.Review-fix proof
Baseline: pre-fix feature @
68b95c1(current main merged, before the review fix). Main alone has no latch, so these game-free regression checks compare the feature before and after the fix, using its real input/Npad path.68b95c1bc0c1292Fresh build:
rev137-bc0c1292 (source f94c58fd), Gitbc0c1292, on an RX 7900 XT / RADV Mesa 26.2.3, Ryzen 5 7600X, 32 GB, NixOS 26.11 / Linux 7.2.7. Private headless Sway 1.12 + Xwayland, 1920×1080, render 100%, game upscaler, FG/HDR Off, forced mesh fallback, udmabuf enabled, audio off, copied save. Hyrule Castle Town (Market_Child), indoors by the wooden door. One 10m06s functional session, clean control-socket exit, no crash/device-lost/hang marker. Another isolated CPU build ran concurrently, so this is not an FPS comparison.bc0c1292Reproduce: enable ZR/ZL Toggle and Mouse camera Native; click the game to capture the mouse, tap ZR and release, then move the mouse. Tap ZR again to release/fire and B to leave the aiming mode. In normal play, tap ZL twice to check shield hold/release. Open Insert to clear a new latch. The test sent relative XTest events to the private Xwayland display; xdotool pointer warps did not produce relative motion and are not counted as proof.
The real
read_input/Npad regression fixture failed before the fix on latched L pointing and background pads. It now passes for L/R/ZL/ZR, all three mouse modes and enabled background input. Manually supplied Plus/Minus flags cannot change their ordinary holds.Historical performance comparison (before this review revision)
Baseline: main @
316f9319. This PR @0f4b3374is the old measured feature head, not the build above. These four runs remain useful history but do not measure the revised mouse route or current-main performance.Hardware: AMD RX 7900 XT 20 GB, RADV Mesa 26.2.3; Ryzen 5 7600X, 32 GB RAM; NixOS 26.11, Linux 7.2.7. Private headless Sway 1.12/Xwayland; udmabuf enabled;
RX_VK_FORCE_MESH_FALLBACK=1; speakers disabled.Scene: Hyrule Castle Town (
Market_Child), stationary at the copied save's wooden door. 1920×1080 drawable and output, 100% render resolution, game's own upscaler, frame generation Off, game frame-rate cap Uncapped, VSync Auto, HDR Off. Effective menu values were read through the control bridge. No overlays, injectors or external shader-cache tools.Method: One short session, main → PR → main → PR, with fresh copies of the same save. Each process warms at the stationary scene for at least 10 s, then samples for 20 s; the last five
vk: N presents, X fpslog lines per run are retained. The per-run median and median of the two run medians are below. These are native-present measurements at the settings below, not a claim about other scenes or 1% lows.How to reproduce: Load a disposable copy of a Castle Town save and stay by the same wooden door, without movement/camera input. Use the settings above, with ZL Toggle On; no held button during FPS samples on this PR; baseline main has stock behavior. Alternate the two binaries twice, let each settle, and take the last five presents-log readings per run. The additional feature proof below is separate from these stationary FPS samples.
316f93190f4b3374Latch proof: on this PR with ZL Toggle On, a 120-present press covered frames 22514–22633. At frame 22775 the control bridge reported
input.queued=0, and the inspected capture still shows the shield raised. The next 16-present press covered frames 22778–22793; the next capture shows the shield back on Link's back. On main, the same finite 120-present press released the shield normally.The old session's optional menu navigation did not finish; those attempts were not counted as reset or Off-setting passes. The fresh functional session above now verifies the Insert reset. Neither the old nor new session verifies physical controllers or Windows.
Exact presents-log samples (five per run)
Testing
Tested: Full Linux suite under the shared lock: 512 passed, 5 failed, 16 skipped in 497 seconds. Three failures were extracted-fixture compilation errors caused by the new latch type/pause constants;
7720460fixes them without weakening assertions. The final focused run (including all three) is 14 passed, 0 failed. The two remaining failures also reproduce on untouched mainf3fe1763:test_linux_tools.py(four assertions, its fixed/usr/bin:/binPATH on Nix) andtest_menu_i18n.py(stale Portuguese debug-help text).test_display_pointer.pyandtest_texture_staging_publication.pypass on main. The fresh Linux NUP build and shader preparation both passed; tracked source hashes matched the pinned archive before and after the build. The game-free fixtures compile the real input and menu paths.git diff --checkpasses. The project has no standalone formatter, linter or typechecker; compiler checks run through the fixtures and Linux build.Only isolated folders, a private compositor and copied test saves were used. No installed game, live save, owner game session or
korri*unit was changed. New screenshots below were opened and inspected; attachments load without login.Not tested: Windows native builds/gameplay or non-ASCII Windows paths, physical controllers, other GPUs, HDR, a full playthrough and every feature combination. There is no Windows machine available. The old main/PR FPS comparison was not rerun on this head; the fresh run proves the review fix.
Needs other testers:
Screenshots
Risk and rollback
A latched action can surprise the player. There is no separate held-button indicator; scene changes, cutscenes and disconnects do not universally reset it. Linux title/game-over/save dialogs are not guaranteed resets. Open Insert to release, or set the button's Toggle Off. Long binding labels can still clip. Reverting the feature is sufficient.
Reviewer notes
Read
host/nn_hid.c's mouse routing and Npad latch first. No publicMkwVkHost/MkwVkRendererstructure changes, so this feature needs no API-version bump. Option 54 overlaps #196 and #152; coordination notes cover renumbering and Sprint seeing toggled A as held. A HUD indicator and a full Linux port of Windows-only menu tracking are deliberately outside this input fix. The help does not recommend an unverified built-in Targeting setting.Checklist
For everyone
type(scope): description).main(f3fe1763) into the branch (68b95c1); not rebased.python -I -B tests/run_all.pyrun and passing (any failure that also fails onmainis named above).third_party/untouched (a new folder with aPROVENANCE.mdinstead).If applicable
Assisted-by:/Co-Authored-By:trailers.docs/tested-configurations.mdfor each measured run.mainand passing here. The mouse/background regression is against the pre-fix feature head merged with main; main alone has no latch.tests/that reads changed source updated in this PR, not weakened.Code review only (diff and surrounding code read; no build, tests or game run). Commenting rather than approving because the PR is WIP.
The change holds together. The latch in
host/hold_toggle.his small and correct: a press edge flips it, a repeat doesn't, and a reset blocks every enabled button until it is released. It sits in the one place every input path meets (npad_states, after remapping, for No.1 only). It costs nothing while every flag is Off. The flags round-trip throughsave()andmenu_init()without duplicate lines, and Reset to defaults and rebinding both clear the latch. I found no blocking bug.On the
pr statushint:vk_api.honly gains an option ID, carried by the existingoptioncallback.MkwVkHostandMkwVkRendererdon't change, soMKWVK_API_VERSIONdoesn't need a bump and that box shouldn't be added. ID 54 is still free on current main.Suggestions, none of them blocking:
pad_/key_but not the Toggle column ortoggle_<button>=0|1.What keeps it WIP is in the checklist already: a run of this Longshot build in the game, interleaved main/PR measurements, and the Windows llvm-mingw build and test request.
@ -3304,2 +3307,2 @@const float a = col_x[col] + (col == kCtlPrimary ? 0 : 3 * s), b = col_x[col + 1] - (keyboard ? 0 : 3 * s);const std::string v = keyboard ? keys_text(ctl.button) : disabled ? "-" : pad_name(host->binding(HOS_BIND_PAD, ctl.button, col));const bool keyboard = col == kCtlKeyboard, toggle = col == kCtlToggle;const bool disabled = (keys && !keyboard) || (toggle && ctl.button >= HOS_SW_N);Suggestion: also disable Toggle for
HOS_SW_PLUSandHOS_SW_MINUS.npad_statestreats their press edge as a reset, so turning their Toggle On never latches anything. Instead it turns a held Plus or Minus into a one-frame tap. The cell would show On without doing what the help text promises.menu_tick'scolumns(line 4931) would need the same condition.@ -11,6 +11,7 @@while one of the game's own menus is open (host/oot_menu.c: hos_mouse_game_menu each frame it runs), its clicks andwheel for that menu (hos_mouse_game_pointer). */#include "hos.h"void hos_input_clear_toggles(void);Nit: the same prototype is declared again in
host/posix/display_sdl.c. Both files includehos.h, andhos.hwould keep the one definition innn_hid.cand its callers in step.@ -189,6 +190,7 @@ static void gm_cursor_off(HWND w) {back, if the window still has the focus */static void menus_mouse(HWND w, int insert_open, int game_open) {const int was = menu_open || game_menu_open, now = insert_open || game_open, game_was = game_menu_open;if ((insert_open && !menu_open) || (game_open && !game_was)) hos_input_clear_toggles();Windows also clears latches when one of the game's own menus opens (
WM_APP + 10, fromhost/oot_menu.c).display_sdl.chas no equivalent, so Linux only clears on Plus or Minus innn_hid.c. That's probably harmless if every game menu opens through Plus or Minus, but then this extra clear isn't needed. If some menus open another way, Linux misses them. Either way, the description's claim that the two paths are kept in step isn't quite true.@ -1088,1 +1093,4 @@const int menu_open = rx_option(1, 0);static HoldToggle toggles;const uint64_t enabled = (uint32_t)rx_option(HOS_OPT_HOLD_TOGGLE, 0);const int reset = InterlockedExchange(&toggle_reset, 0) || menu_open || (enabled && !game_in_front());Question:
enabled && !game_in_front()resets the latch on every poll while the window is in the background.read_inputstill passes pads through in that state when the player turned on pads in the background (option 4,HOS_INPUT_BACKGROUND) and for a scripted run. With Toggle On, those players get ordinary hold input, but only for the toggled buttons. If that's intended, a word in the help text would cover it. If not, the condition could followpads_in_background. Focus loss is already handled by the display'shos_input_clear_toggles()onWM_KILLFOCUSandHOS_SDL_EV_FOCUS_LOST.Added a fresh interleaved main/PR Linux measurement session at the named commits, the last-five-sample FPS medians, feature proof and inspected screenshots. Windows testing requests are open; the description names the remaining gaps. The branch also rebases cleanly onto main
1be180bbwith an identical stable patch ID and passing focused tests; the force-with-lease push is still gated, so the measured remote head stays pinned. No new baseline FPS is claimed for the local rebase. The extra synthetic menu-navigation attempt is explicitly excluded from the successful latch proof.WIP: feat(input): hold-to-toggle for any bound buttonto feat(input): hold-to-toggle for any bound buttonCode review only: I read the diff and the code around it. I didn't build it, run the tests or play the game.
The latch itself is right.
hold_toggle_applyflips on a press edge and ignores a repeat. After a reset it blocks every enabled button until that button is let go, and turning a button's Toggle Off drops its latch at once (latched &= enabled). The latch sits after remapping innpad_states, so both gamepad slots, the keyboard, mouse buttons and the control bridge share one latch. The default is Off for every button, andtoggle_lines survive a save without duplicates. These all clear the latch: the Insert menu, focus loss (both platforms), rebinding, Reset to defaults, and Plus or Minus. Stick directions and ocarina note rows get no Toggle cell. Steam Input and the left stick's dead zone are untouched, because the latch only sees buttons after remapping. The earlier review is still open in full: the head is unchanged since then (0f4b3374).Must change
read_inputdecides the mouse's part from the buttons before the latch. It callshos_mouse_native_free(!(*buttons & game_stick))for ZR, L and R, and passesmouse_stick's pointing flag from L. So with Mouse camera Native and ZR or R toggled, the mouse turns the camera's angles while the game is aiming or shielding, instead of acting as the right stick that aims. With L toggled, the item wheel never gets the pointing mouse, in either mouse mode. Aim and shield are the uses this feature is for. Inline comment onhost/nn_hid.c.316f9319. main@origin is nowfbd42024(#198), 55 commits on. The description says the rebase onto1be180bbwas never pushed, and1be180bbis itself about 38 commits behind main. The text merges without conflicts againstfbd42024; I checked the 10 files both sides touch with a three-way merge. Main has changedread_input's mouse modes, though (Mouse camera Off, #178), and the same fixtures. Rebase, runtests/run_all.pyon the result, and then tick the box.toggle_<button>=0|1ini key, but nothing is documented: the README's Controls section and its options-file block (README.md lines 684 to 696), and docs/development.md "The options file" (line 558). The "Flags noted … switch reference updated" and "Docs updated" boxes were deleted, but they apply. The earlier review also asked for the README.docs/tested-configurations.md" became "a folded table below", and the file is unchanged. AGENTS.md asks for the rows in that file.Should change
HOS_INPUT_BACKGROUND),enabled && !game_in_front()resets every poll. On that pathhold_toggle_applyreturnsdown & ~enabled, so a toggled button does nothing at all behind another window, not even an ordinary hold. Followpads_in_backgroundthere, or leave the button unlatched instead of dropping it.WM_APP + 10), buthost/oot_menu.cis Windows-only. On Linux, a menu that doesn't open through Plus or Minus (the game-over and save dialogs, the title) keeps the latch. The help text says the menu releases it.HOS_OPT_SPRINT) and #152 (HOS_OPT_KEYBOARD) also use 54, and both bumpMKWVK_API_VERSIONto 22. Whichever merges second must renumber. A note in those PRs would avoid a silent clash.sprint_stepafter this latch, so a toggled A looks to Sprint like A held by the player. Worth a look when the two meet.Not checked: any run in the game, the Windows build, and whether the Controls page's 128 hit boxes are enough when every row fits on screen. On main the 96 boxes were already short of a full grid, so that limit predates this PR.
@ -55,2 +55,4 @@90, 120, 144, 165, 240, 6 Uncapped; a change applies in play) */enum { HOS_OPT_FPS_CAP = 53 };/* Hold-to-toggle's enabled Switch buttons, 1 << HOS_SW_*. Off (zero) by default; the existing option callback. */enum { HOS_OPT_HOLD_TOGGLE = 54 };#196 (
HOS_OPT_SPRINT) and #152 (HOS_OPT_KEYBOARD) also take 54. Whichever merges second must renumber; worth a word on those PRs.@ -3304,2 +3307,2 @@const float a = col_x[col] + (col == kCtlPrimary ? 0 : 3 * s), b = col_x[col + 1] - (keyboard ? 0 : 3 * s);const std::string v = keyboard ? keys_text(ctl.button) : disabled ? "-" : pad_name(host->binding(HOS_BIND_PAD, ctl.button, col));const bool keyboard = col == kCtlKeyboard, toggle = col == kCtlToggle;const bool disabled = (keys && !keyboard) || (toggle && ctl.button >= HOS_SW_N);Still open from the earlier review: also disable the cell for
HOS_SW_PLUSandHOS_SW_MINUS(and thecolumnsinmenu_tick). Their press edge is a reset innpad_states, so On only turns a held Plus or Minus into a one-frame tap.@ -1088,1 +1093,4 @@const int menu_open = rx_option(1, 0);static HoldToggle toggles;const uint64_t enabled = (uint32_t)rx_option(HOS_OPT_HOLD_TOGGLE, 0);const int reset = InterlockedExchange(&toggle_reset, 0) || menu_open || (enabled && !game_in_front());On the
!game_in_front()reset,hold_toggle_applyreturnsdown & ~enabled. With pads counting in the background (option 4 orHOS_INPUT_BACKGROUND), a toggled button is then dropped completely, not passed through as an ordinary hold. Followpads_in_backgroundhere, or pass the enabled buttons through unlatched while the reset is from the background.@ -1089,0 +1096,4 @@const int reset = InterlockedExchange(&toggle_reset, 0) || menu_open || (enabled && !game_in_front());/* Pause/map must release shield/aim before the game's menu reads input. */const uint64_t pause = buttons & ~toggles.previous & ((1ULL << B_PLUS) | (1ULL << B_MINUS));buttons = hold_toggle_apply(&toggles, buttons, enabled, reset || pause);read_inputhas already decided the mouse's part from the unlatched buttons:hos_mouse_native_free(!(*buttons & game_stick))for ZR, L and R, andmouse_stick(..., *buttons >> B_L & 1). With ZR or R toggled and Mouse camera Native, the mouse turns the camera's angles while the game aims or shields, instead of acting as the right stick that aims. With L toggled, the item wheel loses the pointing mouse. Maketogglesfile-static and use(*buttons | toggles.latched) & game_stickthere, and the same for L. Last poll's latch is close enough.Thanks for review 1150. I merged main
f3fe1763dda267a134494f1d21e6ad10b1ec4cd8with merge commit68b95c1, rather than rewriting the branch. The description now states that explicitly.bc0c129makes the latch file-static. Native's ZR/L/R decision and L's pointing flag both use*buttons | toggles.latched. The regression fixture exercises the realread_inputand Npad writer for L/R/ZL/ZR in all three mouse modes. L pointing and background-pad tests failed before the fix and pass after it.toggle_<button>=0|1, defaults, exclusions and reset limits. The original checklist boxes are restored, including the unticked Windows build box and its open test request T-NDNY.547f5f7adds the four previously recorded runs todocs/tested-configurations.md, below the day's existing rows, with their original commits. New proof is recorded separately, not presented as a repeat of the old FPS comparison.pads_in_background, including the environment override. Losing focus still clears once; a new background pad press can latch when background pads are enabled.bc0c129, test in72e2b34). Title/game-over/save-dialog tracking remains Windows-only. Porting that whole mouse/menu tracker is not a cheap, safe input fix. Help and docs now name Insert and Plus/Minus rather than promising every Linux game dialog clears a latch.Full Linux suite under the shared lock: 512 passed, 5 failed, 16 skipped in 497 seconds. Three failures were extracted-fixture compilation errors caused by the new latch type/pause constants;
7720460fixes them without weakening assertions. The final focused run (including all three) is 14 passed, 0 failed. The two remaining failures also reproduce on untouched mainf3fe1763:test_linux_tools.py(four assertions, its fixed/usr/bin:/binPATH on Nix) andtest_menu_i18n.py(stale Portuguese debug-help text).test_display_pointer.pyandtest_texture_staging_publication.pypass on main. The fresh Linux NUP build and shader preparation both passed; tracked source hashes matched the pinned archive before and after the build.Fresh game proof (
bc0c1292, same production code as head3582a10): Native mouse + latched ZR moved the slingshot aim with relative mouse events after the press ended; ZL latched/released on successive presses; opening Insert cleared a new shield latch. Inspected Controls screenshot shows Plus/Minus disabled. The new run is indocs/tested-configurations.md(3582a10). Six screenshots are PR attachments, byte-checked without authentication; the private-fork links are removed.The current-head two-pair FPS comparison and Windows build boxes remain unticked with reasons. The older named performance runs remain explicitly historical, not measurements of this revised head. No new full-suite performance claim or Windows pass is implied.
Please re-review the updated head.
docs/forge.mdhas no separate re-review request command, so this is the request.feat(input): hold-to-toggle for any bound buttonto WIP: feat(input): hold-to-toggle for any bound buttonView command line instructions
Checkout
From your project repository, check out a new branch and test the changes.