FIX: Composite touchscreen controls not firing action after enabling (ISXB-98) (#1540)

* Investigation test

* FIX: Composite touchscreen controls not firing action after enabling (ISXB-98)

* Formatting changes

* Review changes. Keep bit packing code together in same class

* Review changes. Cleanup test comments

* Update changelog

* Review change: update doc regarding sorting behaviour

* Review change: fix comment

* Review change: fix comments and access modifier

Co-authored-by: Dmytro Ivanov <dmytro@unity3d.com>

Author: jamesmcgill

Assets/Tests/InputSystem/CoreTests_Actions.cs

@@ -53,7 +53,7 @@ public void Actions_CanConsumeInput(bool legacyComposites)
 
         // Enable some actions individually to make sure the code that deals
         // with re-resolution of already enabled bindings handles the enabling
-        // of just individual actions out of the whole set correctlyuk.
+        // of just individual actions out of the whole set correctly.
         action1.Enable();
         action2.Enable();
 
@@ -2077,10 +2077,10 @@ public void Actions_CanCreateActionAssetWithMultipleActionMaps()
                     .AndThen(Performed(action4,
                         value: new StickDeadzoneProcessor().Process(new Vector2(0.123f, 0.234f)) * new Vector2(1, -1),
                         control: gamepad.leftStick, time: startTime + 0.234))
-                    // map3/action5 should have been started.
-                    .AndThen(Started<TapInteraction>(action5, value: 1f, control: gamepad.buttonSouth, time: startTime + 0.345))
                     // map2/action3 should have been started.
                     .AndThen(Started<TapInteraction>(action3, value: 1f, control: gamepad.buttonSouth, time: startTime + 0.345))
+                    // map3/action5 should have been started.
+                    .AndThen(Started<TapInteraction>(action5, value: 1f, control: gamepad.buttonSouth, time: startTime + 0.345))
                     // map3/action4 should have been performed as the stick has been moved
                     // beyond where it had already moved.
                     .AndThen(Performed(action4,
@@ -7463,7 +7463,10 @@ public void Actions_CanCreateAxisComposite()
             InputSystem.QueueStateEvent(gamepad, new GamepadState {rightTrigger = 0.456f});
             InputSystem.Update();
 
-            Assert.That(trace, Performed(action, control: gamepad.rightTrigger, value: 0.456f));
+            // Bit of an odd case. leftTrigger and rightTrigger have both changed state here so
+            // in a way, it's up to the system which one to pick. Might be useful if it was deliberately
+            // picking the control with the highest magnitude but not sure it's worth the effort.
+            Assert.That(trace, Performed(action, control: gamepad.leftTrigger, value: 0.456f));
 
             trace.Clear();
 
@@ -8177,7 +8180,7 @@ public void Actions_CompositesReportControlThatTriggeredTheCompositeInCallback()
         InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.A, Key.S));
         InputSystem.Update();
 
-        Assert.That(performedControl, Is.EqualTo(keyboard.aKey));
+        Assert.That(performedControl, Is.EqualTo(keyboard.sKey));
 
         LogAssert.NoUnexpectedReceived();
     }
@@ -10045,6 +10048,116 @@ public void Actions_RebindingCandidatesShouldBeSorted_IfAddingNewCandidate()
         }
     }
 
+    // Straight from demo project
+    public struct PointerInput
+    {
+        public bool Contact;
+        public int InputId;
+        public Vector2 Position;
+        public Vector2? Tilt;
+        public float? Pressure;
+        public Vector2? Radius;
+        public float? Twist;
+    }
+
+    public class PointerInputComposite : InputBindingComposite<PointerInput>
+    {
+        [InputControl(layout = "Button")]
+        public int contact;
+
+        [InputControl(layout = "Vector2")]
+        public int position;
+
+        [InputControl(layout = "Vector2")]
+        public int tilt;
+
+        [InputControl(layout = "Vector2")]
+        public int radius;
+
+        [InputControl(layout = "Axis")]
+        public int pressure;
+
+        [InputControl(layout = "Axis")]
+        public int twist;
+
+        [InputControl(layout = "Integer")]
+        public int inputId;
+
+        public override PointerInput ReadValue(ref InputBindingCompositeContext context)
+        {
+            var contact = context.ReadValueAsButton(this.contact);
+            var pointerId = context.ReadValue<int>(inputId);
+            var pressure = context.ReadValue<float>(this.pressure);
+            var radius = context.ReadValue<Vector2, Vector2MagnitudeComparer>(this.radius);
+            var tilt = context.ReadValue<Vector2, Vector2MagnitudeComparer>(this.tilt);
+            var position = context.ReadValue<Vector2, Vector2MagnitudeComparer>(this.position);
+            var twist = context.ReadValue<float>(this.twist);
+
+            return new PointerInput
+            {
+                Contact = contact,
+                InputId = pointerId,
+                Position = position,
+                Tilt = tilt != default ? tilt : (Vector2?)null,
+                Pressure = pressure > 0 ? pressure : (float?)null,
+                Radius = radius.sqrMagnitude > 0 ? radius : (Vector2?)null,
+                Twist = twist > 0 ? twist : (float?)null,
+            };
+        }
+    }
+
+    // https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-98
+    [Test]
+    [Category("Actions")]
+    [TestCase(true)]
+    [TestCase(false)]
+    public void Actions_WithMultipleCompositeBindings_WithoutEvaluateMagnitude_Works(bool prepopulateTouchesBeforeEnablingAction)
+    {
+        InputSystem.RegisterBindingComposite<PointerInputComposite>();
+
+        InputSystem.AddDevice<Touchscreen>();
+
+        var actionMap = new InputActionMap("test");
+        var action = actionMap.AddAction("point", InputActionType.Value);
+        for (var i = 0; i < 5; ++i)
+            action.AddCompositeBinding("PointerInput")
+                .With("contact", $"<Touchscreen>/touch{i}/press")
+                .With("position", $"<Touchscreen>/touch{i}/position")
+                .With("radius", $"<Touchscreen>/touch{i}/radius")
+                .With("pressure", $"<Touchscreen>/touch{i}/pressure")
+                .With("inputId", $"<Touchscreen>/touch{i}/touchId");
+
+        var values = new List<PointerInput>();
+        action.started += ctx => values.Add(ctx.ReadValue<PointerInput>());
+        action.performed += ctx => values.Add(ctx.ReadValue<PointerInput>());
+        action.canceled += ctx => values.Add(ctx.ReadValue<PointerInput>());
+
+        if (!prepopulateTouchesBeforeEnablingAction) // normally actions are enabled before any control actuations happen
+            actionMap.Enable();
+
+        // Start 5 touches, so we fill all slots [touch0, touch4] in Touchscreen with some valid touchId
+        for (var i = 0; i < 2; ++i)
+            BeginTouch(100 + i, new Vector2(100 * (i + 1), 100 * (i + 1)));
+        for (var i = 0; i < 2; ++i)
+            EndTouch(100 + i, new Vector2(100 * (i + 1), 100 * (i + 1)));
+        Assert.That(values.Count, Is.EqualTo(prepopulateTouchesBeforeEnablingAction ? 0 : 3));
+        values.Clear();
+
+        // Now when enabling actionMap ..
+        actionMap.Enable();
+        // On the following update we will trigger OnBeforeUpdate which will rise started/performed
+        // from InputActionState.OnBeforeInitialUpdate as controls are "actuated"
+        InputSystem.Update();
+        Assert.That(values.Count, Is.EqualTo(prepopulateTouchesBeforeEnablingAction ? 2 : 0)); // started+performed arrive from OnBeforeUpdate
+        values.Clear();
+
+        // Now subsequent touches should not be ignored
+        BeginTouch(200, new Vector2(1, 1));
+        Assert.That(values.Count, Is.EqualTo(1));
+        Assert.That(values[0].InputId, Is.EqualTo(200));
+        Assert.That(values[0].Position, Is.EqualTo(new Vector2(1, 1)));
+    }
+
     [Test]
     [Category("Actions")]
     [Ignore("TODO")]

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -10,6 +10,13 @@ however, it has to be formatted properly to pass verification tests.
 
 ## [Unreleased]
 
+### Changed
+
+### Fixed
+- Fixed composite touchscreen controls were not firing an action if screen was touched before enabling the action ([case ISXB-98](https://jira.unity3d.com/browse/ISXB-98)).
+
+### Added
+
 ## [1.4.0] - 2022-04-10
 
 ### Changed

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

@@ -1333,11 +1333,19 @@ void IInputStateChangeMonitor.NotifyTimerExpired(InputControl control, double ti
             ProcessTimeout(time, mapIndex, controlIndex, bindingIndex, interactionIndex);
         }
 
-        // We mangle the various indices we use into a single long for association with state change
-        // monitors. While we could look up map and binding indices from control indices, keeping
-        // all the information together avoids having to unnecessarily jump around in memory to grab
-        // the various pieces of data.
-
+        /// <summary>
+        /// Bit pack the mapIndex, controlIndex, bindingIndex and complexity components into a single long monitor index value.
+        /// </summary>
+        /// <param name="mapIndex">The mapIndex value to pack.</param>
+        /// <param name="controlIndex">The controlIndex value to pack.</param>
+        /// <param name="bindingIndex">The bindingIndex value to pack..</param>
+        /// <remarks>
+        /// We mangle the various indices we use into a single long for association with state change
+        /// monitors. While we could look up map and binding indices from control indices, keeping
+        /// all the information together avoids having to unnecessarily jump around in memory to grab
+        /// the various pieces of data.
+        /// The complexity component is implicitly derived and does not need to be passed as an argument.
+        /// </remarks>
         private long ToCombinedMapAndControlAndBindingIndex(int mapIndex, int controlIndex, int bindingIndex)
         {
             // We have limits on the numbers of maps, controls, and bindings we allow in any single
@@ -1350,6 +1358,13 @@ private long ToCombinedMapAndControlAndBindingIndex(int mapIndex, int controlInd
             return result;
         }
 
+        /// <summary>
+        /// Extract the mapIndex, controlIndex and bindingIndex components from the provided bit packed argument (monitor index).
+        /// </summary>
+        /// <param name="mapControlAndBindingIndex">Represents a monitor index, which is a bit packed field containing multiple components.</param>
+        /// <param name="mapIndex">Will hold the extracted mapIndex value after the function completes.</param>
+        /// <param name="controlIndex">Will hold the extracted controlIndex value after the function completes.</param>
+        /// <param name="bindingIndex">Will hold the extracted bindingIndex value after the function completes.</param>
         private void SplitUpMapAndControlAndBindingIndex(long mapControlAndBindingIndex, out int mapIndex,
             out int controlIndex, out int bindingIndex)
         {
@@ -1358,6 +1373,15 @@ private void SplitUpMapAndControlAndBindingIndex(long mapControlAndBindingIndex,
             mapIndex = (int)((mapControlAndBindingIndex >> 40) & 0xff);
         }
 
+        /// <summary>
+        /// Extract the 'complexity' component from the provided bit packed argument (monitor index).
+        /// </summary>
+        /// <param name="mapControlAndBindingIndex">Represents a monitor index, which is a bit packed field containing multiple components.</param>
+        internal static int GetComplexityFromMonitorIndex(long mapControlAndBindingIndex)
+        {
+            return (int)((mapControlAndBindingIndex >> 48) & 0xff);
+        }
+
         /// <summary>
         /// Process a state change that has happened in one of the controls attached
         /// to this action map state.

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

@@ -261,8 +261,15 @@ public void SortMonitorsByIndex()
                 // Insertion sort.
                 for (var i = 1; i < signalled.length; ++i)
                 {
-                    for (var j = i; j > 0 && listeners[j - 1].monitorIndex < listeners[j].monitorIndex; --j)
+                    for (var j = i; j > 0; --j)
                     {
+                        // Sort by complexities only to keep the sort stable
+                        // i.e. don't reverse the order of controls which have the same complexity
+                        var firstComplexity = InputActionState.GetComplexityFromMonitorIndex(listeners[j - 1].monitorIndex);
+                        var secondComplexity = InputActionState.GetComplexityFromMonitorIndex(listeners[j].monitorIndex);
+                        if (firstComplexity >= secondComplexity)
+                            break;
+
                         listeners.SwapElements(j, j - 1);
                         memoryRegions.SwapElements(j, j - 1);
 

Packages/com.unity.inputsystem/InputSystem/State/InputState.cs

@@ -155,7 +155,7 @@ public static bool IsIntegerFormat(this FourCC format)
         /// <exception cref="ArgumentNullException"><paramref name="control"/> is <c>null</c> -or- <paramref name="monitor"/> is <c>null</c>.</exception>
         /// <exception cref="ArgumentException">The <see cref="InputDevice"/> of <paramref name="control"/> has not been <see cref="InputDevice.added"/>.</exception>
         /// <remarks>
-        /// All monitors on an <see cref="InputDevice"/> are sorted by their <paramref name="monitorIndex"/> (in decreasing order) and invoked
+        /// All monitors on an <see cref="InputDevice"/> are sorted by the complexity specified in their <paramref name="monitorIndex"/> (in decreasing order) and invoked
         /// in that order.
         ///
         /// Every handler gets an opportunity to set <see cref="InputEventPtr.handled"/> to <c>true</c>. When doing so, all remaining pending monitors