WIP: feat(input): add a held or toggled Walk key with adjustable speed #66
No reviewers
Labels
No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
needs measurements
question
testing wanted
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!66
Loading…
Reference in a new issue
No description provided.
Delete branch "mooki/Longshot:walk-key"
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
Keyboard players should be able to walk instead of always running. A rebindable Walk key gives the movement keys a partial stick tilt, either while held or from one press to the next.
This remains work in progress: the feature works in the Longshot runs below, but the shared PC varied too much to establish its cost while unused. The default-key preference is also still a maintainer decision; this move keeps Caps Lock.
What changed
walk_held(),walk_percent()andmove_keys_stick()inhost/nn_hid.cscale only the movement keys. Hold is the default; Toggle changes state on a fresh press, not a held key returning from a menu/focus change. Running, controllers, camera keys and mouse input keep their own tilt.HOS_KEY_WALKto the key enum/name tables; display Walk after Camera right and before the ocarina notes.binds_load()preserves older mappings: if another control already owns Caps Lock, Walk starts unbound and logs why. No saved-options version change.gpu/vk/vk_menu.cpp/.h, with the existing binding/rebinding UI and live renderer callbacks invk_replay.cpp. The default remains 45%, range 15–90% in steps of five.host/hos_options.chere:host/rx_backend.cforwards option reads, andhost/nn_vi.creads the cap. No host/renderer table entry changes, so API version 21 stays.README.mdinstead ofreadme.md. Preserve all historical result rows and add the five new Longshot attempt/run rows.Flags and settings
No new environment variable, build flag or command-line flag is added. Internal identifiers below are callback/key IDs, not environment switches.
HOS_OPT_WALK_MODE, 49)walk_modeHOS_OPT_WALK_SPEED, 50)walk_speedHOS_KEY_WALK)key_walk20,0; unbound if an old mapping already uses Caps LockMeasurements
Branch state: on main
d725b3631982ed0035ea3ac9cbe1677e9fad260c; head629766eb206386d0777aa262b212d7e07f530d23; whole game-free suite on this head:511 passed, 1 failed, 20 skipped(maind725b3631982ed0035ea3ac9cbe1677e9fad260c:510 passed, 1 failed, 20 skipped); the commits are unchanged by rebases apart from the results table.The only failure on both heads is
tests/test_updater_asset_digest.py: local .NETNU1100, unable to resolve the missingMicrosoft.AspNetCore.App.Ref (= 10.0.13)reference pack for net10.0.main:
d4a69456(2026-11-12), with the same local test probe as this PRthis PR: walk-key @ 3fc3c891, with that local test probe
Hardware: RTX 4090 24 GB, NVIDIA 615.78.08; Core i9-13900K, 96 GB; Arch Linux, kernel 7.2.15-1-omarchy
Scene: same copied Kokiri Forest save, daytime; 1920×1080, Custom preset, game's own upscaler at 100%, frame generation off, cap 60, shader precompile off; windowless
Method: main → this PR → main → this PR, four completed runs of 151.0 / 206.4 / 116.0 / 134.4 seconds. Wait for ten fresh controllable scene samples, warm up another 20 seconds, observe 40 seconds with Walk unused, then exercise the keys. Each movement median is nine game-set-speed readings after the first reading of the hold. FPS medians use complete 120-present log blocks inside the unused window. Ordinary shared runs, not a controlled performance measurement.
How to reproduce: on a Kokiri Forest save, set the settings above; hold W or S with Caps Lock, then without it. Switch Walk mode to Toggle, tap Caps Lock, move, tap it again, and move again. Inspect Walk mode/speed and the Walk binding on Controls. Run
python -I -B tests/test_walk_key.pyandpython -I -B tests/test_option_ids.pyfor the game-free checks. The automated runs used a local test probe because the current bridge has no keyboard input; a person pressing the key was not part of this check.Main
d725b363differs from measuredd4a69456only by scripts, docs and tests, not game code; every carried patch is unchanged and the five product files are byte-identical to the measured head, so the game observations still hold.The identical, uncommitted probe on both builds fills the Linux key table that the normal keyboard path reads and observes the game's controller-speed setter/position. It does not replace Walk with an analog controller command. The bridge supplies title input, settings and captures. A test-only menu-key hook and headless list-scroll step make the rows visible. Both probes were removed from the source worktrees after building; no probe or game-derived data is in the PR.
d4a69456Off-cost is inconclusive. Main alone changed from 27.45 to 60.00 FPS; the branch from 29.00 to 56.25. The 1.10-FPS gap between their arithmetic medians is much smaller than either build's repeat spread, but that large spread prevents a useful “no visible cost” conclusion. This is not evidence that Walk is faster, slower or free. A controlled comparison remains necessary before marking this feature ready.
d4a69456+ probe3fc3c891+ probed4a69456+ probe3fc3c891+ probeObserved movement output, extracted from the retained samples (not a percentage multiplied into an assumed game speed):
The central log lines from each sorted unused-window population produce its median; all blocks are retained, not just these central lines:
All four completed runs had zero fatal/crash/device-loss/core-escape/kernel-fault counts and complete journal coverage. A separate 59.8-second cold attempt is rejected, not counted as a fifth measurement: the driver sent a blocking A during loading, timed out waiting for a controller read and requested quit. Game exit was 0 with no fault lines; it reached no stable movement or unused window. The corrected driver required an advancing menu before title input. No binary changed to fix that test-driver mistake.
The whole suite used a fresh HOME and repository-default compiler selection after the last documentation commit. Separate check output:
The preliminary suite on
3fc3c891was 509/1/20. The earlier shared main suite at measuredd4a69456was 507/2/20: the same .NET failure plus a timing-dependent AppImage retry fixture failure under heavy load, whose isolated follow-up passed all nine cases. No package was installed and no failure is called a pass. The whole suite includes the new description/forge tests fromd725b3631.The shared main suite was run once for all moved pull requests of this day.
The unchanged Walk test against measured main rejects the absent helper and option allocation (one error and one failure before behavioral compilation); that is an absence/wiring negative, not a baseline game-speed fixture. The actual game comparison above supplies the behavioral before result. Five changed input fixture units passed Windows-target syntax; the option-ID driver separately used the real Windows-target compiler exits for every expected collision. Two unused-variable warnings in the ocarina binding fixture remain visible. No Windows execution is implied.
Historical GitHub checks were green on Windows and Ubuntu at
45f3e06c2ed59ee56cca05ee13bc6b24d131d990; the published head2202ab57d77cadc220a2bb3c26b0413f0c21f3e7carried the same five patches. Relative to that published topic, the forge's product changes are rename-only; the new adaptation is the fixture's README filename, plus new measured-run documentation. This is historical CI, not a new Forgejo Windows run.Testing
Tested: On this PC: Arch Linux, RTX 4090, NVIDIA 615.78.08, Core i9-13900K. Game-free checks covered the Walk behavior/wiring, option-ID collisions, changed input fixtures and the whole suite reported above; five input units also passed Windows-target syntax, not native Windows execution. The measured Longshot builds
d4a69456/3fc3c891used the same copied daytime Kokiri Forest save and identical local key probes: hold W/S with and without Caps Lock, switch to Toggle, tap to walk, tap again to run, and inspect Walk mode/speed and the Controls binding. Those game observations are carried to the current head, not a new game run or physical-keyboard test.Tested but not measured: the menu's appearance and binding-page navigation were inspected without a menu-specific overhead measurement. The shared unused-key FPS observations are inconclusive, not a performance verdict. The 15–90% range and 45% default keep their earlier Hookshot threshold evidence; the full range was not remeasured in Longshot.
Not tested: A physical keyboard or mouse on this PC, AMD/Intel gameplay, Caps Lock's OS state, first-person aiming while walking, slopes/water or places beyond Kokiri Forest; a controlled on/off overhead comparison. The runs used a key probe on the RTX 4090 in one area, not physical input or those other devices/scenes, and shared load prevents an overhead conclusion. No new test request has been opened for those platforms.
Needs other testers:
Screenshots
Historical pictures, inspected and kept separately rather than relabelled as Longshot:
Risk and rollback
Caps Lock still changes the OS's capitals state; bind another key if unwanted. First-person actions that read the movement stick could also become slower while Walk is active; camera-key and mouse tilt are not scaled, but first-person aiming was not tried. Old conflicting bindings must remain owned by their original controls, and future option additions must preserve the unique-ID guard.
Release Walk in Hold mode, toggle it off, or clear its binding to stop walking. Reverting the feature removes the new input path entirely; there is no new global off switch or saved-options migration to undo. The lack of a controlled unused-cost result is why this PR stays WIP.
Reviewer notes
Start with
walk_held()/move_keys_stick(), the old-binding conflict rule, and the host/renderer's 49/50 callbacks.tests/test_input_background.pyonly adds to main's checks; the four assertions Joey-Magnus requested remain intact.tests/test_option_ids.pyretains all five negative cases.Joey-Magnus approved
c3c7d19eafter the earlier review fixes, including then-free 52/53, namedkey_walkpersistence, no API-table bump and the safe conflicting-key upgrade. The later published rebase moved to main's reserved 49/50; the current cap owns 53, so the historical numbers must not be restored. That approval predates this forge head. Caps Lock versus another default key remains the maintainers' choice; no default was changed during this move.Issues
Original request: https://github.com/NotRexed/Hookshot/issues/2344 (closed). Original pull request: https://github.com/NotRexed/Hookshot/issues/2345.
No matching issue exists in the forge's all-issues list checked for this move. No old GitHub number is used as a forge closing reference.
Checklist
For everyone
type(scope): description).main.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. — Feature test: main lacks the helper/allocation; runtime comparison supplies the behavioral before result.tests/that reads changed source updated in this PR, not weakened.Moved from NotRexed/Hookshot#2345.
Assisted-by: Claude Opus 5.5
Co-Authored-By: Claude Opus 5.5 [email protected]
Assisted-by: GPT-6 Astra
Co-Authored-By: GPT-6 Astra [email protected]
Discussion carried over from NotRexed/Hookshot#2345
Every comment that was readable on the original thread when it was copied on 2026-11-12, in order, unchanged except that issue and pull request numbers are written as links to the old repository and names carry no @. Comments by an account that has since been closed there could no longer be read and are missing; where a reply quotes them, the quote remains.
mooki000, 2026-11-11 16:54 UTC (comment, original):
Rebased onto main
b744b1ec. It merged without a conflict, but its test would not have built there: theread_inputthattests/test_walk_key.pycuts out ofhost/nn_hid.cnow callshos_control_input()(NotRexed/Hookshot#926) and readsHOS_INPUT_EXCLUSIVEwithenv_on()(NotRexed/Hookshot#2042). The test now getsgpu/vk/compat/hookshot_env.hwith-includeand stubs the control bridge as off, astests/test_input_background.pydoes on main. The source lines of this pull request are unchanged.With the control socket: its input keeps its own stick tilt, like scripted input, and an exclusive socket input keeps the keyboard out while it lasts, Walk included.
Rerun on the new head (
8a14b300): the branch's three tests pass with gcc 16 and clang 23, andtest_walk_key.pystill fails against main's source.tests/run_all.py: 374 passed, 4 failed, 10 skipped. All four fail on main too on this machine; two of them are main'stest_vcore_fs_open.pyandtest_vcore_video_acquire.py, which NotRexed/Hookshot#2380 fixes, so expect those two red here until it is merged. The game runs in the description are from the old base (49f75dc8) and were not repeated.mooki000, 2026-11-11 17:56 UTC (comment, original):
Moved Walk mode/speed to 49/50 and updated both
vk_menu.hcomments in37687e2f. The host and renderer use the same constants; a compiled header check reportswalk_mode=49 walk_speed=50 camera_speed=48. Saved named keys are unchanged. NotRexed/Hookshot#2366 can keep 46; its remaining overlaps are text conflicts, not shared IDs.Also rebased onto
7759a6f3, including NotRexed/Hookshot#2380. The three input checks pass with GCC and Clang. The full suite is386 passed, 1 failed, 10 skipped; only the missing .NET 8 reference packs fail here. I removed the source-text wiring assertions rather than repinning the IDs; the compiled behavior check still rejects a variant with Walk tilt suppressed, with both compilers.No new game run after this rebase/renumber; first-person aiming remains explicitly untested.
mooki000, 2026-11-11 18:03 UTC (comment, original):
tests/test_walk_key.pypasses on Windows on37687e2f. Windows part 1/2 fails only ontests/test_native_astc_texture_count.py(the knownscanfCRT deprecation); NotRexed/Hookshot#2397 contains that fix. The description now records this Windows game-free coverage. Physical keyboard input and Windows gameplay remain untested.pmwork-personal, 2026-11-11 19:26 UTC (comment, original):
I think i prefer Alt as walk. its consistent with many other default control schemes in other implementations with walk keys.
mooki000, 2026-11-11 20:08 UTC (comment, original):
pmwork-personal thanks. Walk is rebindable on the Controls page. This CI refresh keeps Caps Lock; the default choice can be settled separately. The readme already calls out its effect on capitals.
Rebased onto
47ed9f14, now796b1bdf, including the merged NotRexed/Hookshot#2397 repair and NotRexed/Hookshot#2366 live prompts. The latter exposed a fixture bug: the test selected an earlierhos_binddeclaration instead of its definition. The range search is fixed, and all three input checks pass with GCC and Clang. Walk still uses 49/50. No new gameplay, physical-input or first-person-aiming proof is claimed.Update 2026-11-11: the refreshed source-regression workflow is green on
796b1bdf: both Windows and both Ubuntu shards pass. This is game-free CI evidence, not a new game run.Joey-Magnus, 2026-11-11 21:23 UTC (review: changes requested, original):
The code is sound (Caps Lock is scancode 57, a conflicting old ini leaves Walk unbound with a log line, and no API bump is needed). Two things before merge:
tests/test_input_background.pydrops four checks unrelated to this feature: the background block reads pads but not keyboard or mouse,t.optionmaps 4 topad_background, and the "Controller in background" row exists. All four still pass on this branch, so they can simply go back in. AGENTS.md asks for checks to be updated, not removed.HOS_OPT_WALK_MODE = 49matches NotRexed/Hookshot#2357'sHOS_OPT_FOV = 49. The two PRs touch different hunks, so git won't flag it. Yourif (id == HOS_OPT_WALK_MODE)comes first, so the game would read Walk mode (0/1) as the FOV percentage, whichoot_fov.cthen rejects. Whichever lands second has to renumber; please coordinate with polaco1782.There's also an open question from a commenter about Alt instead of Caps Lock as the default.
mooki000, 2026-11-11 23:45 UTC (comment, original):
I fixed both points in
5ab213a6:vk_api.h. Neither ID was claimed; the refreshed table now puts FOV 51 on main and still finds 52/53 free. NotRexed/Hookshot#2530 claims 49 for Keyboard. The real header is compiled with the host's reads, and the renderer, menu comments and wiring checks agree.The three input checks and the 43-ID guard pass with GCC and Clang; suppressed Walk tilt fails at runtime, and all 13 socket-fixture states pass. The updated description keeps both unequal raw suite results with their loads; the three differing depth/recorder/appimage checks all pass when run serially.
I kept Caps Lock for now: Walk is rebindable on the Controls page, and I can change the default if the maintainers prefer another key.
I rebased onto
8fcc8c22after the FOV merge, keeping both formatters, main's two-slot pad bindings and its documentation additions. No new game run, physical-input or first-person-aiming proof is claimed.mooki000, 2026-11-12 06:07 UTC (comment, original):
Rebased the pushed
5ab213a6onto current main87991bc8asc3c7d19e, keeping both commits and their complete authors/messages/trailers. The resolutions retain all five ocarina-note controls, keyboard prompts, camera handover, upscaler reset and current translated hints beside Walk's unchanged Hold/Caps Lock/45% defaults, 15–90% range and IDs 52/53. Fixtures retain all assertions; the new note-layer integration also asserts that an inactive note layer consumes no controller input. GCC passed all 122 selected checks; Clang passed 121 with main's inherited background-input warning. The suite was 469 passed / 2 inherited failures / 20 skipped: that warning and the known release-script race. The last rebase adds only NotRexed/Hookshot#2671 and changes no PR file blob; Walk/ocarina/wiring checks, final-main negatives, NotRexed/Hookshot#2671's offline check and Windows-target syntax ran again. The final PR gate passes. No second full suite, game run or native Windows execution is claimed. Sources: the NotRexed/Hookshot#2345 evidence and final-handoff ledger above.Joey-Magnus, 2026-11-12 14:32 UTC (review: approved, original):
Both of my points are fixed at c3c7d19e.
tests/test_input_background.pynow only adds to main's checks, with the four I asked about back unchanged, and Walk uses option ids 52 and 53 (gpu/vk/vk_api.h:34), which nothing on main uses (main has 45 to 48, and NotRexed/Hookshot#2357's FOV took 51). The bind table grows by one entry at its end, saved by name askey_walk,options_versionis untouched and no API bump is needed.I merged it into current main and ran
test_walk_key.py,test_bound_keys.py,test_input_background.py,test_ocarina_note_bindings.py,test_ocarina_notes.py,test_option_ids.py,test_menu_i18n.pyandtest_env_on.py: all pass, and CI is green on this head. Leaving Walk unbound with a log line when an older ini already gives Caps Lock to another control (binds_load) is the right upgrade path.Caps Lock versus Alt as the default is the maintainers' call, and it's rebindable either way. First-person aiming while walking is still untested, as you say.
mooki000, 2026-11-12 17:04 UTC (comment, original):
Both are taken in the default layout: Shift is B and Ctrl is the left-stick click (
key_defaultinhost/nn_hid.c), so Walk on either means moving one of those to another key. Caps Lock was the free key beside them, which is the only reason it was picked. The row is rebound on the Controls page like any other, so a player who wants Walk on Shift or Ctrl can swap them today.Caps Lock has a cost, to be fair: every press also flips the system's Caps Lock state, and I have not tried it on a physical keyboard (the runs were headless, as the description says).
If you would rather have Walk on Ctrl by default, with the stick click moved to another key or left without one, say which and I will change the default table, the README row and the test. One thing to decide with it: a player whose saved bindings already name Ctrl for the stick click would start with Walk unbound (the existing rule for a default key that is taken), not with the two swapped.
mooki000, 2026-11-12 18:48 UTC (comment, original):
Rebased onto main
3c2d80cf3; new head2202ab57d.Walk mode and speed now use 49 and 50, the numbers main reserves for Walk, because 52 went to Skip opening. Saved option names are unchanged.
No other gameplay change: Caps Lock, Hold, 45% and the 15–90% range remain. The default key still awaits the maintainer's preference.
Four input fixtures now declare CRT usage before headers; every assertion is retained. The fork's Windows run uses
45f3e06c2; all five patches are identical at this final head.Focused checks: 24 passed with GCC 16 and 24 with Clang 23. A full run of the project's checks in my fork, Windows included, is green on
45f3e06c: the same five commits one main commit earlier (git range-diffmarks each=), so it was not repeated for this head.The earlier full suite was 497 passed, 3 failed, 20 skipped: two failures on main too, now fixed there; the compiler SIGKILL passed alone with fewer workers.
First windowless Kokiri run: three inspected menu pictures, Toggle walking/running 1.467 / 4.500 m/s, and Hold running 4.500 m/s.
Hold walking was not measured in that run: first-load shader compilation swallowed the short holds.
One ordinary completion run, same binary: Hold walking/running 1.469 / 4.500 m/s, nine readings each after the first.
Both walking results round to the description's 1.47 m/s, running to 4.50 m/s. Physical input and Windows gameplay remain untested.
Runs were built at
7e536c33, before the final refresh: the Walk code is identical, but the intervening main renderer changes were not rerun.Joey-Magnus's earlier approval was for
c3c7d19eand predates this refresh.Added the three-part Testing section and updated the full checklist. Added one branch base/head/suite block under Measurements; apart from consolidating duplicate results and minor wording cleanup, nothing else changed.
View command line instructions
Manual merge helper
Use this merge commit message when completing the merge manually.
Checkout
From your project repository, check out a new branch and test the changes.