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

Fixes for macOS scancode implementation #2243

Merged
merged 20 commits into from Dec 19, 2022

Conversation

kimci86
Copy link
Contributor

@kimci86 kimci86 commented Oct 12, 2022

Description

Here are some fixes for the scancode feature implementation for macOS. This PR is related to #1235.

This is a work is progress. I hope to make some more improvements soon but I open this draft PR already so that other contributors do not repeat the same work and can look for other improvements from there.

Some issues remain to be fixed:

  • Some keys are not detected and/or trigger no events (caps lock and a few others)
  • Shift key being pressed or not has an impact of key events

Edit: fixed.

How to test this PR?

Running SFML-Input on macOS

@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Oct 12, 2022
@eXpl0it3r eXpl0it3r added this to the 2.6 milestone Oct 12, 2022
@eXpl0it3r eXpl0it3r moved this from Discussion to WIP in SFML 2.6.0 Oct 12, 2022
@JonnyPtn
Copy link
Contributor

JonnyPtn commented Oct 13, 2022

I looked into the caps lock issue a while ago and I'm not sure there's a perfect solution for it based on what we get from the OS. I looked at what others do and tried some things myself and only found these options:

  • Use flagsChanged checking for the caps lock modifier almost works, but tracks the actual state of caps lock not the state of the physical button
  • Manually track the state of the key by polling the hardware (a bit unreliable, inconsistent, and also requires extra user permissions)

The only other keys I can see not working are:

  • §/± (cocoa key code 0xA0) so just need to decide what key that maps to and add it (EDIT: apparently should be Grave and the key which is currently Grave should be NonUsBackslash)
  • fn key, which also seems to use flagsChanged so we can collect it there along with the other modifiers (it'/s NSEventModifierFlagFunction)

What's the issue you're seeing with the shift key? As far as I can tell the scan codes are always the same whether shift is pressed or not

@kimci86
Copy link
Contributor Author

kimci86 commented Oct 17, 2022

What's the issue you're seeing with the shift key? As far as I can tell the scan codes are always the same whether shift is pressed or not

The issue is that, when pressing shift, event.key.code values are different (bad). EDIT: now fixed.
I confirm event.key.scancode values are the same with or without shift (good).

@kimci86
Copy link
Contributor Author

kimci86 commented Nov 11, 2022

I think I fixed all I could fix.

Remaining issues:

  1. Caps lock generates no events
  2. §/± (key above tab) generates no events (EDIT: fixed)
  3. `/~ (key next to left shift) generates event with Scan::Grave code (EDIT: fixed)

Regarding issues 2. and 3., fixing it would require detecting if keyboard uses ANSI or ISO layout because virtual key code 0x32 then has a different meaning. See Figure 2-10 in Macintosh Toolbox Essentials.

@kimci86 kimci86 marked this pull request as ready for review November 11, 2022 22:02
@kimci86
Copy link
Contributor Author

kimci86 commented Nov 11, 2022

iOS build failure should vanish as soon as the feature branch is rebased on 2.6.x thanks to #2263

@eXpl0it3r
Copy link
Member

iOS build failure should vanish as soon as the feature branch is rebased on 2.6.x thanks to #2263

I just rebased 2.6.x

@eXpl0it3r eXpl0it3r moved this from WIP to Review & Testing in SFML 2.6.0 Nov 15, 2022
@JonnyPtn
Copy link
Contributor

For caps lock if we're happy with it representing the lock status then we can use the method I mentioned above

For the other two keys, does the virtual key code matter for scan codes, where we're representing the physical location of the key?

We discussed it briefly on discord a while ago and the conclusion was that the one above tab should be Grave and the one next to left shift should be non-US backslash?

@kimci86
Copy link
Contributor Author

kimci86 commented Nov 16, 2022

For caps lock if we're happy with it representing the lock status then we can use the method I mentioned above

I would not say I would be happy with that, but that's maybe better than nothing. I don't know.

For the other two keys, does the virtual key code matter for scan codes, where we're representing the physical location of the key?

As far as I understand, virtual key code is Apple's equivalent of SFML's scancode. It represents a physical location of a key. But the location of virtual key code 0x32 is above tab for ANSI keyboards and next to left shift for ISO keyboards.

Digging a little further, it should be doable to detect which type of keyboard is used and map the correct scancode accordingly. This is what chromium does for example.
I can give it a try next weekend. Real-time polling might need adjustments too.

@kimci86
Copy link
Contributor Author

kimci86 commented Nov 19, 2022

I think I fixed the issue related to §/± (key above tab) and `/~ (key next to left shift).
It works well with an ISO keyboard. I do not have an ANSI keyboard to test fully though.

@kimci86
Copy link
Contributor Author

kimci86 commented Nov 19, 2022

With the help of @ChrisThrasher, we could check the key above tab is correctly mapped to Scancode::Grave on ANSI keyboard.

@kimci86
Copy link
Contributor Author

kimci86 commented Dec 16, 2022

Should I implement events for caps lock using the lock status? It would get one out of every two events for that key, so it would be half broken, but maybe better than nothing?
Besides caps lock, this PR is ready in my humble opinion.

@JonnyPtn
Copy link
Contributor

Yeah I'd say it's better than nothing, and also agree this is otherwise ready

@kimci86
Copy link
Contributor Author

kimci86 commented Dec 17, 2022

Here it is! Caps lock now generates a key pressed / released event when the corresponding modifier state changes.

Rebasing should solve CI failures thanks to fa2e61b.

SFML 2.6.0 automation moved this from Review & Testing to Ready Dec 17, 2022
@eXpl0it3r eXpl0it3r merged commit 49f190a into SFML:feature/scancode Dec 19, 2022
SFML 2.6.0 automation moved this from Ready to Done Dec 19, 2022
@eXpl0it3r
Copy link
Member

eXpl0it3r commented Dec 19, 2022

Thanks A MILLION TONS for this! I personally know how much time it takes to dive into stuff like that. ❤️

I'll clean up the commits a bit on the feature branch.

@ChrisThrasher
Copy link
Member

Thank you @kimci86 for all you've done! I think we're all really excited to see this ship.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants