Repository navigation
feat(input): a Keyboard row: Auto, On or Off - #2530
omgitsgela wants to merge 2 commits into
Conversation
Steam Input's Desktop layout sends a pad's stick and buttons to the focused window as key presses: the left stick the arrow keys, A Enter, B Esc, View Tab, Menu Esc. Hookshot reads the keyboard's bindings with the game's, so on a Steam Deck in Desktop Mode, with the game started outside Steam, walking moved the camera (the arrows are the camera's) and View and Menu both pressed Plus (Tab and Enter are Plus's). Auto counts the keyboard's bindings only while no controller is connected; On and Off force them. A mouse button is the mouse's and is not affected, and the Insert menu's own keys never come through this path.
hckrman101
left a comment
There was a problem hiding this 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 aspads_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.
| /* (HOS_OPT_KEYBOARD, 49: the Controls page's Keyboard row, host/nn_hid.c: 0 Auto, 1 On, 2 Off. Auto counts the keyboard's | ||
| bindings only while no controller is connected: Steam Input's Desktop layout sends a pad's stick and buttons to a focused | ||
| window as key presses, which reached the game's camera and Plus on a Steam Deck in Desktop Mode) */ | ||
| enum { HOS_OPT_KEYBOARD = 49 }; |
There was a problem hiding this comment.
Collides with #2345 (HOS_OPT_WALK_MODE = 49, HOS_OPT_WALK_SPEED); see the review body.
|
This now conflicts with main in |
|
For the id collision noted above: Walk in #2345 moved to 52/53 ( |
vk_api.h: main added HOS_OPT_FOV (51) next to the 49/50 reserved for NotRexed#2345. The Keyboard row moves to 54 (49 is NotRexed#2345's; NotRexed#2345 and NotRexed#2567 now use 52/53).
hckrman101
left a comment
There was a problem hiding this 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_nowcounts 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.
|
This conflicts with main again, in 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. |
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=:HOS_OPT_KEYBOARDis 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
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,optionandhidreports 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.hinto 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 gameplaykey()/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.Acceptance still required
Before marking ready, verify the amended candidate on the Deck:
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.