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

Wayland: Unstuck keys with same keycode #102641

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

Riteo
Copy link
Contributor

@Riteo Riteo commented Feb 10, 2025

Depends on #102602.
Fixes (only for Wayland) #102137.

This fixes once and for all the core issue of different Godot keycodes released from the same raw XKB keycode.

The InputEventKey keycode value should map to the "unmodified" key, but unfortunately there's an ambiguity with their encoding for "special" keys ("delete", "insert", etc.), in witch they ignore their unicode representation. This means that a key that is special when plain but a character when modified would never be properly picked up, so we do indeed change its keycode. As a consequence of this exception, some Godot keys never receive release events and get "stuck".

This patch adds an extra check through an HashMap to "unstuck" keys that changed while having the same keycode.

I also could not resist simplifying a bit the regular key event generation method but this makes things more consistent and predictable IMO.


Despite me not resisting to actually make a proper patch, this is actually somewhat of a RFC, as it's a bit of a "heavy handed" approach and makes some assumptions that I think should be discussed.

The main points I'm putting up are:

  1. Should this "unstucking" should be done in Input? Windows has similar issues with the numlock and I suppose macos too although it doesn't have one. Note that issue is not exclusive to numlock, just from keys which can be modified in/out of special values. I think that the fact that we rely on the native keycode could get too messy inside of Input so perhaps duplicating this behavior per-backend might be the safest bet.
  2. Should this "ambiguous" encoding be kept in the first place? I think so because of compatibility reasons.

@Riteo
Copy link
Contributor Author

Riteo commented Feb 10, 2025

I only now realized that using "unstuck" like this is not correct. I'm too lazy to fix it, sorry :P

@Riteo Riteo force-pushed the modifying-spacetime-itself branch from 389f6c5 to deb17a7 Compare February 10, 2025 03:14
@AThousandShips AThousandShips added this to the 4.4 milestone Feb 10, 2025
@Riteo Riteo force-pushed the modifying-spacetime-itself branch from deb17a7 to 1dd4a88 Compare February 10, 2025 13:12
@akien-mga akien-mga requested a review from bruvzg February 10, 2025 15:45
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be working as expected.

@bruvzg
Copy link
Member

bruvzg commented Feb 11, 2025

Should this "unstucking" should be done in Input?

Probably, but conditions in which it can happen on Windows is too specific to be a real issue, and macOS do not have NumLock, so it not something that is absolutely necessary to do now.

@akien-mga
Copy link
Member

Do we need to do something similar for X11?

@bruvzg
Copy link
Member

bruvzg commented Feb 11, 2025

Do we need to do something similar for X11?

The is_sym_numpadKey::SPECIAL change can be applied to X11 as well.

Unstuck part should be an issue only for the same NumLock case as can happen on Windows (toggling NumLock while numpad key is hold).

@bruvzg
Copy link
Member

bruvzg commented Feb 11, 2025

Also, can something else except the same numpad scenario still get stuck on Wayland, à getting stuck should be already solved, so I'm not sure if unstucking is necessary.

@bruvzg
Copy link
Member

bruvzg commented Feb 11, 2025

Retested it again with both X11 and Wayland:

  • is_sym_numpad& Key::SPECIAL works fine on X11.
  • I was able to get only aforementioned numpad scenario to get stuck (on both Wayland and X11), so moving it to input is probably a better way to do it, since it's not fixing anything else.

@Riteo
Copy link
Contributor Author

Riteo commented Feb 11, 2025

Also, can something else except the same numpad scenario still get stuck on Wayland, à getting stuck should be already solved, so I'm not sure if unstucking is necessary.

It can. It's not only the keypad, it's any key that can map to both a modifier and a regular character. This means that custom layouts (think fancy split ergo keyboards) can trigger the issue.

@bruvzg
Copy link
Member

bruvzg commented Feb 11, 2025

I guess it can also be an issue if you switch layout while holding the key.

@bruvzg
Copy link
Member

bruvzg commented Feb 11, 2025

I'm not sure if we should track pressed state of non-physical keys as is, and was thinking about something like this - bruvzg@d569214 (keep pressed state only based on physical key, and keycode/label only in association with it, completely untested).

@Riteo
Copy link
Contributor Author

Riteo commented Feb 11, 2025

@bruvzg well, we're tracking keys by their raw XKB keycode here, which is lossless compared to physical keys. Honestly I think that it's still a correctness issue to not report a reliable pair of press-release events for keycodes.

@bruvzg
Copy link
Member

bruvzg commented Feb 11, 2025

I not against this PR, but not sure if the same will be possible to implement on the all platforms (which would be good to do at least on X11 and Windows).

@Riteo
Copy link
Contributor Author

Riteo commented Feb 11, 2025

I don't see why it should not be doable on other platforms too.

@Riteo Riteo force-pushed the modifying-spacetime-itself branch from 1dd4a88 to 54755a2 Compare February 11, 2025 12:15
This fixes once and for all the core issue of different Godot `keycode`s
released from the same raw XKB keycode.

The `InputEventKey` `keycode` value _should_ map to the "unmodified"
key, but unfortunately there's an ambiguity with their encoding for
"special" keys ("delete", "insert", etc.), in witch they ignore their
unicode representation. This means that a key that is special when plain
but a character when modified would never be properly picked up, so we
do indeed change its keycode. As a consequence of this exception, some
Godot keys never receive release events and get "stuck".

This patch adds an extra check through an `HashMap` to "unstuck" keys
that changed while having the same keycode.

I also could not resist simplifying a bit the regular key event
generation method but this makes things more consistent and predictable
IMO.
@Riteo
Copy link
Contributor Author

Riteo commented Feb 11, 2025

Do we need to do something similar for X11?

Btw, @akien-mga, this should definitely be implemented for X11 too (and probably even other platforms), I just did it for Wayland because it's what I'm comfortable with and I was not sure if this approach would've been the final one in the first place.

Worst case we can move things in core if that ends up being duplicated ig. I mean, this does not break compat so I think we can be quite chill overall as long as there's consensus.

@akien-mga akien-mga merged commit a8a1009 into godotengine:master Feb 11, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

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