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

Use SDL_Keycodes instead of SDL_Scancodes to map the keys #5774

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MatthieuHernandez
Copy link

This my first Pull Request, so tell me if something is missing or doesn't fit the contributing guidelines.
I submit a patch that fix the issue #5767 that I reported last week.
I changed the mapping to make the shortcut keys, like Ctrl + Z, also works on other type of keyboard than US.
I added a small unit test to check that Key.NonUSBackSlash still working.

I tested with Visual-Test that all keys is working correctly.
⚠️ I only tested on Windows10 with a french keyboard with AZERTY and QWERTY layout.
It would be safer to have a few people testing on Linux, Mac or other keyboards to make sure everything is working properly and that there is no regression.

Hoping that this PR will help.

@bdach
Copy link
Collaborator

bdach commented May 7, 2023

Please read the description of #3950 and check that this PR does not regress what that one was intended to fix.

@MatthieuHernandez
Copy link
Author

From what I just tested, there doesn't seem to be any regression from #3950. (I didn't test on German QWERTZ)
image

I compared the logs in the console and it produces the same result between my Pull Request and the master.
Everything seems to be okay 👍

@bdach
Copy link
Collaborator

bdach commented May 7, 2023

I just tested myself, and while yes, text input does seem to be working correctly still (likely because we're now exclusively using IME code paths from SDL for that), there is this part of the description of the aforementioned PR:

In the case of those special keyboard layouts, pressing the special diacritics keys would emit a Keycode with a value correspondent to the Unicode code point of the character pressed by the user, which has no osuTK correspondent, therefore getting converted to Key.Unknown and as such discarded.

This is still the case:

if (key == Key.Unknown)
{
Logger.Log($"Unknown SDL key: {evtKey.keysym.scancode}, {evtKey.keysym.sym}");
return;
}

As such, if you perform the following steps:

  • Clone the game repository locally, to a directory next to the osu-framework directory
  • Run the UseLocalFramework.sh script
  • Compile and run the game
  • Open the settings overlay, go to the keybindings section
  • Attempt to set any keybinding to one of the following keyboard keys (using the French layout here, but the same will happen for German, etc.):

image

you should see that the keys highlighted in orange on the image above can no longer be assigned to any keybinding.

I've tested this (mostly, because I don't physically have some of these keys) on my machine just now.

@MatthieuHernandez
Copy link
Author

Okay, I was able to reproduce the problem on osu! I'll look for a way to fix it.

Replace the switch case with a HashSet that contains InputKey, Key,
SDL_Keycode and SDL_Scancode. A key is assigned to an SDL_Keycode and
if the key is UNKNOWN then it is assigned to an SDL_Scancode as before.
This will allow the shortcuts to be used normally on non-US keyboards.
@MatthieuHernandez
Copy link
Author

It's ok, it took me a while but I finally found a satisfying solution that works perfectly (according to the tests I did).
My solution is to use SDL_Keycode for the standard keys and SDL_Scancode for the specific keys. I also replaced the switch case with Dictionary for more readability.

In addition to my previous tests, I tested with the Windows virtual keyboard to test different keyboards: US, French, Genman, Japanese and Arabic. I also checked that the NumLock still works.

@MatthieuHernandez MatthieuHernandez marked this pull request as ready for review May 10, 2023 14:08
@Susko3
Copy link
Member

Susko3 commented May 14, 2023

Sorry, I should have stopped you earlier, but this is not the correct direction. I already have a pull in the works that does this in a different way.

There needs to be an InputKey / KeyCombination-level distinction between scancodes and keycodes. Why? Because some actions don't really care about what the key is, they only care about where the key is. For example, for osu! and Z, X, it's not really about the Z and X keys, but instead it's about the two keys at the bottom-left of the keyboard. By using keycodes for "standard" keys (including Z and X) you completely break this scenarion on keyboard layouts where Z and/or X are in a different position.

But for other actions, like undo, people expect them to use keycodes like you did here. Both cases need to be supported at once, so this complicates things.

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

Successfully merging this pull request may close these issues.

3 participants