Fixed keycode mismatch, which caused "pressed" trouble with modifiers

[tomlechner]

This fixes (on linux) where non-letter keys pressed with modifiers sometimes get their pressed state wrong, like pressing shift-7.

Steps to reproduce problem:

1. press 7, "7" registers as pressed
2. press shift, "shift" reports as pressed
3. release 7, nothing happens!
4. release shift, "shift" reports as released
5. query "7" and godot says it's still pressed.
6. tap 7 again and it reports as an echo of 7, then a release of 7

Minimal reproduction project:
KeycodeBug.zip

This code seems vaguely related to issues #2915 and #8039.

If I understand the Godot code, there seemed to be confusion between X11 keycodes (unique per physical key, usually but not necessarily same on different keyboard hardware) and keysyms (might be many per physical key). Godot seems to want a "scancode" that is basically unique per physical key, but is numerically similar to keysyms, possibly or'd with modifiers (unlike native x11 keycodes, which are in range 8..255).

The old code used XLookupString to get a "keycode", but that actually returns a keysym filtered by modifiers, messing things up when doing "out of sync" modifier presses/releases like above. Instead using XLookupKeysym(xevent,0) always returns the first in a list of keysyms for that keycode, which is basically unique.

note: this is my first PR here, not sure if I should have done an issue first, or both!

[akien-mga]

CC @reduz @hpvb @bruvzg @Hinsbart

[bruvzg]

This PR breaks input on all non QWERTY layouts! (All layouts gives QWERTY key-codes).

I have tied to use XLookupKeysym to get QWERTY (layout independent) scan-codes in #18020 but this function behaves differently on different Linux distributions/DEs (Gives QWERTY key-codes on Debian GNU/Linux 9, Xfce 4 and localised key-codes on Manjaro Linux 17, KDE for example) so I end up with direct scan-code parsing.

release 7, nothing happens!

On Linux (Debian) Shift + 7 reports 38 - Ampersand key-code, and on macOS and Windows it reports 55 - 7. So I guess it should be changed somehow to be consistent on all platforms.

[tomlechner]

In key_mapping_x11.cpp, it looks like it is converting an x keysym to a code that arbitrarily limits to less than 0x100, and to any known conversion in _xkeysym_to_keycode[]. Every other value such as unicode based syms (range 0x1000000 to 0x0110ffff) is zapped to 0. I didn't touch that file. Is it possible that is what's breaking non-qwerty layouts, rather than XLookupKeysym usage?

I'm only on debian and can't test on Manjaro or arch, but XLooupKeysym seems to pull from a keysym list that is specific to X keycode, with the list order being independent of modifiers, but is "localized" for current keymapping. I made this to get that full list. For me, both Shift-7 and 7, XLooupKeysym(ev, 0) returns 55 (keysym for 7), and XLooupKeysym(ev, 1) returns 38 (keysym for ampersand). They both index the same list based on keycode 16. I wonder what Manjaro is returning?

If I switch mappings like to something with cyrillic, XLooupKeysym no longer returns qwerty values, but returns the remapped cyrillic syms. index of 0, as far as I can tell anyway, still appears to be basically unique per physical key per system. Not as broadly unique as system keycodes perhaps, but even those change sometimes (i think, judging from this).

I switch between US and Dvorak a lot. It's very rare indeed to find a game that doesn't make me switch or remap controls all the time, your approach of system keycodes is certainly a more thorough fix, absolutely my preference!

However, I just tried your pr, and it does not fix the modifier problem of this pr. I guess wherever keypressed state is stored is still using old keycode value, rather than your new qwerty keycode?

[rosshadden]

I have this issue on Arch Linux. I use Dvorak, though the problem exists in US as well. I've tried asking in IRC but nobody knows or cares what I'm talking about. I seem to have finally found the right place, right before I filed my own issue about it :).

Basically shift is completely unusable as an input mapping, which includes using it with both input.is_action_pressed and input.is_key_pressed. Not only is it unusable, but as the OP mentioned, it basically breaks all other controls from the moment you hit shift.

[bruvzg]

@rosshadden, @tomlechner I have updated #18020 PR to fix the modifier key problem, please give it a try.

[tomlechner]

That seems to work! At least for keys, and single key actions (crunched for time, I didn't try Control-alt-windows key combos).

A multi-key action bound to shift-7, for instance, fails with the sequence above, in that it never registers as true. Also if you do shift - 7 - releaseShift - release7, that Shift+7 action goes true when you press 7, but stays true when you release shift, then goes false when you release 7. I would expect it to be true only when both keys are pressed.

Although I see that problem exists with my fix as well!

[rosshadden]

It works great for me, too!

[mhilbrunner]

Whats the status of this?

[mhilbrunner]

Whats the status of this?

[akien-mga]

If I understand correctly #18020 supersedes this PR, right?

[tomlechner]

#18020 is a more thorough fix for the original problem I had here, yes.

[akien-mga]

Thanks, closing then and we'll continue the work on #18020.