FIX: Regression from suppressing same control bound repeatedly (#1394).

Author: Rene Damm

Assets/Tests/InputSystem/CoreTests_Actions.cs

@@ -170,7 +170,7 @@ public void Actions_CanTargetSameControlWithMultipleActions()
     // https://fogbugz.unity3d.com/f/cases/1293808/
     [Test]
     [Category("Actions")]
-    public void Actions_WhenSeveralBindingsResolveToSameControl_ControlIsAssociatedWithFirstActiveBinding()
+    public void Actions_WhenSeveralBindingsResolveToSameControl_SameControlFeedsIntoActionMultipleTimes_ButIsListedInControlsOnlyOnce()
     {
         var gamepad = InputSystem.AddDevice<Gamepad>();
 
@@ -180,6 +180,8 @@ public void Actions_WhenSeveralBindingsResolveToSameControl_ControlIsAssociatedW
         var actionMap = new InputActionMap();
         var action3 = actionMap.AddAction("action3");
         var action4 = actionMap.AddAction("action4");
+        var action5 = actionMap.AddAction("action5");
+        var action6 = actionMap.AddAction("action6");
 
         action1.AddBinding("<Gamepad>/buttonSouth");
         action1.AddBinding("<Gamepad>/buttonSouth");
@@ -192,16 +194,30 @@ public void Actions_WhenSeveralBindingsResolveToSameControl_ControlIsAssociatedW
         action3.AddBinding("<Gamepad>/buttonSouth");
         action4.AddBinding("<Gamepad>/buttonSouth"); // Should not be removed; different action.
 
+        action5.AddBinding("<Gamepad>/buttonSouth", interactions: "press(behavior=0)");
+        action5.AddBinding("<Gamepad>/buttonSouth", interactions: "press(behavior=1)");
+        action5.AddBinding("<Gamepad>/buttonSouth", processors: "invert");
+
+        action6.AddCompositeBinding("Dpad")
+            .With("Left", "<Gamepad>/leftStick/y", processors: "clamp(min=0,max=1)")
+            .With("Right", "<Gamepad>/leftStick/y", processors: "clamp(min=-1,max=0),invert");
+
+        var action6Performed = 0;
+        action6.performed += ctx => action6Performed += ctx.performed ? 1 : 0;
+
         Assert.That(action1.controls, Is.EquivalentTo(new[] { gamepad.buttonSouth }));
         Assert.That(action2.controls, Has.Exactly(1).SameAs(gamepad.buttonSouth));
-        Assert.That(action2.controls, Has.Count.EqualTo(4)); // North, south, east, west
+        Assert.That(action2.controls, Is.EquivalentTo(new[] { gamepad.buttonNorth, gamepad.buttonSouth, gamepad.buttonEast, gamepad.buttonWest }));
         Assert.That(action3.controls, Is.EquivalentTo(new[] { gamepad.buttonNorth, gamepad.buttonSouth }));
         Assert.That(action4.controls, Is.EquivalentTo(new[] { gamepad.buttonSouth }));
+        Assert.That(action5.controls, Is.EquivalentTo(new[] { gamepad.buttonSouth }));
+        Assert.That(action6.controls, Is.EquivalentTo(new[] { gamepad.leftStick.y })); // Only mentioned once.
 
         Assert.That(action1.GetBindingIndexForControl(gamepad.buttonSouth), Is.EqualTo(0));
         Assert.That(action2.GetBindingIndexForControl(gamepad.buttonSouth), Is.EqualTo(0));
         Assert.That(action3.GetBindingIndexForControl(gamepad.buttonSouth), Is.EqualTo(1));
         Assert.That(action4.GetBindingIndexForControl(gamepad.buttonSouth), Is.EqualTo(0));
+        Assert.That(action5.GetBindingIndexForControl(gamepad.buttonSouth), Is.EqualTo(0));
 
         // Go through a bit of pressing and releasing to make sure that the action state
         // processing wasn't thrown off its track.
@@ -210,6 +226,8 @@ public void Actions_WhenSeveralBindingsResolveToSameControl_ControlIsAssociatedW
         action2.Enable();
         action3.Enable();
         action4.Enable();
+        action5.Enable();
+        action6.Enable();
 
         Press(gamepad.buttonSouth);
 
@@ -245,6 +263,11 @@ public void Actions_WhenSeveralBindingsResolveToSameControl_ControlIsAssociatedW
         Assert.That(action2.triggered, Is.False);
         Assert.That(action3.triggered, Is.False);
         Assert.That(action4.triggered, Is.False);
+
+        Set(gamepad.leftStick, new Vector2(0, -1));
+
+        Assert.That(action6Performed, Is.EqualTo(1));
+        Assert.That(action6.ReadValue<Vector2>(), Is.EqualTo(new Vector2(1,  0)));
     }
 
     [Test]

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -8,6 +8,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 Due to package verification, the latest version below is the unpublished version and the date is meaningless.
 however, it has to be formatted properly to pass verification tests.
 
+## [Unreleased]
+
+### Changed
+
+- Modified the fix that landed in `1.1-preview.3` for [any given control being added to an action only once](#same_control_multiple_times_fix).
+  * This caused a regression with some setups that, for example, bound the same control multiple times in a composite using processors to alter the value of the control.
+  * Internally, a control is now again allowed to feed into the same action through more than one binding.
+  * However, externally the control will be mentioned on the action's `InputAction.controls` list only once.
+
 ## [1.1.0-pre.6] - 2021-08-23
 
 ### Fixed
@@ -266,7 +275,7 @@ however, it has to be formatted properly to pass verification tests.
 
 #### Actions
 
-- Fixed actions not triggering correctly when multiple bindings on the same action were referencing the same control ([case 1293808](https://issuetracker.unity3d.com/product/unity/issues/guid/1293808/)).
+- <a name="same_control_multiple_times_fix"></a>Fixed actions not triggering correctly when multiple bindings on the same action were referencing the same control ([case 1293808](https://issuetracker.unity3d.com/product/unity/issues/guid/1293808/)).
   * Bindings will now "claim" controls during resolution. If several bindings __on the same action__ resolve to the same control, only the first such binding will successfully resolve to the control. Subsequent bindings will only resolve to controls not already referenced by other bindings on the action.
   ```CSharp
   var action = new InputAction();

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

@@ -660,22 +660,7 @@ Note that a single [Binding path](Controls.md#control-paths) can match multiple
 
 * A Binding path can also contain wildcards, such as `<Gamepad>/button*`. This matches any Control on any gamepad with a name starting with "button", which matches all the four action buttons on any connected gamepad. A different example: `*/{Submit}` matches any Control tagged with the "Submit" [usage](Controls.md#control-usages) on any Device.
 
-If there are multiple Bindings on the same Action that all reference the same Control(s), only the first such Binding will successfully bind to the control.
-
-```CSharp
-var action1 = new InputAction();
-
-action1.AddBinding("<Gamepad>/buttonSouth");
-action1.AddBinding("<Gamepad>/buttonSouth"); // This binding will be ignored.
-
-var action2 = new InputAction();
-
-action2.AddBinding("<Gamepad>/buttonSouth");
-// Add a binding that implicitly matches the first binding, too. When binding resolution
-// happens, this binding will only receive buttonNorth, buttonWest, and buttonEast, but not
-// buttonSouth as the first binding already received that control.
-action2.AddBinding("<Gamepad>/button*");
-```
+If there are multiple Bindings on the same Action that all reference the same Control(s), the Control will effectively feed into the Action multiple times. This is to allow, for example, a single Control to produce different input on the same Action by virtue of being bound in a different fashion (composites, processors, interactions, etc). However, regardless of how many times a Control is bound on any given action, it will only be mentioned once in the Action's [array of `controls`](../api/UnityEngine.InputSystem.InputAction.html#UnityEngine_InputSystem_InputAction_controls).
 
 To query the Controls that an Action resolves to, you can use [`InputAction.controls`](../api/UnityEngine.InputSystem.InputAction.html#UnityEngine_InputSystem_InputAction_controls). You can also run this query if the Action is disabled.
 

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

@@ -441,9 +441,7 @@ public InputBinding? bindingMask
         /// </example>
         ///
         /// Note that this array will not contain the same control multiple times even if more than
-        /// one binding on an action references the same control. Instead, the first binding on
-        /// an action that resolves to a particular control will essentially "own" the control
-        /// and subsequent bindings for the action will be blocked from resolving to the same control.
+        /// one binding on an action references the same control.
         ///
         /// <example>
         /// <code>

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

@@ -836,6 +836,25 @@ private unsafe void SetUpPerActionCachedBindingData()
                 m_SingletonAction.m_BindingsCount = m_Bindings.Length;
                 m_SingletonAction.m_ControlStartIndex = 0;
                 m_SingletonAction.m_ControlCount = m_State?.totalControlCount ?? 0;
+
+                // Only complication, InputActionState allows a control to appear multiple times
+                // on the same action and InputAction.controls[] doesn't.
+                if (m_ControlsForEachAction.HaveDuplicateReferences(0, m_SingletonAction.m_ControlCount))
+                {
+                    var numControls = 0;
+                    var controls = new InputControl[m_SingletonAction.m_ControlCount];
+                    for (var i = 0; i < m_SingletonAction.m_ControlCount; ++i)
+                    {
+                        if (!controls.ContainsReference(m_ControlsForEachAction[i]))
+                        {
+                            controls[numControls] = m_ControlsForEachAction[i];
+                            ++numControls;
+                        }
+                    }
+
+                    m_ControlsForEachAction = controls;
+                    m_SingletonAction.m_ControlCount = numControls;
+                }
             }
             else
             {
@@ -943,17 +962,27 @@ private unsafe void SetUpPerActionCachedBindingData()
                         // but do not really resolve to controls themselves).
                         if (m_State != null && !m_Bindings[sourceBindingToCopy].isComposite)
                         {
-                            var controlCountForBinding = m_State
-                                .bindingStates[mapIndices.bindingStartIndex + sourceBindingToCopy].controlCount;
+                            ref var bindingState = ref m_State.bindingStates[mapIndices.bindingStartIndex + sourceBindingToCopy];
+
+                            var controlCountForBinding = bindingState.controlCount;
                             if (controlCountForBinding > 0)
                             {
-                                Array.Copy(m_State.controls,
-                                    m_State.bindingStates[mapIndices.bindingStartIndex + sourceBindingToCopy]
-                                        .controlStartIndex,
-                                    m_ControlsForEachAction, currentControlIndex, controlCountForBinding);
-
-                                currentControlIndex += controlCountForBinding;
-                                currentAction.m_ControlCount += controlCountForBinding;
+                                // Internally, we allow several bindings on a given action to resolve to the same control.
+                                // Externally, however, InputAction.controls[] is a set and thus should not contain duplicates.
+                                // So, instead of just doing a straight copy here, we copy controls one by one.
+
+                                var controlStartIndexForBinding = bindingState.controlStartIndex;
+                                for (var n = 0; n < controlCountForBinding; ++n)
+                                {
+                                    var control = m_State.controls[controlStartIndexForBinding + n];
+                                    if (!m_ControlsForEachAction.ContainsReference(currentAction.m_ControlStartIndex,
+                                        currentAction.m_ControlCount, control))
+                                    {
+                                        m_ControlsForEachAction[currentControlIndex] = control;
+                                        ++currentControlIndex;
+                                        ++currentAction.m_ControlCount;
+                                    }
+                                }
                             }
                         }
 

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

@@ -1205,6 +1205,7 @@ private bool ShouldIgnoreControlStateChange(ref TriggerState trigger, int action
             // anything.
             if (actionState->controlIndex == kInvalidIndex)
             {
+                actionState->magnitude = trigger.magnitude;
                 Profiler.EndSample();
                 return false;
             }
@@ -1219,7 +1220,7 @@ private bool ShouldIgnoreControlStateChange(ref TriggerState trigger, int action
             if (trigger.magnitude > actionState->magnitude)
             {
                 // If this is not the control that is currently driving the action, we know
-                // there are multiple controls that are concurrently actuated on the control.
+                // there are multiple controls that are concurrently actuated on the action.
                 // Remember that so that when the controls are released again, we can more
                 // efficiently determine whether we need to take multiple bound controls into
                 // account or not.
@@ -1378,7 +1379,12 @@ private bool ShouldIgnoreControlStateChange(ref TriggerState trigger, int action
                 // it drive the action - this is like a direction change on the same control.
                 if (bindingStates[trigger.bindingIndex].isPartOfComposite && triggerControlIndex == actionStateControlIndex)
                     return false;
-                if (trigger.magnitude > 0 && triggerControlIndex != actionState->controlIndex)
+                // If we do have an actuation on a control that isn't currently driving the action, flag the action has
+                // having multiple concurrent inputs ATM.
+                // NOTE: We explicitly check for whether it is in fact not the same control even if the control indices are different.
+                //       The reason is that we allow the same control, on the same action to be bound more than once on the same
+                //       action.
+                if (trigger.magnitude > 0 && triggerControlIndex != actionState->controlIndex && controls[triggerControlIndex] != controls[actionState->controlIndex])
                     actionState->hasMultipleConcurrentActuations = true;
                 return true;
             }

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

@@ -281,40 +281,6 @@ public unsafe void AddActionMap(InputActionMap map)
                                 numControls = InputSystem.FindControls(path, ref resolvedControls);
                             }
 
-                            // Check for controls that are already bound to the action through other
-                            // bindings. The first binding that grabs a specific control gets to "own" it.
-                            if (numControls > 0)
-                            {
-                                for (var i = 0; i < n; ++i)
-                                {
-                                    ref var otherBindingState = ref bindingStatesPtr[bindingStartIndex + i];
-
-                                    // Skip if binding has no controls.
-                                    if (otherBindingState.controlCount == 0)
-                                        continue;
-
-                                    // Skip if binding isn't from same action.
-                                    if (otherBindingState.actionIndex != actionStartIndex + actionIndexInMap)
-                                        continue;
-
-                                    // Check for controls in the set that we just resolved that are also on the other
-                                    // binding. Each such control we find, we kick out of the list.
-                                    for (var k = 0; k < numControls; ++k)
-                                    {
-                                        var controlOnCurrentBinding = resolvedControls[firstControlIndex + k - memory.controlCount];
-                                        var controlIndexOnOtherBinding = resolvedControls.IndexOf(controlOnCurrentBinding,
-                                            otherBindingState.controlStartIndex - memory.controlCount, otherBindingState.controlCount);
-                                        if (controlIndexOnOtherBinding != -1)
-                                        {
-                                            // Control is bound to a previous binding. Remove it from the current binding.
-                                            resolvedControls.RemoveAt(firstControlIndex + k - memory.controlCount);
-                                            --numControls;
-                                            --k;
-                                        }
-                                    }
-                                }
-                            }
-
                             // Disable binding if it doesn't resolve to any controls.
                             // NOTE: This also happens to bindings that got all their resolved controls removed because other bindings from the same
                             //       action already grabbed them.
@@ -408,7 +374,8 @@ public unsafe void AddActionMap(InputActionMap map)
                                 throw new InvalidOperationException(
                                     $"Binding '{unresolvedBinding}' that is part of composite '{composites[currentCompositeIndex]}' is missing a name");
 
-                            // Give a part index for the
+                            // Assign an index to the current part of the composite which
+                            // can be used by the composite to read input from this part.
                             partIndex = AssignCompositePartIndex(composites[currentCompositeIndex], unresolvedBinding.name,
                                 ref currentCompositePartCount);
 

Packages/com.unity.inputsystem/InputSystem/Utilities/ArrayHelpers.cs

@@ -107,6 +107,27 @@ public static bool ContainsReference<TFirst, TSecond>(this TFirst[] array, int c
             return IndexOfReference(array, value, count) != -1;
         }
 
+        public static bool ContainsReference<TFirst, TSecond>(this TFirst[] array, int startIndex, int count, TSecond value)
+            where TSecond : class
+            where TFirst : TSecond
+        {
+            return IndexOfReference(array, value, startIndex, count) != -1;
+        }
+
+        public static bool HaveDuplicateReferences<TFirst>(this TFirst[] first, int index, int count)
+        {
+            for (var i = 0; i < count; ++i)
+            {
+                var element = first[i];
+                for (var n = i + 1; n < count - i; ++n)
+                {
+                    if (ReferenceEquals(element, first[n]))
+                        return true;
+                }
+            }
+            return false;
+        }
+
         public static bool HaveEqualElements<TValue>(TValue[] first, TValue[] second, int count = int.MaxValue)
         {
             if (first == null || second == null)