Better shortcut key support #1646

andrew-oc · 2023-02-17T11:51:54Z

Description

Existing shortcut key support falls a bit a short of being useful:

When shortcutKeysConsumeInput is off, there is basically no shortcut key support at all. Shift+B will trigger bindings to just B and the application has no reasonable way of determining that a different key combination was pressed
When all action maps are enabled (anecdotally the default for most users) event consumption causes confusion due to unintuitive behaviour. A Vector2 binding to WASD would consume the input of a binding to Ctrl+W when Ctrl+W is pressed.
The application has no way to override the default event consumption

Changes made

Composite bindings are now sorted by a priority system that comes from the bindings themselves, rather than as a count of the number of binding parts. In practice this means that for example a Vector2Composite binding does not have a higher priority than a one or two modifier composite binding i.e. pressing W to trigger a WASD action would not by default prevent a binding to Ctrl-W from performing.
Added several APIs to allow application logic to control event handling/consumption.
- CallbackContext.eventHandled can be used to check if a higher priority binding has already handled the current event
- CallbackContext.HandleEvent() allows any 'performed' callback to set an input event as handled
- InputAction.wasPerformedThisFrame(bool checkConflictingActions) allows polling code to determine if a higher priority binding has performed
- InputBindingComposite.handleInputEvents allows applications to decide which composite bindings will handle events

Notes

This PR does not change the default behaviour of code that has InputSettings.shortcutKeysConsumeInput turned off i.e. no bindings are blocked due to event consumption. However it is possible that due to the differences in how controls are sorted within the state monitor code that some bindings will now be triggered in a different order than before.

Future work

We can build two interesting features on top of this PR:

It would be possible to allow users to set the priority of bindings on an instance by instance basis, and hence fully control the order that bindings perform in
The action grouping that this PR comes with would allow us to detect conflicting bindings that we could display in the input action asset editor as warnings or even detect at runtime for players rebinding controls.

Checklist

Before review:

Changelog entry added.
- Explains the change in Changed, Fixed, Added sections.
- For API change contains an example snippet and/or migration example.
- FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
- FogBugz is marked as "Resolved" with next release version correctly set.
Tests added/changed, if applicable.
- Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
- Performance tests.
- Integration tests.
Docs for new/changed API's.
- Xmldoc cross references are set correctly.
- Added explanation how the API works.
- Usage code examples added.
- The manual is updated, if needed.

Also made changes for 2019.4

jfreire-unity

Added questions for things that weren't clear when I look through it them. But I believe I need a walkthrough from you @andrew-oc to understand better the work involved.

I also noticed some formatting inconsistencies for you to look at. Don't know if it's the GitHub "diff" viewer.

Assets/Tests/InputSystem/CoreTests_Actions.cs

Packages/com.unity.inputsystem/InputSystem/Actions/InputAction.cs

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

Packages/com.unity.inputsystem/InputSystem/InputSettings.cs

Packages/com.unity.inputsystem/Documentation~/ActionBindings.md

jfreire-unity

Overall, the work looks great and gives more control to developers. Great work 👍

Regarding the PR, I would like to suggest that we have more granularity in the commits OR smaller PRs to have a better understanding of the changes added. I
It was a bit hard for me to follow everything added in just one commit, and some parts are still clueless to me. But you can take this with "a grain of salt" as I'm still very new to the codebase.

lyndon-unity

There are a couple of suggested changes and I'd like to discuss with design/product before landing on develop as the preference would be to expand further to see what could be done to handle the common case [CTRL/SHIFT/ALT]+KEY overriding KEY (without impacting WASD in two places).

lyndon-unity · 2023-02-23T14:22:17Z

Assets/Tests/InputSystem/CoreTests_Actions.cs

        var keyboard = InputSystem.AddDevice<Keyboard>();

-        var map1 = new InputActionMap("map1");


Why are we removeing this WASD test case that we've seen from forums (ie the UI / game use case)

It isn't really relevant anymore because the composites in that test don't by default handle input events, so they never get consumed. Those paths are tested by other tests though.

lyndon-unity · 2023-02-23T14:25:49Z

Assets/Tests/InputSystem/CoreTests_Actions.cs

+        var map = new InputActionMap();
+
+        // action with bindings to Ctrl+Shift+C and C should appear twice in the 'C' group
+        var action0 = map.AddAction("action1", binding: "<Keyboard>/c");


action0 variable and action 1 name is a bit confusing for me to read can we be consistent. E.g rename variable to match strings

lyndon-unity · 2023-02-23T14:29:48Z

Packages/com.unity.inputsystem/CHANGELOG.md

+  * Added several APIs to allow application logic to control event handling/consumption.
+    * CallbackContext.eventHandled can be used to check if a higher priority binding has already handled the current event
+    * CallbackContext.HandleEvent() allows any 'performed' callback to set an input event as handled
+    * InputAction.wasPerformedThisFrame(bool checkConflictingActions) allows polling code to determine if a higher priority binding has performed


I'm not a fan of the bool here. I'd like to see that changing to an enum

InputAction.wasPerformedThisFrame(true) isn't very clear what that does

Done. Introduced InputAction.EventHandledBehaviour enum.

lyndon-unity · 2023-02-23T14:42:50Z

Packages/com.unity.inputsystem/Documentation~/KnownLimitations.md

@@ -2,6 +2,10 @@

 The following is a list of known limitations that the Input System currently has.

+## Actions
+
+* Input event consumption i.e. the ability to prevent a binding to "A" from triggering when a binding to "SHIFT+A" has already triggered, does not reliably work across multiple Input Action Assets, or multiple `InputActionMap` instances created through code. This is due to the way the runtime state for control grouping and priority is calculated in isolation for each Input Action Asset or standalone `InputActionMap`.


Is the limitation specific to multiple action maps created in code (i.e. multiple maps in an asset would work)

Yes. Multiple action maps in the same input action asset work as intended. This also extends to singleton input actions created in code, as internally they get their own InputActionMap instances.

Multiples maps in an asset would work. The limitation is only for maps that don't belong to an asset.

lyndon-unity · 2023-02-23T14:46:36Z

Packages/com.unity.inputsystem/InputSystem/Actions/InputAction.cs

-        public unsafe bool WasPerformedThisFrame()
+        /// <seealso cref="InputSettings.shortcutKeysConsumeInput"/>
+        /// <seealso cref="InputBindingComposite.handleInputEvents"/>
+        public unsafe bool WasPerformedThisFrame(bool checkConflictingActions)


As mentioned earlier I'd prefer this to be an enum for the parameter to make it clearer in usage

andrew-oc · 2023-02-28T13:53:38Z

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

                }
-                controlGroupingAndComplexity[i * 2 + 1] = (ushort)complexity;
+                controlGroupingAndComplexity[i].complexity = (ushort)complexity;


lyndon-unity · 2023-02-23T14:55:09Z

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs


                    ++currentGroup;
                }
+
+                // add the action that the current binding belongs to to the relevant group
+                if (controlGroupingAndComplexity[i].group >= m_ActionGroups.Length)


Wondering if we can move the resize outside the for loop so we only resize once

Done. It means two loops over all the controls, but is probably still a win for larger input action assets, and the code is neater.

lyndon-unity · 2023-02-23T14:56:22Z

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

+                    controlGroupingAndComplexity[i].group);
+                #endif
+                if (m_ActionGroups[controlGroupingAndComplexity[i].group].isCreated == false)
+                    grouping = new ActionGroup(5);


What's this 5 - perhaps we can make this a constant with a name

lyndon-unity · 2023-02-23T15:16:41Z

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

+
+            public ActionGroup(int capacity)
+            {
+                actionIndicies = (int*)UnsafeUtility.Malloc(capacity, 8, Allocator.Persistent);


Could we add a named variable / static to make this 8 a bit clearer that its alignment (and why 8 chosen)

Went one better. 8 was in fact an incorrect alignment!

lyndon-unity · 2023-02-23T15:17:30Z

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

+            public void AddActionIndex(int actionIndex)
+            {
+                // return immediately if the actionIndex already exists in this group
+                for (var i = 0; i < actionCount; i++)


Could call the contains function from below ?

Done, thanks

andrew-oc · 2023-02-28T15:03:49Z

There are a couple of suggested changes and I'd like to discuss with design/product before landing on develop as the preference would be to expand further to see what could be done to handle the common case [CTRL/SHIFT/ALT]+KEY overriding KEY (without impacting WASD in two places).

This PR changes things so that WASD is never impacted anywhere. Event consumption is still off by default, so no inputs will go missing, while still allowing users to check for shortcut keys, but even if event consumption is turned on, Vector2 controls (and 1DAxis) don't handle events by default, so they still won't be impacted. The only time you'll see a WASD getting blocked (out of the box) is when event consumption is on and a modifier is used with W A S or D keys.

lyndon-unity

Thanks for the updates.

I still have a query about a resize call in a nested for loop and a bounds check.

lyndon-unity · 2023-02-28T15:44:40Z

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

@@ -4155,6 +4156,11 @@ public void Dispose()

                actionIndicies = null;
            }
+
+            private static void* Allocate(int length)


This is a nice tidy up and using AlignOf

lyndon-unity · 2023-02-28T15:44:48Z

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

-                    if (actionIndicies[i] == actionIndex)
-                        return;
-                }
+                if (Contains(actionIndex))


lyndon-unity · 2023-02-28T15:46:34Z

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

@@ -246,9 +249,10 @@ private void ComputeControlGroupingIfNecessary()



Wondering if this Resize could be moved out of the nested for loop, e.g. by. spinning round in advance to calculate max size.

Also wondering if the actionCountInGroup index overflow the tempActionGroupIndexes above.
Perhaps splitting the loop could allow a sanity check (or perhaps a sanity check could be added without splitting the loop)

unity-cla-assistant · 2023-04-05T14:59:28Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

andrew-oc added 6 commits February 17, 2023 12:31

Better shortcut key support

f0d8a65

Formatting

4b999c9

Some fixes for 2019.4 lack of ArrayElementAsRef method

4964588

Updated docs

67be686

Also made changes for 2019.4

Changelog

1ca087e

Formatting

838ab96

andrew-oc requested review from jamesmcgill, jimon, lyndon-unity and jfreire-unity February 17, 2023 11:52

jfreire-unity requested changes Feb 21, 2023

View reviewed changes

jfreire-unity reviewed Feb 21, 2023

View reviewed changes

Packages/com.unity.inputsystem/Documentation~/ActionBindings.md Outdated Show resolved Hide resolved

jfreire-unity reviewed Feb 21, 2023

View reviewed changes

Packages/com.unity.inputsystem/Documentation~/ActionBindings.md Show resolved Hide resolved

jfreire-unity reviewed Feb 21, 2023

View reviewed changes

Packages/com.unity.inputsystem/Documentation~/ActionBindings.md Show resolved Hide resolved

jfreire-unity reviewed Feb 21, 2023

View reviewed changes

andrew-oc and others added 2 commits February 23, 2023 13:18

Implement review changes

32c82ce

Merge branch 'develop' into better-shortcut-key-support

4b04751

andrew-oc requested a review from jfreire-unity February 23, 2023 12:21

lyndon-unity requested changes Feb 23, 2023

View reviewed changes

Addressing review comments

8a7a907

andrew-oc requested a review from lyndon-unity February 28, 2023 15:10

lyndon-unity reviewed Feb 28, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better shortcut key support #1646

Better shortcut key support #1646

andrew-oc commented Feb 17, 2023

jfreire-unity left a comment

jfreire-unity left a comment

lyndon-unity left a comment

lyndon-unity Feb 23, 2023

andrew-oc Feb 28, 2023

lyndon-unity Feb 23, 2023

andrew-oc Feb 28, 2023

lyndon-unity Feb 23, 2023

andrew-oc Feb 28, 2023

lyndon-unity Feb 23, 2023

andrew-oc Feb 23, 2023

andrew-oc Feb 28, 2023

lyndon-unity Feb 23, 2023

andrew-oc Feb 28, 2023

This comment was marked as resolved.

andrew-oc Feb 28, 2023

lyndon-unity Feb 23, 2023

andrew-oc Feb 28, 2023

lyndon-unity Feb 23, 2023

andrew-oc Feb 28, 2023

lyndon-unity Feb 23, 2023

andrew-oc Feb 28, 2023 •

edited

lyndon-unity Feb 23, 2023

andrew-oc Feb 28, 2023

andrew-oc commented Feb 28, 2023 •

edited

lyndon-unity left a comment

lyndon-unity Feb 28, 2023

lyndon-unity Feb 28, 2023

lyndon-unity Feb 28, 2023

lyndon-unity Feb 28, 2023

unity-cla-assistant commented Apr 5, 2023

		var keyboard = InputSystem.AddDevice<Keyboard>();

		var map1 = new InputActionMap("map1");

		@@ -246,9 +249,10 @@ private void ComputeControlGroupingIfNecessary()

Better shortcut key support #1646

Are you sure you want to change the base?

Better shortcut key support #1646

Conversation

andrew-oc commented Feb 17, 2023

Description

Changes made

Notes

Future work

Checklist

jfreire-unity left a comment

Choose a reason for hiding this comment

jfreire-unity left a comment

Choose a reason for hiding this comment

lyndon-unity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-oc Feb 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-oc commented Feb 28, 2023 • edited

lyndon-unity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unity-cla-assistant commented Apr 5, 2023

andrew-oc Feb 28, 2023 •

edited

andrew-oc commented Feb 28, 2023 •

edited