Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

buresu
Copy link
Contributor

@buresu buresu commented Nov 24, 2024

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

@buresu buresu requested a review from a team as a code owner November 24, 2024 16:10
@buresu buresu changed the title Fix underscore input not working on jp106 keyboard with wayland platform Fix underscore input not working with jp106 keyboard on wayland platform Nov 24, 2024
@Riteo
Copy link
Contributor

Riteo commented Nov 25, 2024

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:

if (physical_keycode == Key::NONE && keycode == Key::NONE && unicode == 0) {
return;
}
if (keycode == Key::NONE) {
keycode = (Key)physical_keycode;
}

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
Copy link
Member

bruvzg commented Nov 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants