SDL: Use scancodes for keybindings

[y5nw]

This PR makes Minetest use scancodes for keybindings.

• Goal of the PR
  Make Minetest use scancodes (instead of keycodes) for key bindings.
• How does the PR work?
  The SDL scancode is passed to SKeyInput::SystemKeyCode. The KeyPress class is modified to be basically a wrapper around this.
• Does it resolve any reported issue?
  Fixes Use scancodes for key binding #14940
• Does this relate to a goal in the roadmap?
  Yes - SDL.
• If not a bug fix, why is this PR needed? What usecases does it solve?
  This was discussed recently.

Related: #8781 (not fully fixed), #12507 (new default key), #12556 (relevant code removed/refactored), #13904, #14545, #14874 (allows people to continue using e.g. Shift+7 if they want to)

To do

This PR is ready for review.

• Switch to scancodes for SDL. Address regressions:
  • Fix: Escape and AC_BACK keys not working.
  • Fix: (Android) Cannot close formspec when a text field is focused.
  • Fix: actions without keybindings are bound to the left mouse button.
  • Properly implement scancodes (using std::variant) for keypresses from non-keyboard input (mouse, joystick, etc.)
  • Convert KEY_OEM_* keys to scancodes directly. (See note below)
    The current approach no longer requires changing non-SDL IrrlichtDevices.
• Partial rework of KeyPress and event handling
  • Rework KeyPress
  • Allow binding to keys with keychars (keycodes) not in the lookup table.
  • Allow binding to keys with no keycode (e.g. dead keys).
  • Change touchscreen GUI to send events with scancodes.
• Keybinding settings
  • Add syntax for scancode settings. (see notes)
• Followup PRs:
  • Allow using secondary keybindings: Implement secondary keybindings #15577
  • Allow keybindings with modifer keys: Allow keybindings with modifiers #14874
  • Important: Warn users about new keybindings.
  • After removing non-SDL IrrlichtDevices:
    • Expose SDL headers to Luanti.
    • Implement std::hash<KeyPress> and replace KeyList with std::unordered_set<KeyPress>
    • Implement SDL-specific scancode names (i.e. SDL_SCANCODE_x). (Useful for Support custom key binds (with keyboard and touchscreen support) #12488) Note that this requires Luanti (not just Irrlicht) to have access to SDL headers.
    • Fix unittests

How to test

• Set keybindings as usual or use existing keybinding settings. (In particular, test binding to keys such as backtick, ü, and dead keys). Observe that the keybinding (in terms of scancodes) is the same after switching to a different layout.
• Observe that both the Escape key and the Android back key work as the same key.

Notes

• The scancode-based keybinding syntax introduced in this PR is only used (and enabled) for SDL builds.
• Unittests have been disabled:
  • The behavior of KeyPress is fully dependent on the Irrlicht device.
  • Scancode/keycode conversions for SDL require an active IrrlichtDevice which is not available in unittests.
• The "press Shift to bind to symbols" feature has been removed as it does not work with SDL scancodes. Note that this causes existing configurations to break even when Minetest is built without SDL.
• The Irrlicht source code suggests that the various OEM keys refer to phsical keys (similar to SDL scancodes), but on my non-SDL X11 build it appears that the exact OEM key still depends on the keyboard layout.
• SDL: Use scancodes for keybindings #14964 (comment)
• This PR uses scancodes for individual keys. In particular, left shift and right shift are treated as separate keys. Addressing this requires a separate/followup PR that implements secondary keybindings.
• We will end up (one way or another) requiring requiring some users to rebind their keys regardless of what we choose as the default. Scancode-based defaults basically break keyboard layouts where a key is located in a different position than its US counterpart, while keycode-based configurations would break on layouts where a key is not present in the unshifted position. (SDL: Use scancodes for keybindings #14964 (comment) and https://irc.minetest.net/minetest-dev/2024-12-26#i_6230893) Using a mixture of both can result in key conflicts (https://irc.minetest.net/minetest-dev/2024-11-10#i_6216414)

[rubenwardy]

Very nice work y5nw! Thanks for picking this up so quickly

[rubenwardy]

When using the X11 driver, pressing a key (P) in the controls menu shows invalid wide string

Entering dev test, I'm completely unable to do anything using the keyboard. I can't move, jump, or open the pause menu. Default settings, Linux, EndeavourOS. QWERTY keyboard in UK layout

[y5nw]

When using the X11 driver, pressing a key (P) in the controls menu shows invalid wide string

Oops, I wrote some boilerplate code to allow other devices to pass scancode information but did not actually implement scancode passing/handling for devices other than SDL. Will fix.

[y5nw]

Testing needed. Note that binding to Shift+[Key] is also removed for non-SDL devices and non-SDL devices practically still use keycodes instead of scancodes.

[rubenwardy]

Sorry for being such a Linux user, but this key should be called Left Super not Left Windows:

[rubenwardy]

Inc range is unbound by default. Should be bound to =

[rubenwardy]

This PR does not change the default keybindings to scancodes.
The default setting in this PR remains that keybindings are saved as keycodes.

Would it not make sense to change the defaults when built with SDL? We don't need to worry about breaking user configs created using a -dev version

save_keys_as_scancodes = true
keymap_toggle_fog = <60>
keymap_toggle_chat = <59>
keymap_cmd = <56>
keymap_backward = <22>
keymap_toggle_hud = <58>
keymap_freemove = <14>
keymap_screenshot = <69>
keymap_right = <7>
keymap_left = <4>
keymap_sneak = <225>
keymap_camera_mode = <6>
keymap_minimap = <25>
keymap_forward = <26>
keymap_fastmove = <13>
keymap_aux1 = <8>
keymap_jump = <44>
keymap_hotbar_previous = <5>
keymap_increase_viewing_range_min = <46>
keymap_zoom = <29>
keymap_cmd_local = <55>
keymap_drop = <20>
keymap_inventory = <12>
keymap_hotbar_next = <17>
keymap_noclip = <11>
keymap_mute = <16>
keymap_chat = <23>
keymap_console = <67>
keymap_decrease_viewing_range_min = <45>

[y5nw]

Sorry for being such a Linux user, but this key should be called Left Super not Left Windows:

This is defined in the existing keycode map:
https://github.com/minetest/minetest/blob/b0ad9a6c3321c58044c5bcb5d2c0d904487d14fa/src/client/keycode.cpp#L128-L129

In the future (i.e. after fully switching to SDL), this map should ideally be refactored to drop irr::EKEY_CODE (since it is just another layer of abstraction) in favor of SDL's native keycode (potentially also scancode, if we want to make the default settings more readable than just numbers). I would prefer renaming the keys at that point.

Inc range is unbound by default. Should be bound to =

Yes, this is a result of = not corresponding to a scancode (and therefore being unset). This should be fixed now that the default settings have been adjust to use scancodes instead:

This PR does not change the default keybindings to scancodes.
The default setting in this PR remains that keybindings are saved as keycodes.

Would it not make sense to change the defaults when built with SDL? We don't need to worry about breaking user configs created using a -dev version

On a second thought, this would make sense. I have added this in a later commit now.

That said, I have chosen to keep F1, F2, etc as keycodes as I do not see any reason to use scancodes for those (and such keys usually have a scancode anyway).

(I only skimmed through the defaultsettings.cpp file and had the impression that the file would be parsed be some other code. Turned out I was wrong.)

[okias]

I would love to backport this one to 5.9.0, so we can have proper SDL 2 for Linux distros (and we can run Minetest nicely on the phones).

If I would ask if it would be possible to keep this MR without behavior changes when using SDL 2 and drop anything which changing behavior into followup MR.

Thank you for addressing it so quickly!

[y5nw]

I would love to backport this one to 5.9.0, so we can have proper SDL 2 for Linux distros (and we can run Minetest nicely on the phones).

If I would ask if it would be possible to keep this MR without behavior changes when using SDL 2 and drop anything which changing behavior into followup MR.

The PR in itself is intended to bring a (potentially breaking) behavior change: switching from SDL keycodes to scancodes, so I don't think it would make much sense to split this PR specifically.

My intention with this PR is to have this merged immediately before/after creating a hard dependency on SDL, as support for the non-SDL device is (at least) partly broken by this PR anyway and it would therefore not make sense to ship a release with a non-SDL device with breaking changes only to break things again in the release after that. The result of this intention is that the code changes are relatively (in comparison to another approach I attempted) small and simple (quite some things can be removed without much pain in a post-SDL refactor) but at the cost of partly breaking non-SDL devices.

[rubenwardy]

We're planning on releasing much more frequently, on a regular interval of every 2-3 months rather than 6-9 months. The idea is to release on a date rather than waiting for certain features to make it in. If this gets in soon then we can release 5.10.0 with SDL2 in October.

There's also an argument to release 5.10 sooner than that if we finish all the SDL2 required changes

We cannot backport this to 5.9.1 with our version scheme, it would have to be 5.10.0

[okias]

We're planning on releasing much more frequently, on a regular interval of every 2-3 months rather than 6-9 months.

amazing, that sounds like very good news to me! :)

If the new release comes soon after the SDL2 testing and switch, that would help the effort of bringing more users to mobile Linux platforms by a lot (X11 is serious pain on mobile). Good games always help 😉

[Zughy]

@y5nw rebase needed

[appgurueu]

Took a look at the code (looks pretty good overall), haven't tested so far. Thanks for working on this.

[sfan5]

Future steps, as proposed:

1. We agree on the following:

• Settings don't need to be backwards-compatible (mainmenu dialog needed!)
• Settings don't need to be forwards-compatible
• Immediately breaking all non-SDL devices is okay

2. PR is reworked as needed (e.g. to remove unnecessary compat)
3. We merge it early in the 5.12 cycle and work on issues that crop up.
4. ???
5. Profit

[y5nw]

* (mainmenu dialog needed!)

+1. Although I would prefer having this in a different PR. (Also: do we have code to check whether the user upgraded from a preivous version? Or would we simply check whether the user has settings based on keycodes?)

* Settings don't need to be forwards-compatible

Agreed. The current numeric scancodes are a temporary/fallback solution anyway; I would personally prefer naming keys based on SDL scancodes as such names are IMO much easier to work with.

(Note that SDL renamed certain scancodes from SDL2 to SDL3.)

* Immediately breaking all non-SDL devices is okay

I sort of did: the modified GUIKeyChangeMenu no longer considers modifier keys, so it is no longer possible to bind to e.g. Shift-7 for / even on non-SDL devices.

The main limitation here is that SDL headers are not exposed to Luanti except for Android builds, which limits our ability to use e.g. SDL scancodes directly.

2. PR is reworked as needed (e.g. to remove unnecessary compat)

Aside from keeping the PR buildable with non-SDL devices (and without SDL headers, as I mentioned above), the other piece of compat code involves keeping compatibility with keycodes. I am largely not sure whether we want to (at least) attempt to help migrate user settings to scancodescodes. Note that we currently cannot drop EKEY_CODE yet due to the lack of SDL headers. I still need to figure out how to work with cmake, so I would prefer having a follow-up PR to require SDL - or would we merge such a PR before this one?

3. We merge it early in the 5.12 cycle and work on issues that crop up.

I suppose I will just request a review when I finish some of the cleanup (and rebase).

Another idea here is to tag (in what name?) such an SDL build to make users aware of the oncoming change.