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 non-latin layout scancodes on Linux, adds access to physical scancodes. #18020

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Apr 6, 2018

[Linux/X11] Fix non-latin keyboard layout scan-codes. (see #17827 comments).
Fallback to physical keycode if layout remapping fails.

Fixes #8039

[Linux/X11, macOS, Windows, UWP] scancode renamed to keycode, added physical_keycode to InputEventKey.
Layout independent keycode, which represent the physical location on keyboard. (e.g. keys in highlighted locations will produce event with keycode = Q, MINUS, K, C on DVORAK-L layout, and physical_keycode = W, A, S, D on any keyboard layout.)

dvrk

@reduz
Copy link
Member

reduz commented Apr 7, 2018

I would like to know why remapping was changed for OSX only, did we not have remapping in other platforms?

The idea of the scancodes is that they are keyboard dependent, there is no use case IMO for physical (qwerty) keycodes.

@bruvzg
Copy link
Member Author

bruvzg commented Apr 7, 2018

I would like to know why remapping was changed for OSX only, did we not have remapping in other platforms?

All platforms have remapping, macOS was only one limited to some hardcoded mappings, #17827 changes mapping from hardcoded to generated for current OS keyboard layout.

Edit: Summary for remapping on different platforms

Windows:
OS provide both physical scancode and remapped virual key code. No additional remapping required, works with all layouts.

Linux/X11:
OS provides only physical scancode. Remapping is done using XLookupString, works with latin variants only, with non-latin layouts return zeros, this PR add fallback to physical scancode in case XLookupString remapping fails (AFAIK most non-latin keyboards have both QWERTY and local symbols).

masOS:
OS provides only physical scancode.
Before #17827 remapping was done using hardcoded tables in keyboard.cpp (only for 5 most common layouts, this code was not used by other platforms).
After #17827 remapping is done using UCKeyTranslate (works in a similar way to X11 remapping), with fallback to hardware scancode if remapping fails.

@bruvzg
Copy link
Member Author

bruvzg commented Apr 7, 2018

The idea of the scancodes is that they are keyboard dependent, there is no use case IMO for physical (qwerty) keycodes.

Don't know, for common game keys like WASD I would care only about physical location not letters on the keys. Most GUI frameworks I have used and libs like SDL and glfw provide both scancode for physical location and keycode for layout remapped key.

@bruvzg bruvzg force-pushed the input_fix_non_latin_and_add_hw_scancodes branch 2 times, most recently from 037191a to 2c9413f Compare April 23, 2018 21:04
@rosshadden
Copy link
Contributor

rosshadden commented Apr 24, 2018

Letting you map to the physical keys is a(n old) dream come true! This branch is a bit buggy when adding a remapped key or changing existing key mappings (as of 2c9413f), but the core feature itself works and is SO VERY exciting for me.

I always have things mapped to dvorak keys while developing a game and change them out later once I add configuration. This makes it particularly complicated when sharing prototypes with people before this point, as I have to remap everything just for them, and then back.

@bruvzg bruvzg force-pushed the input_fix_non_latin_and_add_hw_scancodes branch from 2c9413f to 2959519 Compare May 28, 2018 10:56
@mhilbrunner
Copy link
Member

Whats the status of this?

@reduz
Copy link
Member

reduz commented Jul 25, 2018

Let me explain why this should not be merged.

  1. All keyboards, no matter the language, whether Japanese, Arabic, Hebrew, etc. have a latin mode. This is what Godot returns as scancode. If it doesn't for some reason, it should be fixed but that's the idea.
  2. For the actual symbol, Godot returns Unicode.

Physical scancode (which is pretty much the US keyboard code) is undesired, as it's non standard.

@reduz
Copy link
Member

reduz commented Jul 25, 2018

The suggested feature to get something like this working, would be to add a function to OS:

OS.get_IBM_scancode(scancode)

that translates an IBM scancode to godot latin key, and a backwards one. This way, common Input functions can still be used like:

Input.is_key_pressed( OS.IBM_to_scancode(KEY_A) )

if (ev.scancode == OS.IBM_to_scancode(KEY_A) ):

or something similar, a better name needs to be found.

@bruvzg
Copy link
Member Author

bruvzg commented Jul 25, 2018

The suggested feature to get something like this working, would be to add a function to OS:
OS.get_IBM_scancode(scancode)

38335618-8e60e15a-3867-11e8-99f6-55eb3ddadd67
Lets look at this keyboard layout, currently following scancodes are returned:

  • Blue Key -> KEY_G
  • Red Key -> KEY_E (on macOS and Linux with this PR), KEY_SEMICOLON (Windows) or 0 (Linux without this PR)
  • Green Key -> KEY_E

With current scancodes OS.get_IBM_scancode(scancode) like function will not work since two keys return same scancode in this layout.

What should be correct scancode for Red Key? KEY_G (it will be duplicate key as well)? Or something else?

@bruvzg
Copy link
Member Author

bruvzg commented Jul 25, 2018

Physical scancode (which is pretty much the US keyboard code) is undesired, as it's non standard.

It is standard. X11 and macOS keypress events provide ONLY physical key codes. Windows events have both physical and remapped key codes.

@moiman100
Copy link
Contributor

moiman100 commented Jul 30, 2018

This has currently build errors after rebase, but they are just two lines which aren't used anymore it seems.
EDIT:
Just noticed that editing key bindings doesn't seem to work.

I think this is really nice and should be merged after it is fixed.

I don't think all of these are problems of this PR, but it'd be nice if they would be fixed.
My keyboard is 102/105 - ISO using Finnish QWERTY layout.

  1. Context menu key is unknown on remapped, but works on hardware. VK_APPS on windows
  2. < key is unknown on both. VK_OEM_102 on windows
  3. If num lock is off on remapped keypad 5 is unknown.
  4. Meta key show "Meta +" on remapped and "Meta" on hardware.
  5. Media keys and calc key are unknown on hardware. (Calc is launch1 on remapped)
  6. Play/Pause is unknown on remapped. Other media keys work.

Here is a img of different physical layouts to show what might be missing on other layouts.
This is from wikipedia.

@bruvzg
Copy link
Member Author

bruvzg commented Jul 30, 2018

All keyboards, no matter the language, whether Japanese, Arabic, Hebrew, etc. have a latin mode. This is what Godot returns as scan-code. If it doesn't for some reason, it should be fixed but that's the idea.

All keyboards have a way to input latin characters, but not all keys on every keyboard have base latin char associated with it (see ĞIÜŞÖÇ key in one of my previous comments), I do not see any reasonable way to assign scan-code for these keys.

Personally I don't think remapped scan-codes have any use at all, menu shortcuts and other staff implying reference to the specific word should be bound to unicode character instead of scan-code.

Everything else should use physical scan-codes, but UI should display character representation for current layout (for this we'll probably need to process OS keyboard layout change event, like WM_INPUTLANGCHANGE on Windows and kTISNotifySelectedKeyboardInputSourceChanged on macOS).

Context menu key is unknown on remapped, but works on hardware. VK_APPS on windows
< key is unknown on both. VK_OEM_102 on windows
If num lock is off on remapped keypad 5 is unknown.
Meta key show "Meta +" on remapped and "Meta" on hardware.
Media keys and calc key are unknown on hardware. (Calc is launch1 on remapped)
Play/Pause is unknown on remapped. Other media keys work.

Some scan codes are definitely missing.

On macOS media keys doesn't trigger any keypress event at all and I have unknown \| key on both macOS and Windows.

I prefer to wait until we all get to a consensus before fixing anything, otherwise this work'll be in vain.

@mischiefaaron
Copy link

mischiefaaron commented Aug 13, 2018

To clarify a bit, I am using Linux Mint MATE and I am using a U.S. QWERTY keyboard, but want to support multiple layouts. I am testing by switching the layout in software. I tend to use Dvorak, so I test with that. It reports the key correctly, but the keys all seem to be tied to a certain keyboard layout when they shouldn't be.

In response to Juan: "Physical scancode (which is pretty much the US keyboard code)"
I thought it applies to all keyboards as a standard because it refers to the placement of keys by number, not the character itself. I want to use them specifically so I can easily set up for the user one of many layouts that are outside of U.S. QWERTY keyboards, but they all give me different codes when I switch to different layouts in.

This is a little program I made to test this and it is how I came to my conclusions: https://github.com/agameraaron/simple-input-observer/releases/tag/v0.1

@akien-mga
Copy link
Member

Just tested to compile this on the current master branch after a rebase, and this diff needs to be applied:

diff --git a/editor/project_settings_editor.cpp b/editor/project_settings_editor.cpp
index 4016009cf..ca0b1490c 100644
--- a/editor/project_settings_editor.cpp
+++ b/editor/project_settings_editor.cpp
@@ -473,9 +473,6 @@ void ProjectSettingsEditor::_add_item(int p_item, Ref<InputEvent> p_exiting_even
 			last_wait_for_key = Ref<InputEvent>();
 			press_a_key->popup_centered(Size2(250, 80) * EDSCALE);
 			press_a_key->grab_focus();
-
-			device_special_value_label->hide();
-			device_special_value->hide();
 		} break;
 		case INPUT_MOUSE_BUTTON: {
 

@akien-mga
Copy link
Member

@bruvzg Commit 29595192 doesn't seem to depend on the other two, and would likely be simple to review by itself, so it might be worth splitting in its own PR?

As for the other two, I'm very much in favour of giving a way to access a kind of physical scancodes. I just tried the PR and it works great to bind e.g. 1, 2, 3, 4 (hardware) instead of &, é, ", ' (remapped) on AZERTY as actions like you would have typically in FPS or RTS games.

Some more discussion might be needed regarding how to name and expose the feature, and @reduz still needs to be convinced fully (but he's not too far from it ;)), which is why I suggest to split the 3rd commit for now.

@akien-mga
Copy link
Member

Moving to 3.2 milestone, we're very close to beta for 3.1 and there is still discussion and design changes needed for this PR.

@reduz
Copy link
Member

reduz commented Feb 23, 2020

What we discussed is that we should do something similar as this PR, including the Qwerty scancode (we should call it "us" or "hardware", or "physical" keycode, some people have no idea what qwerty is or how it looks like, also what @Yukitty mentions is not really doable due to modifiers and deadkeys).

The idea is that the following should be supported:

  • we should probably rename scancode to keycode, as scancode is misleading
  • The hardware scancode (physical_keycode property I propose?) should indeed be added to InputEventKey
  • Actions that are physical keycodes should be marked as such, it shouldl be saved/loaded as an InputEventKey with keycode == 0 , physical_keycode != 0
  • The key editor in ProjectSettings does not use InputMap, so it should always treat/display the event as physical keycode when the above condition is met.
  • When loaded in InputMap, it should just be converted from physical keycode to regular keycode by calling a function in OS. InputMap does not really need to understand physical keycodes.

Again, apologies for leaving this on StandBy, we did not properly document what we discussed in 2019 so this was lost, we tried to change this in 2020 and now we have a detailed doc for this.

@bruvzg
Copy link
Member Author

bruvzg commented Feb 23, 2020

When loaded in InputMap, it should just be converted from physical keycode to regular keycode by calling a function in OS. InputMap does not really need to understand physical keycodes.

That's bad idea, keyboard layout is not constant, especially if someone uses non-latin layout, they will probably have at least two layouts "Local" and "English" quickly switchable, and it probably will be switched if app have any text input involved and break input mappings that were loaded with different layout.

@reduz
Copy link
Member

reduz commented Feb 23, 2020

@bruvzg ah, I did not know that and did not come up in the discussion, clearly a product of our ignorance when we discussed it, i guess it then makes full sense to add support for it in InputMap. If this is the case, what would be remaining in this PR for it to be merged? The only suggestion we had remaining is renaming qwerty to physical.

@reduz
Copy link
Member

reduz commented Feb 23, 2020

and given we are breaking compat in 4.x, maybe renaming scancode to keycode, if there is agreement for this (we can use the script converter from 3 to 4 to automatically rename this too?)

@bruvzg bruvzg force-pushed the input_fix_non_latin_and_add_hw_scancodes branch from ce48f6a to 0caab5f Compare February 24, 2020 07:21
@bruvzg bruvzg requested a review from vnen as a code owner February 24, 2020 07:21
@bruvzg bruvzg force-pushed the input_fix_non_latin_and_add_hw_scancodes branch from 0caab5f to 1c1beee Compare February 24, 2020 08:26
@Yukitty
Copy link

Yukitty commented Feb 24, 2020

Thanks for the updates and consideration. I'm glad you're on top of this. 👍

@bruvzg
Copy link
Member Author

bruvzg commented Feb 24, 2020

  • scancode renamed to keycode, qwerty_scancode to physical_keycode
  • physical_keycode is used only if keycode == 0
  • both are available in Project Settings/Input Map, Editor Shortcuts are left as is.

Screenshot 2020-02-24 at 10 53 04

  • physical_keycode is implemented and tested on Linux/X11, macOS and Windows, implemented but not tested on Android and UWP, on the other platforms physical_keycode is always set to be equal to keycode.

Add `physical_keycode` (keyboard layout independent keycodes) to InputEventKey and InputMap.
Fix non-latin keyboard layout keycodes on Linux/X11 (fallback to physical keycodes).
@bruvzg bruvzg force-pushed the input_fix_non_latin_and_add_hw_scancodes branch from 1c1beee to 1af06d3 Compare February 25, 2020 10:31
@akien-mga akien-mga merged commit e2b66ca into godotengine:master Mar 1, 2020
@akien-mga
Copy link
Member

Thanks a lot!

@bruvzg bruvzg deleted the input_fix_non_latin_and_add_hw_scancodes branch November 5, 2021 12:45
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.

In game inputs from non english layouts should be treated as their qwerty equivalent (like SDL2 scancodes)
10 participants