godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.38k stars 21.26k forks source link

Fix underscore input not working with jp106 keyboard on wayland platform #99633

Open buresu opened 1 day ago

buresu commented 1 day ago

This PR fixes an issue where the underscore key can't be typed in the jp106 Japanese input environments on the wayland platform.

Details

Godot's wayland platform correctly converts keysyms using xkb and godot's xkb_keycode_map table. However, in jp106 environments, when retrieving the physical key, physical_key becomes Key::NONE and input fails because the scancode_map table doesn't have a value set corresponding to underscore (0x61: backslash).

Solutions

  1. Set Key::BACKSLASH to 0x61 in scancode_map While this solution works correctly in jp106 keymap environments, it breaks the other environments where 0x61 is assigned to different keys.

  2. Allow Key::NONE for physical_key This seems valid as godot's windows implementation also allows Key::NONE for physical_key.

This PR implements solution 2.

Additional Notes

Godot currently stores scancodes as its internal code, but converting to internal codes can result in loss of information when using keymapping. In the future, it might be better to store platform native scancodes. Looking at Qt's implementation, scancode is stored as uint32 type with platform native values. https://doc.qt.io/qt-6/qkeyevent.html

Riteo commented 3 hours ago

Hi, thank you for the PR!

IIRC the physical map is supposed to be mapped to an "average" QWERTY keyboard, hence the missing value, as this keyboard seems to have an extra key if I understand correctly.

Because of this, I think that an empty physical key makes sense and it indeed seems that the Wayland backend is being too strict compared to the other ones.

I agree with the solution, with a small gotcha. As-is, this patch would allow for keycode to be Key::NONE, which is not acceptable AFAIK.

Other backends (or at least the X11 one) seem to have a more sophisticated check where they fail if and only if there's no meaning at all, namely with a void physical keycode, keycode and unicode value.

I'd recommend to switch to a similar pattern:

https://github.com/godotengine/godot/blob/d09d82d433b03bb3773fd2a8cc8d6ccc2f8739ce/platform/linuxbsd/x11/display_server_x11.cpp#L3651-L3657

Note that unicode is obtained differently compared to X11 (looks like xcbcommon handles it for us). I think that the raw value returned by xkb_state_key_get_utf32 should work but I'm not really sure if it should be "sanitized" first. @bruvzg is way more knowledgeable in this regard and might help.

Godot currently stores scancodes as its internal code, but converting to internal codes can result in loss of information when using keymapping.

It looks like the Qt6 API still has a level of translation when it comes to checking and whatnot, which is needed for cross-compatibility, so that would be offsetting the problem I think. You're free to discuss this with other input people on our RocketChat instance if you want though! https://chat.godotengine.org

bruvzg commented 2 hours ago

physical_keycoed is intended to represent position on the QWERTY keyboard (primary purpose is game controls like WASD, when you do not care about keyboard layout, only about physical location of the keys), since there's no corresponding key for JP106 backslash, it's OK to have it set to NONE.

I'd recommend to switch to a similar pattern:

Yes, this definitely should be added, input should have at least some valid value.

Note that unicode is obtained differently compared to X11 (looks like xcbcommon handles it for us). I think that the raw value returned by xkb_state_key_get_utf32 should work but I'm not really sure if it should be "sanitized" first.

Should be OK, anything complex will be handled by IME (_wp_text_input_*).

Godot currently stores scancodes as its internal code, but converting to internal codes can result in loss of information when using keymapping.

If it's stored in addition to existing values, I do have anything against it, but it probably will be almost never used.