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

Added support for left- and right- versions of modifier keys. #43888

Conversation

EricEzaM
Copy link
Contributor

@EricEzaM EricEzaM commented Nov 26, 2020

Adds the ability to use Left/Right Shift, Control, Alt and Meta as key bindings.
These keys being used as modifiers (e.g. Shift+Ins, Ctrl+T) is unchanged, and either left or right will work.

References used:
https://docs.microsoft.com/en-us/windows/win32/inputdev/virtual-key-codes
https://www.win.tue.nl/~aeb/linux/kbd/scancodes-1.html

Will close issues:
Closes #3347.

@EricEzaM EricEzaM marked this pull request as draft November 26, 2020 14:25
@EricEzaM EricEzaM force-pushed the PR/add-left-right-modifier-distinction branch 2 times, most recently from 3c9caf4 to f35049f Compare November 26, 2020 14:32
@Calinou Calinou added this to the 4.0 milestone Nov 26, 2020
@EricEzaM
Copy link
Contributor Author

@akien-mga @reduz

How do we allow for users to set a key to be "Any Shift/Control/Alt", so it doesn't matter which of left/right is pressed? Do we want this functionality? Sometimes they may just want alt/shift/ctrl to be pressed generically, rather than a specific side.

This might also go for editor shortcuts where we currently use KEY_CONTROL for things like going to definition in the Script editor... this might have to be changed to KEY_LEFT_CONTROL unless there is a way to make KEY_CONTROL represent any control key.

Please also note that since Mac does not differentiate between Left and Right Command, I have kept "Meta" as one key and have not split it into Left/Right variants.

@EricEzaM EricEzaM force-pushed the PR/add-left-right-modifier-distinction branch from 94f2558 to b9bcbfb Compare November 27, 2020 01:17
@EricEzaM
Copy link
Contributor Author

EricEzaM commented Nov 28, 2020

Related: #3347

@EricEzaM
Copy link
Contributor Author

EricEzaM commented Nov 28, 2020

  • I added bool Input::is_modifier_pressed(int p_keycode_mask) where you can pass a mask like KEY_MASK_CMD or KEY_MASK_SHIFT and it will return whether one of the keys that make up that mask is pressed.
  • I converted all code to use the above method. A lot of methods used is_key_pressed(KEY_CONTROL) which is wrong because we want it to act as Command on Apple and Control on others - which this would not do (would be Control on Apple too). Now, with is_modifier_pressed(KEY_MASK_CMD) it will correctly check for Command on Apple and Left/Right Control on Windows/Others.
  • I removed the KEY_COMMAND definition as it was no longer compatible with the new Left/Right definitions (Apple does not distinguish L/R command). Also it was not used anywhere in the codebase, and was also not bound to scripting languages...
  • Unfortunately, it seems now there is no easy cross-platform way to say if Command pressed on Apple, or control pressed on Windows/Other. Edit: see solution 2 points down We could potentially add the code below... but it is a bit misleading.
#ifdef APPLE_STYLE_KEYS
  KEY_COMMAND = KEY_META
#else
  KEY_COMMAND = KEY_LEFT_CONTROL
#endif
  • Another option perhaps... faking left/right command?
#ifdef APPLE_STYLE_KEYS
  KEY_LEFT_COMMAND = KEY_META
  KEY_RIGHT_COMMAND = KEY_META
#else
  KEY_LEFT_COMMAND = KEY_LEFT_CONTROL
  KEY_RIGHT_COMMAND = KEY_RIGHT_CONTROL
#endif
  • Another option... I had the idea which could potentially solve this by abstracting the problem away from the KeyList. The key masks work perfectly fine since we do not discern between left/right for the modifier keys when they are being pressed with another key, in a combination.
    • This is useful, like how I added Input::is_modifier_pressed(KEY_MASK_CMD), but there is still the case of handling InputEventKey: p_key->get_keycode() == KEY_LEFT_CONTROL || p_key->get_keycode() KEY_RIGHT_CONTROL, which is still used quite a bit in the codebase. Writing that code is very long and unneccesary, and also requires an #ifdef APPLE_STYLE_KEYS for compatibility with Apple. The #ifdefs cannot be done in scripting, so another solution is required for proper cross platform compatibility.
    • What if we added a method to InputEventKey which would check if the key pressed is a modifier key specifically. bool InputEventKey::is_keycode_modifier(int p_keycode_mask), and it would use the same logic as the Input version. This way, there is an easy cross-platform way to know if "Command/Control" is pressed, since you could just do p_key->is_keycode_modifier(KEY_MASK_CMD). This would test if the keycode of the event is KEY_META on Apple, or KEY_LEFT_CONTROL || KEY_RIGHT_CONTROL on Windows/Others.

@EricEzaM EricEzaM force-pushed the PR/add-left-right-modifier-distinction branch 2 times, most recently from cf00199 to be1e658 Compare November 28, 2020 02:23
@EricEzaM
Copy link
Contributor Author

EricEzaM commented Nov 28, 2020

Ok, I implemented my idea in the most recent commit. I actually quite like it and think it is a pretty clean way of checking "is any shift key pressed", etc. Interested in feedback.

Look thru the commits one at a time and things should make more sense :)

It also removes a few #ifdef APPLE_STYLE_KEYS which are uses here and there in the source code, so that is a good thing too.

@EricEzaM EricEzaM force-pushed the PR/add-left-right-modifier-distinction branch 4 times, most recently from e00505b to 1325962 Compare November 30, 2020 12:04
@EricEzaM EricEzaM changed the title Added support for left- and -right versions of modifier keys. Added support for left- and right- versions of modifier keys. Dec 3, 2020
@EricEzaM EricEzaM marked this pull request as ready for review December 7, 2020 13:56
@EricEzaM EricEzaM force-pushed the PR/add-left-right-modifier-distinction branch from 1325962 to 4acb02c Compare December 7, 2020 14:01
@EricEzaM
Copy link
Contributor Author

EricEzaM commented Dec 7, 2020

Took it out of draft and rebased... I can't test on Mac/Linux/others so if someone could vet the changes I made there it would be much appreciated.

@EricEzaM EricEzaM force-pushed the PR/add-left-right-modifier-distinction branch 2 times, most recently from b1392bd to 3398607 Compare December 8, 2020 09:39
Adds the ability to use Left/Right Shift, Control, Alt and Meta as key bindings.
These keys being used as modifiers (e.g. Shift+Ins, Ctrl+T) is unchanged, and either left or right will work.

References used:
https://docs.microsoft.com/en-us/windows/win32/inputdev/virtual-key-codes
https://www.win.tue.nl/~aeb/linux/kbd/scancodes-1.html
@reduz
Copy link
Member

reduz commented Jul 12, 2022

We discussed in PR review meeting that there does not seem to be a lot of demand for this and the way things work appear to be more accessible for the majority of users. Even if you used this for having multiple inputs (players) in a single keyboard, a lot of keyboards don't really work well when many of these keys are pressed. It would be nice to understand better which kind of use cases make this a necessary feature.

@EricEzaM
Copy link
Contributor Author

EricEzaM commented Jul 13, 2022

@reduz There is some decent support in the below issues for this functionality.

#3347
godotengine/godot-proposals#1387

I don't mind either way, at the time it was something that had been an outstanding issue for a while, and was one possible way of resolving #6851

I am not sure if @bruvzg was in the meeting and if he has any opinions on it (as he resolved the above issue in one of his PRs)

If this is decided against then the related issues and proposal should also be closed then I assume.

@Kehom
Copy link

Kehom commented Jul 19, 2022

It would be nice to understand better which kind of use cases make this a necessary feature.

@reduz well, I don't know if this would qualify as "necessary feature", but pinball games typically use right and left shift keys to activate the respective flippers. Sometimes right and left Ctrl are then used to perform the "nudge" to simulate a "hit" in the machine from the respective side. While it would be possible to use different keys, I don't think it would be ideal to shift away from something that is somewhat "established".

@reduz
Copy link
Member

reduz commented Jul 29, 2022

@Kehom I know, the problem is the added complexity this creates.

@EricEzaM Maybe one solution would be that we can have both something like
KEY_SHIFT, KEY_LEFT_SHIFT and KET_RIGHT_SHIFT and when you press left shift, you get both of the key events sent.

It would probably need an option in the input action settings (editor shortcut config likely fine ignoring this) when recording them that you want to discern between using the modifier side or not. Like "[x] sided modifiers"

@YuriSizov
Copy link
Contributor

@bruvzg Can you be a tie-breaker and weigh in on the proposed options?

@bruvzg
Copy link
Member

bruvzg commented Sep 8, 2022

Not against it, but I do not think it's many use cases. Not every keyboard have both right and left key options, so it's definitly should have a way to map any of laft/right keys not just a specific one.

I would probably add an extra is_left and is_right properties to the Key event, instead of adding new modifier masks for each key. These properties can be normally ignored when matching actions, and checked manually if needed.

@Riteo
Copy link
Contributor

Riteo commented Sep 8, 2022

@bruvzg that sounds like it could cause some confusion to some people as they might expect that it'd denote the physical position of the key (for example f is left and j is right).

@bruvzg
Copy link
Member

bruvzg commented Sep 10, 2022

that sounds like it could cause some confusion to some people as they might expect that it'd denote the physical position of the key (for example f is left and j is right).

I guess nothing prevents us from actually setting l/r flags for all keys it this way (not sure why anyone would want it, bit it won't conflict with anything).

@EricEzaM
Copy link
Contributor Author

EricEzaM commented Oct 3, 2022

At this stage I will not be continuing with this pull request. Anyone else is welcome to pick up the scraps if they are motivated to do so.

@SchwingSK
Copy link

SchwingSK commented Oct 31, 2022

A bit late to the party, but @reduz another use case is the arcade DIY crowd: a very popular way to hook arcade sticks & buttons is usin IPAC-2 & IPAC-4 which are devices that simulate a keyboard. The default mapping relies on left & right shift & control keys (see https://www.ultimarc.com/control-interfaces/i-pacs/i-pac2/ ).
I'm making a small game that's friendly to these types of setup (and also two players on the same keyboard) and was quite surprised that such a basic (in my mind ;) ) feature was missing in Godot.
I know that it's very niche, and the code adds complexity, but is there any way that this would be re-considered? At least for Godot 4?

@Calinou
Copy link
Member

Calinou commented Oct 31, 2022

As reduz mentioned above, I think it needs to be reimplemented this way to be considered:

Maybe one solution would be that we can have both something like
KEY_SHIFT, KEY_LEFT_SHIFT and KET_RIGHT_SHIFT and when you press left shift, you get both of the key events sent.

4.0 is in feature freeze, so such a change will probably have to wait until 4.1 at least to be merged.

@SchwingSK
Copy link

This solutions suits me, as I don't need the modifier status, and even if I needed it, I could implement it in my project, This means opening a new pull request, simpler than this one, I suppose ?

@Riteo
Copy link
Contributor

Riteo commented Oct 31, 2022

@SchwingSK you could, but you'd have to wait quite some time for it to be merged.

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.

Add Modifier keys left/right support
9 participants