FIX: some actions not being triggered when other duplicate control bindings exist (ISXB-254). (#1600)

jamesmcgill · web-flow · commit aff7a19fe46c · 2022-11-01T10:39:32.000+01:00
* CHANGE: shortcut support disabled by default

* add test and fix shortcut specific tests

* review changes. fix changelog entry

* review comments: add shortcut feature toggle to the Project Settings

* review comments: modify changelog

* formatting

* review comments: improve descriptions

* remove deprecation warning

* more specific property name
diff --git a/Assets/Tests/InputSystem/CoreTests_Actions.cs b/Assets/Tests/InputSystem/CoreTests_Actions.cs
@@ -30,14 +30,50 @@
 // in terms of complexity.
 partial class CoreTests
 {
+    [Test]
+    [Category("Actions")]
+    public void Actions_WhenShortcutsDisabled_AllConflictingActionsTrigger()
+    {
+        var keyboard = InputSystem.AddDevice<Keyboard>();
+
+        var map1 = new InputActionMap("map1");
+        var action1 = map1.AddAction(name: "action1");
+        action1.AddCompositeBinding("2DVector")
+            .With("Up", "<Keyboard>/w")
+            .With("Down", "<Keyboard>/s")
+            .With("Left", "<Keyboard>/a")
+            .With("Right", "<Keyboard>/d");
+
+        var map2 = new InputActionMap("map2");
+        var action2 = map2.AddAction(name: "action2");
+        action2.AddCompositeBinding("2DVector")
+            .With("Up", "<Keyboard>/w")
+            .With("Down", "<Keyboard>/s")
+            .With("Left", "<Keyboard>/a")
+            .With("Right", "<Keyboard>/d");
+        var action3 = map2.AddAction(name: "action3", binding: "<Keyboard>/w");
+
+        map1.Enable();
+        map2.Enable();
+
+        Press(keyboard.wKey);
+
+        // All Actions were triggered
+        Assert.That(action1.WasPerformedThisFrame());
+        Assert.That(action2.WasPerformedThisFrame());
+        Assert.That(action3.WasPerformedThisFrame());
+    }
+
     // Premise: Binding the same control multiple times in different ways from multiple concurrently active
     //          actions should result in the input system figuring out which *one* action gets to act on the input.
     [Test]
     [Category("Actions")]
     [TestCase(true)]
     [TestCase(false)]
-    public void Actions_CanConsumeInput(bool legacyComposites)
+    public void Actions_WhenShortcutsEnabled_CanConsumeInput(bool legacyComposites)
     {
+        InputSystem.settings.SetInternalFeatureFlag(InputFeatureNames.kDisableShortcutSupport, false);
+
         var keyboard = InputSystem.AddDevice<Keyboard>();
 
         var map = new InputActionMap();
@@ -124,13 +160,11 @@ public void Actions_CanConsumeInput(bool legacyComposites)
         Assert.That(!action3.WasPerformedThisFrame());
     }
 
-    // For now, maintain a kill switch for the new behavior for users to have an out where the
-    // the behavior is simply breaking their project.
     [Test]
     [Category("Actions")]
-    public void Actions_CanDisableShortcutSupport()
+    public void Actions_ShortcutSupportDisabledByDefault()
     {
-        InputSystem.settings.SetInternalFeatureFlag(InputFeatureNames.kDisableShortcutSupport, true);
+        Assert.That(InputSystem.settings.IsFeatureEnabled(InputFeatureNames.kDisableShortcutSupport), Is.True);
 
         var keyboard = InputSystem.AddDevice<Keyboard>();
 
@@ -216,8 +250,10 @@ public void Actions_CanBindMultipleShortcutSequencesBasedOnSameModifiers()
     [TestCase("leftShift", "leftAlt", "space", true)]
     [TestCase("leftShift", null, "space", false)]
     [TestCase("leftShift", "leftAlt", "space", false)]
-    public void Actions_PressingShortcutSequenceInWrongOrder_DoesNotTriggerShortcut(string modifier1, string modifier2, string binding, bool legacyComposites)
+    public void Actions_WhenShortcutsEnabled_PressingShortcutSequenceInWrongOrder_DoesNotTriggerShortcut(string modifier1, string modifier2, string binding, bool legacyComposites)
     {
+        InputSystem.settings.SetInternalFeatureFlag(InputFeatureNames.kDisableShortcutSupport, false);
+
         var keyboard = InputSystem.AddDevice<Keyboard>();
 
         var action = new InputAction();
@@ -292,8 +328,10 @@ public void Actions_PressingShortcutSequenceInWrongOrder_DoesNotTriggerShortcut_
 
     [Test]
     [Category("Actions")]
-    public void Actions_CanHaveShortcutsWithButtonsUsingInitialStateChecks()
+    public void Actions_WhenShortcutsAreEnabled_CanHaveShortcutsWithButtonsUsingInitialStateChecks()
     {
+        InputSystem.settings.SetInternalFeatureFlag(InputFeatureNames.kDisableShortcutSupport, false);
+
         var keyboard = InputSystem.AddDevice<Keyboard>();
 
         var map = new InputActionMap();
@@ -3052,7 +3090,7 @@ public void Actions_CanRecordActions_AndReadValueAsObject()
             action2.Disable();
             Set(gamepad.leftTrigger, 0.234f);
 
-            Assert.That(trace, Performed(action2, value: -0.123f).AndThen(Performed(action1, value: 0.123f)));
+            Assert.That(trace, Performed(action1, value: 0.123f).AndThen(Performed(action2, value: -0.123f)));
         }
     }
 
@@ -10158,6 +10196,68 @@ public void Actions_WithMultipleCompositeBindings_WithoutEvaluateMagnitude_Works
         Assert.That(values[0].Position, Is.EqualTo(new Vector2(1, 1)));
     }
 
+    // FIX: This test is currently checking if shortcut support is enabled by testing that the unwanted behaviour exists.
+    // This test should be repurposed once that behaviour is fixed.
+    [Test]
+    [Category("Actions")]
+    [TestCase(true)]
+    [TestCase(false)]
+    public void Actions_ImprovedShortcutSupport_ConsumesWASD(bool shortcutsEnabled)
+    {
+        InputSystem.settings.SetInternalFeatureFlag(InputFeatureNames.kDisableShortcutSupport, !shortcutsEnabled);
+
+        var keyboard = InputSystem.AddDevice<Keyboard>();
+
+        var map1 = new InputActionMap("map1");
+        var action1 = map1.AddAction(name: "action1");
+        action1.AddCompositeBinding("2DVector")
+            .With("Up", "<Keyboard>/w")
+            .With("Down", "<Keyboard>/s")
+            .With("Left", "<Keyboard>/a")
+            .With("Right", "<Keyboard>/d");
+
+        var map2 = new InputActionMap("map2");
+        var action2 = map2.AddAction(name: "action2");
+        action2.AddCompositeBinding("2DVector")
+            .With("Up", "<Keyboard>/w")
+            .With("Down", "<Keyboard>/s")
+            .With("Left", "<Keyboard>/a")
+            .With("Right", "<Keyboard>/d");
+        var action3 = map2.AddAction(name: "action3", binding: "<Keyboard>/w");
+
+        var asset = ScriptableObject.CreateInstance<InputActionAsset>();
+        asset.AddActionMap(map1);
+        asset.AddActionMap(map2);
+
+        map1.Enable();
+        LogAssert.NoUnexpectedReceived();
+
+        map2.Enable();
+
+        int action1Count = 0;
+        int action2Count = 0;
+        int action3Count = 0;
+        action1.started += ctx => action1Count++;
+        action2.started += ctx => action2Count++;
+        action3.started += ctx => action3Count++;
+
+        Press(keyboard.wKey);
+        if (shortcutsEnabled)
+        {
+            // First action with the most bindings is the ONLY one to trigger
+            Assert.That(action1Count, Is.EqualTo(1));
+            Assert.That(action2Count, Is.EqualTo(0));
+            Assert.That(action3Count, Is.EqualTo(0));
+        }
+        else
+        {
+            // All actions were triggered
+            Assert.That(action1Count, Is.EqualTo(1));
+            Assert.That(action2Count, Is.EqualTo(1));
+            Assert.That(action3Count, Is.EqualTo(1));
+        }
+    }
+
     [Test]
     [Category("Actions")]
     [Ignore("TODO")]
diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md
@@ -14,6 +14,7 @@ however, it has to be formatted properly to pass verification tests.
 - Fixed `ArgumentNullException` when opening the Prefab Overrides window and selecting a component with an `InputAction`.
 - Fixed `{fileID: 0}` getting appended to `ProjectSettings.asset` file when building a project ([case ISXB-296](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-296)).
 - Fixed `Type of instance in array does not match expected type` assertion when using PlayerInput in combination with Control Schemes and Interactions ([case ISXB-282](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-282)).
+- The `InputActions consume their inputs` behaviour for shortcut support introduced in v1.4 is opt-in now and can be enabled via the project settings ([case ISXB-254](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-254))).
 - Fixed Memory alignment issue with deserialized InputEventTraces that could cause infinite loops when playing back replays ([case ISXB-317](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-317)).
 - Fixed an InvalidOperationException when using Hold interaction, and by extension any interaction that changes to performed state after a timeout ([case ISXB-332](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-330)).
 - Fixed `Given object is neither an InputAction nor an InputActionMap` when using `InputActionTrace` on input action from an input action asset ([case ISXB-29](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-29)).
diff --git a/Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs b/Packages/com.unity.inputsystem/InputSystem/Editor/Settings/InputSettingsProvider.cs
@@ -145,6 +145,19 @@ public override void OnGUI(string searchContext)
                 EditorGUILayout.Space();
                 EditorGUILayout.PropertyField(m_EditorInputBehaviorInPlayMode, m_EditorInputBehaviorInPlayModeContent);
 
+                EditorGUILayout.Space();
+                EditorGUILayout.LabelField("Improved Shortcut Support", EditorStyles.boldLabel);
+                EditorGUILayout.Space();
+                EditorGUILayout.PropertyField(m_ShortcutKeysConsumeInputs, m_ShortcutKeysConsumeInputsContent);
+                if (m_ShortcutKeysConsumeInputs.boolValue)
+                    EditorGUILayout.HelpBox("Please note that enabling Improved Shortcut Support will cause actions with composite bindings to consume input and block any other actions which are enabled and sharing the same controls. "
+                        + "Input consumption is performed in priority order, with the action containing the greatest number of bindings checked first. "
+                        + "Therefore actions requiring less keypresses will not be triggered if an action using more keypresses is triggered and has overlapping controls. "
+                        + "This works for shortcut keys, however in other cases this might not give the desired result, especially where there are actions with the exact same number of composite controls, in which case it is non-deterministic which action will be triggered. "
+                        + "These conflicts may occur even between actions which belong to different Action Maps e.g. if using an UIInputModule with the Arrow Keys bound to the Navigate Action in the UI Action Map, this would interfere with other Action Maps using those keys. "
+                        + "Since event consumption only occurs for enabled actions, you can resolve unexpected issues by ensuring that only those Actions or Action Maps that are relevant to your game's current context are enabled. Enabling or disabling actions as your game or application moves between different contexts. "
+                        , MessageType.None);
+
                 if (EditorGUI.EndChangeCheck())
                     Apply();
             }
@@ -268,6 +281,7 @@ private void InitializeWithCurrentSettings()
             m_DefaultHoldTime = m_SettingsObject.FindProperty("m_DefaultHoldTime");
             m_TapRadius = m_SettingsObject.FindProperty("m_TapRadius");
             m_MultiTapDelayTime = m_SettingsObject.FindProperty("m_MultiTapDelayTime");
+            m_ShortcutKeysConsumeInputs = m_SettingsObject.FindProperty("m_ShortcutKeysConsumeInputs");
 
             m_UpdateModeContent = new GUIContent("Update Mode", "When should the Input System be updated?");
             m_CompensateForScreenOrientationContent = new GUIContent("Compensate Orientation", "Whether sensor input on mobile devices should be transformed to be relative to the current device orientation.");
@@ -295,6 +309,7 @@ private void InitializeWithCurrentSettings()
             m_DefaultHoldTimeContent = new GUIContent("Default Hold Time", "Default duration to be used for Hold interactions.");
             m_TapRadiusContent = new GUIContent("Tap Radius", "Maximum distance between two finger taps on a touch screen device allowed for the system to consider this a tap of the same touch (as opposed to a new touch).");
             m_MultiTapDelayTimeContent = new GUIContent("MultiTap Delay Time", "Default delay to be allowed between taps for MultiTap interactions. Also used by by touch devices to count multi taps.");
+            m_ShortcutKeysConsumeInputsContent = new GUIContent("Enable Input Consumption", "Actions are exclusively triggered and will consume/block other actions sharing the same input. E.g. when pressing the 'Shift+B' keys, the associated action would trigger but any action bound to just the 'B' key would be prevented from triggering at the same time.");
 
             // Initialize ReorderableList for list of supported devices.
             var supportedDevicesProperty = m_SettingsObject.FindProperty("m_SupportedDevices");
@@ -404,6 +419,7 @@ private static string[] FindInputSettingsInProject()
         [NonSerialized] private SerializedProperty m_DefaultHoldTime;
         [NonSerialized] private SerializedProperty m_TapRadius;
         [NonSerialized] private SerializedProperty m_MultiTapDelayTime;
+        [NonSerialized] private SerializedProperty m_ShortcutKeysConsumeInputs;
 
         [NonSerialized] private ReorderableList m_SupportedDevices;
         [NonSerialized] private string[] m_AvailableInputSettingsAssets;
@@ -426,6 +442,7 @@ private static string[] FindInputSettingsInProject()
         private GUIContent m_DefaultHoldTimeContent;
         private GUIContent m_TapRadiusContent;
         private GUIContent m_MultiTapDelayTimeContent;
+        private GUIContent m_ShortcutKeysConsumeInputsContent;
 
         [NonSerialized] private InputSettingsiOSProvider m_iOSProvider;
 
diff --git a/Packages/com.unity.inputsystem/InputSystem/InputSettings.cs b/Packages/com.unity.inputsystem/InputSystem/InputSettings.cs
@@ -675,6 +675,16 @@ public void SetInternalFeatureFlag(string featureName, bool enabled)
             if (string.IsNullOrEmpty(featureName))
                 throw new ArgumentNullException(nameof(featureName));
 
+            // REMOVE: this is a temporary crutch to disable shortcut support by default but while also preserving the
+            // existing flag name, as users are aware of that now.
+            if (featureName == InputFeatureNames.kDisableShortcutSupport)
+            {
+                if (m_ShortcutKeysConsumeInputs == !enabled) return;
+                m_ShortcutKeysConsumeInputs = !enabled;
+                OnChange();
+                return;
+            }
+
             if (m_FeatureFlags == null)
                 m_FeatureFlags = new HashSet<string>();
 
@@ -712,11 +722,16 @@ public void SetInternalFeatureFlag(string featureName, bool enabled)
         [SerializeField] private float m_TapRadius = 5;
         [SerializeField] private float m_MultiTapDelayTime = 0.75f;
         [SerializeField] private bool m_DisableRedundantEventsMerging = false;
+        [SerializeField] private bool m_ShortcutKeysConsumeInputs = false; // This is the shortcut support from v1.4. Temporarily moved here as an opt-in feature, while it's issues are investigated.
 
         [NonSerialized] internal HashSet<string> m_FeatureFlags;
 
         internal bool IsFeatureEnabled(string featureName)
         {
+            // REMOVE: this is a temporary crutch to disable shortcut support by default but while also preserving the
+            // existing flag name, as some users are aware of that now.
+            if (featureName == InputFeatureNames.kDisableShortcutSupport) return !m_ShortcutKeysConsumeInputs;
+
             return m_FeatureFlags != null && m_FeatureFlags.Contains(featureName.ToUpperInvariant());
         }
 
