MagicBeanDev/Hookshot

Archived

Watch

Fork 0

feat(input): a Keyboard row: Auto, On or Off #2530

Open

omgitsgela wants to merge 2 commits from omgitsgela/pr-keyboard-input into main

omgitsgela commented

2026-11-11 22:20:24 +00:00

(Migrated from github.com)

The current PR head (4c276b88) gates gameplay keyboard bindings only. Hardware testing found that the menu's independent keyboard path still accepts Steam-injected Escape, so this is not yet a complete fix for direct Desktop launches. A local follow-up candidate is built, but its on-device validation is pending. That follow-up is not in this PR's current code.

Why

On a Steam Deck LCD in Desktop Mode, with Hookshot started outside Steam, the left stick moved Link and tilted the camera, and face/menu buttons triggered unintended actions. The initial report followed a camera-inversion change, but the saved and live keyboard/controller bindings matched: it was not a corrupted binding file.

Steam's Desktop layout emits keyboard input in addition to controller input. Hookshot's newer keyboard defaults bind arrows to camera movement and Tab/Enter to Plus. Our earlier XQueryKeymap recording measured Up arrow (keycode 111) held for 2.81 seconds while the left stick was held forward. Remapping the local camera keys to IJKL and Plus to P removed those gameplay collisions, but changing global keyboard defaults would penalize keyboard players.

The newer direct-launch capture found the same physical B press in both paths: native controller bit 1 (east face button, bound to Switch A/confirm) and an Escape key event. The native binding was correct; the independent menu keyboard reader accepted Escape and opened Hookshot's menu.

What the current PR adds

A Keyboard row on Controls, saved as keyboard=:

Auto (default): gameplay keyboard bindings count only while no controller is connected.
On: gameplay keyboard bindings always count, as before.
Off: gameplay keyboard bindings do not count.

HOS_OPT_KEYBOARD is 54 in the current PR code. It was moved from 49 to avoid the Walk mode/speed reservation; this description previously retained the stale value.

pads_read() records whether SDL/XInput controllers are connected; key() applies the selected mode. Connection state, not whether a button happens to be held, decides Auto. Mouse buttons retain their separate behavior.

Current limitation: F1, Escape, arrows, Enter and Tab in Hookshot's menu are still read on a separate path outside this policy. Leaving that path unaffected was deliberate in the first version, but it permits the captured Escape collision. The help text and implementation need to be updated together before this is ready.

Hardware findings and remaining uncertainty

The earlier SDL2 override did not cure the original camera/Plus collision. That observation does not rule out every SDL2/SDL3 controller difference.
In a later direct SDL2 game capture, the reported View presses did not appear in Hookshot's collected controller state. The saved View-to-Minus binding was correct.
A standalone SDL3 probe captured two presses each of View, physical B, Menu and D-pad Down. Hardware reports and SDL3's mapped inputs agreed for all eight presses; View mapped to Back, Menu to Start.
A Steam-launched SDL3 game run had correct controls, reported by the tester. This changes both launch path and backend, so it does not prove SDL3 alone fixes direct launches.
A direct SDL3 game run still opened Hookshot's menu on physical B, blocking save selection. Thus the menu-input collision persists with SDL3. Direct-launch View behavior in gameplay remains unverified.
The older notes labelled runtime Tab/Escape presses differently from the saved Desktop-layout table. Those labels should not be used as a universal physical-button mapping. The newer physical-B/native-bit/Escape capture above is the concrete reproduction for the menu fix.

Follow-up candidate, not yet pushed

The local candidate shares the host's keyboard policy with menu input, preserves intentional F1/enabled-Insert access, drains suppressed queued keys, and gates keyboard direction repeats without suppressing native controller navigation. It adds a host callback and a matched host/renderer ABI change.

A regression extracting the actual menu polling code and actual host policy fails before the fix: Auto with a connected controller accepts Escape. It passes with the local change. The focused Linux run over input, keyboard, menu, bound_keys, pad, option and hid reports 26 passed, 0 failed, 0 skipped for that local candidate. These are not CI results for the published PR head and are not hardware acceptance.

A matched executable and Vulkan renderer built successfully on the Deck. First-launch preparation is currently blocking hardware validation: the builder hashes vk_api.h into the shader translator stamp, so even this input-only ABI change invalidates translated-program caches. No cache checks were bypassed.

Tests already in this PR

tests/test_keyboard_option.py: extracts the gameplay key()/keyboard_counts() functions and checks 10 Auto/On/Off, controller, menu, key and mouse cases.
tests/test_input_background.py: updated fixture for the keyboard policy and controller-count state.
The original focused Linux run reported 23 passing checks. It did not cover the menu's separate keyboard reader.

Acceptance still required

Before marking ready, verify the amended candidate on the Deck:

Direct Desktop launch with Steam running: physical B confirms without opening Hookshot's menu; View opens the map; Menu opens pause separately; movement does not steer the camera.
Hookshot's menu: native controller navigation works; F1 and enabled Insert remain available; suppressed key events do not fire later after a mode change or disconnect.
Auto/On/Off behave as documented, including keyboard-only use and controller disconnection.
Repeat controller checks through Steam to rule out a regression in the known-working launch path.

No universal Deck mapping or SDL2-only/SDL3-only requirement is established by the current evidence. Windows and other controllers are not hardware-tested by these Deck runs.

## Status: draft, menu-input coverage incomplete The current PR head (`4c276b88`) gates gameplay keyboard bindings only. Hardware testing found that the menu's independent keyboard path still accepts Steam-injected Escape, so this is not yet a complete fix for direct Desktop launches. A local follow-up candidate is built, but its on-device validation is pending. That follow-up is **not in this PR's current code**. ## Why On a Steam Deck LCD in Desktop Mode, with Hookshot started outside Steam, the left stick moved Link and tilted the camera, and face/menu buttons triggered unintended actions. The initial report followed a camera-inversion change, but the saved and live keyboard/controller bindings matched: it was not a corrupted binding file. Steam's Desktop layout emits keyboard input in addition to controller input. Hookshot's newer keyboard defaults bind arrows to camera movement and Tab/Enter to Plus. Our earlier XQueryKeymap recording measured Up arrow (keycode 111) held for 2.81 seconds while the left stick was held forward. Remapping the local camera keys to IJKL and Plus to P removed those gameplay collisions, but changing global keyboard defaults would penalize keyboard players. The newer direct-launch capture found the same physical B press in both paths: native controller bit 1 (east face button, bound to Switch A/confirm) and an Escape key event. The native binding was correct; the independent menu keyboard reader accepted Escape and opened Hookshot's menu. ## What the current PR adds A **Keyboard** row on Controls, saved as `keyboard=`: - **Auto** (default): gameplay keyboard bindings count only while no controller is connected. - **On**: gameplay keyboard bindings always count, as before. - **Off**: gameplay keyboard bindings do not count. `HOS_OPT_KEYBOARD` is **54** in the current PR code. It was moved from 49 to avoid the Walk mode/speed reservation; this description previously retained the stale value. `pads_read()` records whether SDL/XInput controllers are connected; `key()` applies the selected mode. Connection state, not whether a button happens to be held, decides Auto. Mouse buttons retain their separate behavior. **Current limitation:** F1, Escape, arrows, Enter and Tab in Hookshot's menu are still read on a separate path outside this policy. Leaving that path unaffected was deliberate in the first version, but it permits the captured Escape collision. The help text and implementation need to be updated together before this is ready. ## Hardware findings and remaining uncertainty - The earlier SDL2 override did not cure the original camera/Plus collision. That observation does **not** rule out every SDL2/SDL3 controller difference. - In a later direct SDL2 game capture, the reported View presses did not appear in Hookshot's collected controller state. The saved View-to-Minus binding was correct. - A standalone SDL3 probe captured two presses each of View, physical B, Menu and D-pad Down. Hardware reports and SDL3's mapped inputs agreed for all eight presses; View mapped to Back, Menu to Start. - A Steam-launched SDL3 game run had correct controls, reported by the tester. This changes both launch path and backend, so it does not prove SDL3 alone fixes direct launches. - A direct SDL3 game run still opened Hookshot's menu on physical B, blocking save selection. Thus the menu-input collision persists with SDL3. Direct-launch View behavior in gameplay remains unverified. - The older notes labelled runtime Tab/Escape presses differently from the saved Desktop-layout table. Those labels should not be used as a universal physical-button mapping. The newer physical-B/native-bit/Escape capture above is the concrete reproduction for the menu fix. ## Follow-up candidate, not yet pushed The local candidate shares the host's keyboard policy with menu input, preserves intentional F1/enabled-Insert access, drains suppressed queued keys, and gates keyboard direction repeats without suppressing native controller navigation. It adds a host callback and a matched host/renderer ABI change. A regression extracting the actual menu polling code and actual host policy fails before the fix: Auto with a connected controller accepts Escape. It passes with the local change. The focused Linux run over `input`, `keyboard`, `menu`, `bound_keys`, `pad`, `option` and `hid` reports **26 passed, 0 failed, 0 skipped** for that local candidate. These are not CI results for the published PR head and are not hardware acceptance. A matched executable and Vulkan renderer built successfully on the Deck. First-launch preparation is currently blocking hardware validation: the builder hashes `vk_api.h` into the shader translator stamp, so even this input-only ABI change invalidates translated-program caches. No cache checks were bypassed. ## Tests already in this PR - `tests/test_keyboard_option.py`: extracts the gameplay `key()`/`keyboard_counts()` functions and checks 10 Auto/On/Off, controller, menu, key and mouse cases. - `tests/test_input_background.py`: updated fixture for the keyboard policy and controller-count state. - The original focused Linux run reported 23 passing checks. It did not cover the menu's separate keyboard reader. ## Acceptance still required Before marking ready, verify the amended candidate on the Deck: 1. Direct Desktop launch with Steam running: physical B confirms without opening Hookshot's menu; View opens the map; Menu opens pause separately; movement does not steer the camera. 2. Hookshot's menu: native controller navigation works; F1 and enabled Insert remain available; suppressed key events do not fire later after a mode change or disconnect. 3. Auto/On/Off behave as documented, including keyboard-only use and controller disconnection. 4. Repeat controller checks through Steam to rule out a regression in the known-working launch path. No universal Deck mapping or SDL2-only/SDL3-only requirement is established by the current evidence. Windows and other controllers are not hardware-tested by these Deck runs.

hckrman101 (Migrated from github.com) reviewed

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

hckrman101 (Migrated from github.com) left a comment

Thanks for the measurements, the diagnosis is convincing and the fix is small and well tested at the unit level. Two things I'd want settled before this goes in, plus a note.

Option id 49 collides with #2345 (HOS_OPT_WALK_MODE = 49, HOS_OPT_WALK_SPEED). Main also has 48 (camera speed) now, and #2357 takes 51, #2261 takes 60. Whichever of these lands second has to renumber, and a saved id that means two things in two builds is the painful kind of bug. Worth agreeing on an id with #2345's author (50-something, or 52+).
Auto changes the default for everyone with a pad plugged in, not just Steam Input users. keyboard_counts() returns false for every key as soon as pads_read() sees one pad, even an idle one. A Windows player on keyboard and mouse with an XInput pad connected (or a Steam virtual pad, a DualSense on USB charging) loses WASD, the camera keys, Enter and so on after updating, and the only hint is a row in the Insert menu. The Deck case you measured is real, but I don't think it should cost these players their keyboard silently. Options: keep the default On and let the Deck case default to Off/Auto (e.g. only when SteamDeck/Steam Input is detected), or make Auto mean "a pad was used more recently than the keyboard's non-injected input" if that's detectable. At the least the PR description should say plainly that Auto turns the keyboard off with any pad connected. I'd leave which way to go to @NotRexed.

Nit: key() also feeds the mouse-sensitivity hotkeys (-/+) at the top of the mouse code, so those go quiet under Auto with a pad connected too. Probably fine, just worth knowing.

What I checked and found fine: the mouse-button range test (0x01..0x06 minus VK_CANCEL) matches the five mouse_vk_slot buttons; controlled counts SDL pads plus connected XInput slots; the Insert menu keys use their own path (vk_menu.cpp key table) so Off cannot lock you out of it; MkwVkHost is unchanged so no MKWVK_API_VERSION bump is needed; fill_replay_table returns opts.keyboard; test is a new test_*.py, so run_all discovers it. CI: only the title check had reported when I looked (pending), so the 23 passing checks in the description are the author's local run, not confirmed by CI yet. No discussion on the PR so far, and I didn't find any other review.

Thanks for the measurements, the diagnosis is convincing and the fix is small and well tested at the unit level. Two things I'd want settled before this goes in, plus a note. 1. **Option id 49 collides with #2345** (`HOS_OPT_WALK_MODE = 49, HOS_OPT_WALK_SPEED`). Main also has 48 (camera speed) now, and #2357 takes 51, #2261 takes 60. Whichever of these lands second has to renumber, and a saved id that means two things in two builds is the painful kind of bug. Worth agreeing on an id with #2345's author (50-something, or 52+). 2. **Auto changes the default for everyone with a pad plugged in, not just Steam Input users.** `keyboard_counts()` returns false for every key as soon as `pads_read()` sees one pad, even an idle one. A Windows player on keyboard and mouse with an XInput pad connected (or a Steam virtual pad, a DualSense on USB charging) loses WASD, the camera keys, Enter and so on after updating, and the only hint is a row in the Insert menu. The Deck case you measured is real, but I don't think it should cost these players their keyboard silently. Options: keep the default On and let the Deck case default to Off/Auto (e.g. only when SteamDeck/Steam Input is detected), or make Auto mean "a pad was used more recently than the keyboard's non-injected input" if that's detectable. At the least the PR description should say plainly that Auto turns the keyboard off with any pad connected. I'd leave which way to go to @NotRexed. Nit: `key()` also feeds the mouse-sensitivity hotkeys (`-`/`+`) at the top of the mouse code, so those go quiet under Auto with a pad connected too. Probably fine, just worth knowing. What I checked and found fine: the mouse-button range test (`0x01..0x06` minus `VK_CANCEL`) matches the five `mouse_vk_slot` buttons; `controlled` counts SDL pads plus connected XInput slots; the Insert menu keys use their own path (`vk_menu.cpp` key table) so Off cannot lock you out of it; `MkwVkHost` is unchanged so no `MKWVK_API_VERSION` bump is needed; `fill_replay_table` returns `opts.keyboard`; test is a new `test_*.py`, so run_all discovers it. CI: only the title check had reported when I looked (pending), so the 23 passing checks in the description are the author's local run, not confirmed by CI yet. No discussion on the PR so far, and I didn't find any other review.

gpu/vk/vk_api.h

Collides with #2345 (HOS_OPT_WALK_MODE = 49, HOS_OPT_WALK_SPEED); see the review body.

Collides with #2345 (`HOS_OPT_WALK_MODE = 49, HOS_OPT_WALK_SPEED`); see the review body.

hckrman101 commented

2026-11-11 22:55:56 +00:00

(Migrated from github.com)

This now conflicts with main in gpu/vk/vk_api.h. Could you merge main into the branch (or rebase) when you get a chance? I'll take another look once it's up to date.

This now conflicts with main in `gpu/vk/vk_api.h`. Could you merge main into the branch (or rebase) when you get a chance? I'll take another look once it's up to date.

mooki000 commented

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

(Migrated from github.com)

For the id collision noted above: Walk in #2345 moved to 52/53 (5ab213a6), so its old 49 no longer stands in this pull request's way, whichever lands first.

For the id collision noted above: Walk in #2345 moved to 52/53 (`5ab213a6`), so its old 49 no longer stands in this pull request's way, whichever lands first.

hckrman101 (Migrated from github.com) reviewed

2026-11-11 23:59:01 +00:00

hckrman101 (Migrated from github.com) left a comment

Heads up: I merged main into this branch (merge commit 4c276b88) to clear the gpu/vk/vk_api.h conflict, so please pull before pushing. The conflict was main's new HOS_OPT_FOV = 51 next to HOS_OPT_KEYBOARD = 49. I moved the Keyboard option to id 54 (49 and 50 are reserved for #2345's Walk options, #2345 and #2567 are taking 52/53, and 60 is #2261) and updated the comment in gpu/vk/vk_menu.h. The keyboard, option-id, nn_hid and vk_api tests pass locally.

That settles the id collision from my earlier review. Two points are still open, and nothing else has changed since:

Default Auto. pad_count_now counts every connected pad, idle or not, so as soon as one is plugged in (or Steam presents a virtual pad) all the keyboard's bindings stop, WASD included. A keyboard and mouse player with a pad plugged in would lose their controls with no hint why. A Deck/Steam-specific default, or an Auto that only drops the keyboard while the pad is actually being used, would avoid that. This is NotRexed's call.
Nit: the mouse-sensitivity -/+ hotkeys also go through key(), so Auto and Off gate them too.

The decision stays with NotRexed. CI is still pending.

Heads up: I merged main into this branch (merge commit `4c276b88`) to clear the `gpu/vk/vk_api.h` conflict, so please pull before pushing. The conflict was main's new `HOS_OPT_FOV = 51` next to `HOS_OPT_KEYBOARD = 49`. I moved the Keyboard option to id **54** (49 and 50 are reserved for #2345's Walk options, #2345 and #2567 are taking 52/53, and 60 is #2261) and updated the comment in `gpu/vk/vk_menu.h`. The keyboard, option-id, nn_hid and vk_api tests pass locally. That settles the id collision from my earlier review. Two points are still open, and nothing else has changed since: - **Default Auto.** `pad_count_now` counts every connected pad, idle or not, so as soon as one is plugged in (or Steam presents a virtual pad) all the keyboard's bindings stop, WASD included. A keyboard and mouse player with a pad plugged in would lose their controls with no hint why. A Deck/Steam-specific default, or an Auto that only drops the keyboard while the pad is actually being used, would avoid that. This is NotRexed's call. - **Nit:** the mouse-sensitivity -/+ hotkeys also go through `key()`, so Auto and Off gate them too. The decision stays with NotRexed. CI is still pending.

hckrman101 commented

2026-11-12 03:41:46 +00:00

(Migrated from github.com)

This conflicts with main again, in host/nn_hid.c (main has changed that file since our merge; the rest merges cleanly). Could you merge main into the branch, pulling our merge commit first so the history stays simple?

A correction to my last review: I wrote that the default for Auto was someone else's call. That was wrong. Whether Auto should be the default, given it drops every keyboard binding once any pad is connected (idle or not), is an open maintainer question, and we will follow up on it here. I am not deciding it in this comment.

This conflicts with main again, in `host/nn_hid.c` (main has changed that file since our merge; the rest merges cleanly). Could you merge main into the branch, pulling our merge commit first so the history stays simple? A correction to my last review: I wrote that the default for Auto was someone else's call. That was wrong. Whether Auto should be the default, given it drops every keyboard binding once any pad is connected (idle or not), is an open maintainer question, and we will follow up on it here. I am not deciding it in this comment.