From 943bf4324e29339be532578797e8fd0163f1c1b3 Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Sat, 1 Aug 2026 21:23:02 -0400 Subject: [PATCH] Let a control map listen to any keyboard, and fix the dead arrow keys The sidescroller's controls did nothing. `akgl_controller_handle_event` matched `event->key.which == curmap->kbid` exactly, with no way to say "whatever keyboard the player is typing on", so the game did the obvious thing and bound `SDL_GetKeyboards()[0]`. That cannot work. The id a key event *carries* is chosen by the video backend and is not required to be an id `SDL_GetKeyboards()` reports. On X11 without XInput2 every key event carries `SDL_GLOBAL_KEYBOARD_ID`, which is 0, while `SDL_AddKeyboard` registers the attached keyboard as `SDL_DEFAULT_KEYBOARD_ID`, which is 1; with XInput2 the events carry the physical slave device's `sourceid` while `SDL_GetKeyboards()[0]` may be the master. Either way the map matched nothing and every key press was dropped. A `kbid` or `jsid` of 0 now matches any device of that kind. 0 is safe to spend: SDL documents `which` as 0 when the source is unknown or virtual, and joystick ids start at 1, so no real device is 0. A non-zero id still matches only that device, which is what keeps two local players on two keyboards apart, and there is a test for that half too. `util/charviewer.c` already passed 0 and depended on the old behaviour by accident; it works under XInput2 now as well. Two reasons this shipped, both closed: - Every test in tests/controller.c dispatched an event whose id equalled the id the map was bound with, so none of them could see it. - The sidescroller's smoke test called the control handlers directly instead of dispatching events, so it passed against a control map that matched nothing. It now pushes synthetic events through akgl_controller_handle_event from a deliberately non-zero device id and fails if the press does not arrive. Verified by breaking the binding and watching the run exit non-zero. `test_controller_wildcard_device_ids` was written first and failed against the unfixed library with "a map bound to keyboard 0 ignored a key press from device 11", which is the whole reason to trust it now that it passes. The controls were documented, in one sentence. Chapter 19 has a proper table of them, and chapter 15 explains why not to reach for SDL_GetKeyboards(). The header said the match was exact and now says what it does. Co-Authored-By: Claude Code Co-Authored-By: Claude Opus 5 (1M context) --- TODO.md | 25 ++++++++++ docs/15-input.md | 25 +++++++++- docs/19-tutorial-sidescroller.md | 53 +++++++++++++++------ examples/sidescroller/player.c | 70 ++++++++++++++++------------ examples/sidescroller/sidescroller.h | 8 ++++ include/akgl/controller.h | 14 ++++-- src/controller.c | 22 ++++++++- tests/controller.c | 69 +++++++++++++++++++++++++++ 8 files changed, 233 insertions(+), 53 deletions(-) diff --git a/TODO.md b/TODO.md index 815ac09..63a016c 100644 --- a/TODO.md +++ b/TODO.md @@ -2306,6 +2306,31 @@ The manual documents each of these where a reader would hit it, and points here. (`ACTOR_STATE_ALIVE`) and `velocity_x`/`velocity_y` instead of `speed_x`/`speed_y`. The current loader accepts neither. +19. **A control map could not say "any keyboard", so binding one was a coin flip.** + `akgl_controller_handle_event` matched `event->key.which == curmap->kbid` exactly, and + there was no way to express "whatever keyboard the player types on". That is not a + theoretical gap, because the id a key event *carries* is chosen by the video backend + and does not have to be an id `SDL_GetKeyboards()` reports: on X11 without XInput2 + every key event carries `SDL_GLOBAL_KEYBOARD_ID` (0) while SDL registers the keyboard + as `SDL_DEFAULT_KEYBOARD_ID` (1), and with XInput2 the events carry the physical slave + device's `sourceid` while `SDL_GetKeyboards()[0]` may be the master. A game that bound + `SDL_GetKeyboards()[0]` -- the obvious thing -- got a map that matched nothing and + controls that silently did nothing. `examples/sidescroller` shipped exactly that. + + **Fixed**: `kbid`/`jsid` of 0 now match any device of that kind. 0 is safe to spend + because SDL documents `which` as 0 when the source is unknown or virtual and joystick + ids start at 1, so no real device is 0. A non-zero id still matches only that device, + which is what keeps two local players apart. `util/charviewer.c` was already passing 0 + and depending on the old accidental behaviour, so it works under XInput2 now too. + + The reason this reached a release is worth keeping: **every test in + `tests/controller.c` dispatched an event whose id equalled the id the map was bound + with**, so none of them could see it, and the sidescroller's smoke test called the + handlers directly rather than dispatching events, so it passed against a control map + that matched nothing. Both are fixed -- `test_controller_wildcard_device_ids` asserts + the contract, and the smoke test now drives `akgl_controller_handle_event` from a + non-zero device id and fails if the press does not arrive. + ### Header comments that describe code that has changed Twenty-seven claims across the public headers were false when checked against `src/`. diff --git a/docs/15-input.md b/docs/15-input.md index f500dc4..b16fd07 100644 --- a/docs/15-input.md +++ b/docs/15-input.md @@ -122,8 +122,29 @@ defaults are already in slots 0–7 and your additions land after them. A match requires **all three** of: the event type equals the binding's `event_on` or `event_off`; the device id on the event equals the map's `kbid` (keyboard events) or `jsid` (gamepad events); and the key or button equals the binding's. The device-id half is what -keeps two players on two keyboards apart, and it is exact — `akgl_controller_list_keyboards` -logs every attached keyboard and its id, which is how you find the number to pass. +keeps two players on two keyboards apart — `akgl_controller_list_keyboards` logs every +attached keyboard and its id, which is how you find the number to pass. + +### Bind device 0 unless you mean a specific device + +**A `kbid` or `jsid` of `0` matches every device of that kind**, and it is what a +single-player game wants. + +Do not reach for `SDL_GetKeyboards()` to fill it in. That call reports what is *attached*; +the id a key event actually *carries* is chosen by the video backend, and the two need not +agree. On X11 without XInput2 every key event carries `SDL_GLOBAL_KEYBOARD_ID`, which is +`0`, while SDL registers the attached keyboard as `SDL_DEFAULT_KEYBOARD_ID`, which is `1`. +With XInput2 the events carry the physical slave device's `sourceid` and +`SDL_GetKeyboards()[0]` may be the master. Binding `SDL_GetKeyboards()[0]` therefore gives +you a map that matches nothing at all, and controls that silently do nothing. + +That is not hypothetical — the sidescroller in [Chapter 19](19-tutorial-sidescroller.md) +shipped exactly that bug, and its smoke test passed anyway because the test called the +handlers instead of dispatching events. + +`0` is the right spelling for "any" because SDL documents `which` as `0` when the source is +unknown or virtual, and joystick ids start at 1 — so no real device is `0` and nothing is +given up by spending it. There is no way to remove a binding. `akgl_controller_pushmap` appends and that is the whole API; the only reset is zeroing `akgl_controlmaps[id]` yourself. **Calling diff --git a/docs/19-tutorial-sidescroller.md b/docs/19-tutorial-sidescroller.md index b760f6a..638a012 100644 --- a/docs/19-tutorial-sidescroller.md +++ b/docs/19-tutorial-sidescroller.md @@ -23,10 +23,20 @@ cmake --build build --parallel --target sidescroller ./build/examples/sidescroller/sidescroller ``` -Arrow keys to run, space to jump, and holding space longer jumps higher. Three flags exist -for the smoke test and are useful by hand too: `--assets DIR` points somewhere other than -the compiled-in asset directory, `--frames N` (or `AKGL_SIDESCROLLER_FRAMES`) exits after -that many frames, and `--autoplay` drives the player from a script instead of the keyboard. +### Controls + +| Key | Gamepad | Does | +|---|---|---| +| Left arrow | D-pad left | Run left | +| Right arrow | D-pad right | Run right | +| Space | South button (A) | Jump — hold it longer to jump higher | + +Close the window to quit; there is no key bound to it. + +Three flags exist for the smoke test and are useful by hand too: `--assets DIR` points +somewhere other than the compiled-in asset directory, `--frames N` (or +`AKGL_SIDESCROLLER_FRAMES`) exits after that many frames, and `--autoplay` drives the +player from a script instead of the keyboard. ## The level @@ -745,22 +755,37 @@ set_tests_properties(example_sidescroller PROPERTIES ``` Four seconds of scripted play under the headless drivers. `--autoplay` holds *right* and -requests a jump every forty-five frames by calling the handlers a keyboard would have -called: +taps jump every forty-five frames, and it does it by pushing synthetic events through +`akgl_controller_handle_event` rather than by calling the handlers: ```c excerpt=examples/sidescroller/player.c if ( frame == 1 ) { - PASS(errctx, akgl_actor_cmhf_right_on(ss_game.player, &synthetic)); - } - if ( (frame % SS_AUTOPLAY_JUMP_PERIOD) == 0 ) { - ss_game.jump_requested = true; + synthetic.type = SDL_EVENT_KEY_DOWN; + synthetic.key.which = SS_AUTOPLAY_KBID; + synthetic.key.key = SDLK_RIGHT; + PASS(errctx, akgl_controller_handle_event((void *)&akgl_game.state, &synthetic)); + FAIL_ZERO_RETURN( + errctx, + AKGL_BITMASK_HAS(ss_game.player->state, AKGL_ACTOR_STATE_MOVING_RIGHT), + AKGL_ERR_BEHAVIOR, + "the right arrow did not reach the player; the control map matched nothing" + ); } ``` -That works because a control-map handler takes the actor and an event, and the keyboard -handlers do not read the event beyond requiring one to be there. In four seconds the script -walks the level, jumps, collides with terrain, collects a coin, falls in the pit and -respawns — so the smoke test exercises the collision and the pickup, not just the startup. +**That distinction is the whole value of the smoke test, and the first version got it +wrong.** Calling the handlers directly is easier and it works — but it skips the binding +table, which is the part with something to get wrong. This game shipped with a control map +that matched no keyboard on earth, and a smoke test that called the handlers passed anyway. +A test that cannot fail is not a test, so the script drives the real dispatch and then +asserts the press actually arrived. Break the binding and the run exits non-zero naming the +reason. + +`SS_AUTOPLAY_KBID` is deliberately not `0`: the map binds keyboard `0` meaning *any* +keyboard, and driving it from a non-zero device id is what proves that. In four seconds the +script walks the level, jumps, collides with terrain, collects a coin, falls in the pit and +respawns — so the smoke test exercises the input path, the collision and the pickup, not +just the startup. A run ends with a line naming what happened, which is worth reading when a change moves the feel: diff --git a/examples/sidescroller/player.c b/examples/sidescroller/player.c index b462619..86f5e8a 100644 --- a/examples/sidescroller/player.c +++ b/examples/sidescroller/player.c @@ -315,9 +315,6 @@ akerr_ErrorContext *ss_player_controls(int controlmapid, char *actorname) { akgl_ControlMap *controlmap = NULL; akgl_Control control; - SDL_KeyboardID *keyboards = NULL; - SDL_JoystickID *gamepads = NULL; - int count = 0; PREPARE_ERROR(errctx); FAIL_ZERO_RETURN(errctx, actorname, AKERR_NULLPOINTER, "actorname"); @@ -345,26 +342,19 @@ akerr_ErrorContext *ss_player_controls(int controlmapid, char *actorname) ); /* - * A control map listens to exactly one keyboard and one gamepad, matched by - * id, and a binding whose id does not match the event's is not consulted. - * That is what keeps two local players on two keyboards apart -- and it is - * also why "the arrow keys do nothing" is usually the wrong id rather than - * the wrong key. + * A control map listens to one keyboard and one gamepad, matched by id, and + * 0 means "any". That is what a one-player game wants, and it is the only + * thing that works: SDL_GetKeyboards() reports what is attached, but the id + * a key event carries is chosen by the video backend, and on X11 it is + * SDL_GLOBAL_KEYBOARD_ID (0) without XInput2 and the physical device's + * sourceid with it -- never the SDL_DEFAULT_KEYBOARD_ID of 1 that SDL + * registers when XInput2 is missing. This function used to bind + * SDL_GetKeyboards()[0] and the arrow keys did nothing at all. + * + * Bind a specific id when two players share a machine on two keyboards. */ - keyboards = SDL_GetKeyboards(&count); - if ( keyboards != NULL ) { - if ( count > 0 ) { - controlmap->kbid = keyboards[0]; - } - SDL_free(keyboards); - } - gamepads = SDL_GetGamepads(&count); - if ( gamepads != NULL ) { - if ( count > 0 ) { - controlmap->jsid = gamepads[0]; - } - SDL_free(gamepads); - } + controlmap->kbid = 0; + controlmap->jsid = 0; /* ---- keyboard ---- */ control.event_on = SDL_EVENT_KEY_DOWN; @@ -418,19 +408,39 @@ akerr_ErrorContext *ss_player_autoplay(int frame) FAIL_ZERO_RETURN(errctx, ss_game.player, AKERR_NULLPOINTER, "ss_game.player"); PASS(errctx, aksl_memset((void *)&synthetic, 0x00, sizeof(SDL_Event))); - synthetic.type = SDL_EVENT_KEY_DOWN; - /* - * The headless smoke run has no keyboard, so it calls the handlers a - * keyboard would have called. They take the actor and an event, and the - * keyboard handlers do not read the event beyond requiring one -- which is - * what makes a scripted run possible at all. + * Dispatch through akgl_controller_handle_event rather than calling the + * handlers directly, so a scripted run exercises the binding table the way a + * player does. The first version of this called the handlers, which meant + * the smoke test passed while the control map matched nothing at all and the + * arrow keys did nothing -- a test that cannot fail is not a test. + * + * SS_AUTOPLAY_KBID is deliberately not 0: the map binds keyboard 0 meaning + * "any", and driving it from a non-zero device id is what proves that. */ if ( frame == 1 ) { - PASS(errctx, akgl_actor_cmhf_right_on(ss_game.player, &synthetic)); + synthetic.type = SDL_EVENT_KEY_DOWN; + synthetic.key.which = SS_AUTOPLAY_KBID; + synthetic.key.key = SDLK_RIGHT; + PASS(errctx, akgl_controller_handle_event((void *)&akgl_game.state, &synthetic)); + FAIL_ZERO_RETURN( + errctx, + AKGL_BITMASK_HAS(ss_game.player->state, AKGL_ACTOR_STATE_MOVING_RIGHT), + AKGL_ERR_BEHAVIOR, + "the right arrow did not reach the player; the control map matched nothing" + ); } + if ( (frame % SS_AUTOPLAY_JUMP_PERIOD) == 0 ) { - ss_game.jump_requested = true; + synthetic.type = SDL_EVENT_KEY_DOWN; + synthetic.key.which = SS_AUTOPLAY_KBID; + synthetic.key.key = SDLK_SPACE; + PASS(errctx, akgl_controller_handle_event((void *)&akgl_game.state, &synthetic)); + } else if ( (frame % SS_AUTOPLAY_JUMP_PERIOD) == SS_AUTOPLAY_JUMP_HOLD ) { + synthetic.type = SDL_EVENT_KEY_UP; + synthetic.key.which = SS_AUTOPLAY_KBID; + synthetic.key.key = SDLK_SPACE; + PASS(errctx, akgl_controller_handle_event((void *)&akgl_game.state, &synthetic)); } SUCCEED_RETURN(errctx); } diff --git a/examples/sidescroller/sidescroller.h b/examples/sidescroller/sidescroller.h index 4e2d048..a8d62f8 100644 --- a/examples/sidescroller/sidescroller.h +++ b/examples/sidescroller/sidescroller.h @@ -72,6 +72,14 @@ /* How far the autoplay script waits between jumps, in frames. */ #define SS_AUTOPLAY_JUMP_PERIOD 45 +/* How long it holds the jump key, in frames. A tap jumps lower than a hold. */ +#define SS_AUTOPLAY_JUMP_HOLD 8 +/* + * The device id the autoplay script stamps on its synthetic key events. + * Deliberately not 0: the control map binds keyboard 0 meaning "any keyboard", + * and driving it from a non-zero id is what proves that binding works. + */ +#define SS_AUTOPLAY_KBID 11 /** * @brief What one call to ss_collide_resolve found. diff --git a/include/akgl/controller.h b/include/akgl/controller.h index 7d9b1b0..31ed9f5 100644 --- a/include/akgl/controller.h +++ b/include/akgl/controller.h @@ -87,7 +87,7 @@ typedef struct { akgl_Actor *target; /**< The actor these bindings drive. A `NULL` target makes the whole map inert, which is how an unused slot is spelled. */ uint16_t nextMap; /**< Number of bindings in use; the index akgl_controller_pushmap writes to next. */ akgl_Control controls[AKGL_MAX_CONTROLS]; /**< The bindings, scanned in order. */ - SDL_KeyboardID kbid; /**< Keyboard this map listens to. A keyboard event from any other id is ignored, which is what keeps two players on two keyboards apart. */ + SDL_KeyboardID kbid; /**< Keyboard this map listens to, or 0 for any. A non-zero id ignores every other keyboard, which is what keeps two players on two keyboards apart. */ SDL_JoystickID jsid; /**< Gamepad this map listens to, matched the same way. */ SDL_MouseID mouseid; /**< Mouse this map listens to. Declared but not yet consulted. */ SDL_PenID penid; /**< Pen this map listens to. Declared but not yet consulted. */ @@ -249,10 +249,14 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_controller_pushmap(int controlmapid, akg * @param actorname Registry name of the actor to drive. Required in practice, * though a `NULL` is reported as "not found" rather than as * a null pointer. - * @param kbid SDL keyboard id to listen to. Only events from this - * keyboard match; see akgl_controller_list_keyboards for - * how to find it. - * @param jsid SDL gamepad id to listen to, matched the same way. + * @param kbid SDL keyboard id to listen to, or **0 for any keyboard**, + * which is what a single-player game wants. Do not pass + * `SDL_GetKeyboards()[0]`: that reports what is attached, + * while the id an event carries is chosen by the video + * backend, and the two need not agree. See + * akgl_controller_list_keyboards. + * @param jsid SDL gamepad id to listen to, or 0 for any gamepad, + * matched the same way. * @return `NULL` on success, otherwise an error context owned by the caller. * @throws AKERR_OUTOFBOUNDS If @p controlmapid is at or above * #AKGL_MAX_CONTROL_MAPS, or if the map cannot hold eight more bindings. diff --git a/src/controller.c b/src/controller.c index 3b9096a..83f5fc7 100644 --- a/src/controller.c +++ b/src/controller.c @@ -291,14 +291,32 @@ akerr_ErrorContext *akgl_controller_handle_event(void *appstate, SDL_Event *even //SDL_Log("button/key is processed by controlmap %d control %d", i, j); //SDL_Log("event %d -> control on %d off %d", event->type, curcontrol->event_on, curcontrol->event_off); // This controlmap processes this control + /* + * A map bound to device 0 hears every device of that kind. + * + * Binding a specific id is what keeps two local players on two + * keyboards apart, so it stays -- but a game with one player + * cannot ask for "the keyboard" any other way. SDL_GetKeyboards() + * reports what is attached; the id a key event actually carries + * is chosen by the video backend, and on X11 it is + * SDL_GLOBAL_KEYBOARD_ID (0) without XInput2 and the physical + * slave device's sourceid with it. Neither is the + * SDL_DEFAULT_KEYBOARD_ID of 1 that SDL registers when XInput2 is + * missing, so a game that bound SDL_GetKeyboards()[0] got a map + * that matched nothing and controls that did nothing. + * + * 0 is the right spelling for "any": SDL documents `which` as 0 + * when the source is unknown or virtual, and joystick ids start + * at 1, so no real device is 0 and nothing is given up. + */ eventButtonComboMatch = ( ((event->type == SDL_EVENT_GAMEPAD_BUTTON_DOWN || event->type == SDL_EVENT_GAMEPAD_BUTTON_UP) && - event->gbutton.which == curmap->jsid && + (curmap->jsid == 0 || event->gbutton.which == curmap->jsid) && event->gbutton.button == curcontrol->button) || ((event->type == SDL_EVENT_KEY_DOWN || event->type == SDL_EVENT_KEY_UP) && - event->key.which == curmap->kbid && + (curmap->kbid == 0 || event->key.which == curmap->kbid) && event->key.key == curcontrol->key) ); if ( event->type == curcontrol->event_on && eventButtonComboMatch) { diff --git a/tests/controller.c b/tests/controller.c index d5c4d19..3756f8e 100644 --- a/tests/controller.c +++ b/tests/controller.c @@ -372,6 +372,74 @@ akerr_ErrorContext *test_controller_handle_gamepad_events(void) SUCCEED_RETURN(e); } +/* + * A control map bound to keyboard 0 hears every keyboard. + * + * There is no way to enumerate "the keyboard the player will actually type on". + * SDL_GetKeyboards() reports what is attached, but the id a key event carries is + * decided by the video backend: X11 without XInput2 sends + * SDL_GLOBAL_KEYBOARD_ID, which is 0, while X11 *with* XInput2 sends the + * physical slave device's sourceid -- and neither is the SDL_DEFAULT_KEYBOARD_ID + * of 1 that SDL_AddKeyboard registers when XInput2 is missing. A game that binds + * SDL_GetKeyboards()[0] therefore gets a map that matches nothing at all. + * + * 0 is the right spelling for "any", because SDL documents `which` as 0 when the + * source is unknown or virtual, so no real device can be 0 and nothing is given + * up by spending it. The same argument applies to a joystick id of 0. + * + * Every other test in this suite dispatches an event whose id equals the id the + * map was bound with, so none of them can see this. + */ +akerr_ErrorContext *test_controller_wildcard_device_ids(void) +{ + PREPARE_ERROR(e); + SDL_Event event; + + ATTEMPT { + reset_control_maps(); + CATCH(e, make_player()); + // 0 for both: "whatever keyboard and whatever gamepad". + CATCH(e, akgl_controller_default(0, "player", 0, 0)); + + // A key event from a real XInput2 device id must still be heard. + player->state = 0; + make_key_event(&event, SDL_EVENT_KEY_DOWN, TEST_KBID, SDLK_LEFT); + TEST_EXPECT_OK(e, akgl_controller_handle_event(&appstate_placeholder, &event), + "dispatching a left key press from a real device id"); + TEST_ASSERT(e, AKGL_BITMASK_HAS(player->state, AKGL_ACTOR_STATE_MOVING_LEFT), + "a map bound to keyboard 0 ignored a key press from device %d (state %d)", + TEST_KBID, player->state); + + // And so must one carrying SDL_GLOBAL_KEYBOARD_ID, which is 0. + player->state = 0; + make_key_event(&event, SDL_EVENT_KEY_DOWN, 0, SDLK_RIGHT); + TEST_EXPECT_OK(e, akgl_controller_handle_event(&appstate_placeholder, &event), + "dispatching a right key press from the global keyboard"); + TEST_ASSERT(e, AKGL_BITMASK_HAS(player->state, AKGL_ACTOR_STATE_MOVING_RIGHT), + "a map bound to keyboard 0 ignored a key press from keyboard 0 (state %d)", + player->state); + + /* + * A map bound to a *specific* keyboard still hears only that one. This is + * the half that keeps two local players on two keyboards apart, and the + * wildcard must not cost it. + */ + reset_control_maps(); + CATCH(e, akgl_controller_default(0, "player", TEST_KBID, TEST_JSID)); + player->state = 0; + make_key_event(&event, SDL_EVENT_KEY_DOWN, TEST_KBID + 1, SDLK_LEFT); + TEST_EXPECT_OK(e, akgl_controller_handle_event(&appstate_placeholder, &event), + "dispatching a left key press from the wrong device"); + TEST_ASSERT(e, (player->state == 0), + "a map bound to keyboard %d acted on a press from %d (state %d)", + TEST_KBID, (TEST_KBID + 1), player->state); + } CLEANUP { + } PROCESS(e) { + } FINISH(e, true); + + SUCCEED_RETURN(e); +} + akerr_ErrorContext *test_controller_handle_event_edge_cases(void) { PREPARE_ERROR(e); @@ -872,6 +940,7 @@ int main(void) CATCH(errctx, test_controller_default_bindings()); CATCH(errctx, test_controller_handle_keyboard_events()); CATCH(errctx, test_controller_handle_gamepad_events()); + CATCH(errctx, test_controller_wildcard_device_ids()); CATCH(errctx, test_controller_handle_event_edge_cases()); CATCH(errctx, test_controller_gamepad_button_handlers()); CATCH(errctx, test_controller_device_events());