FIX: Timers on interactions sometimes triggering erroneously (case 1251231, #1474).

Author: Rene Damm

Assets/Tests/InputSystem/CoreTests_Actions_Interactions.cs

@@ -462,6 +462,64 @@ public void Actions_HoldInteraction_DoesNotGetStuck_WhenHeldAndReleasedInSameEve
         }
     }
 
+    // https://fogbugz.unity3d.com/f/cases/1251231/
+    [Test]
+    [Category("Actions")]
+    public void Actions_HoldInteraction_CanBePerformedWhenInvolvingMoreThanOneControl()
+    {
+        var keyboard = InputSystem.AddDevice<Keyboard>();
+        InputSystem.AddDevice<Mouse>();
+
+        // Add several bindings just to ensure that if conflict resolution is in the mix,
+        // things don't go sideways.
+
+        var action = new InputAction(interactions: "hold(duration=2)");
+        action.AddCompositeBinding("ButtonWithOneModifier")
+            .With("Modifier", "<Keyboard>/a")
+            .With("Button", "<Keyboard>/s");
+        action.AddCompositeBinding("ButtonWithOneModifier")
+            .With("Modifier", "<Mouse>/leftButton")
+            .With("Button", "<Mouse>/rightButton");
+        action.AddCompositeBinding("ButtonWithOneModifier")
+            .With("Modifier", "<Keyboard>/shift")
+            .With("Button", "<Mouse>/rightButton");
+
+        action.Enable();
+
+        var startedCount = 0;
+        var performedCount = 0;
+        var canceledCount = 0;
+
+        action.started += _ => ++ startedCount;
+        action.performed += _ => ++ performedCount;
+        action.canceled += _ => ++ canceledCount;
+
+        InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.A));
+        InputSystem.Update();
+        InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.A, Key.S));
+        InputSystem.Update();
+
+        Assert.That(startedCount, Is.EqualTo(1));
+        Assert.That(performedCount, Is.Zero);
+        Assert.That(canceledCount, Is.Zero);
+
+        // Release before hold time.
+        InputSystem.QueueStateEvent(keyboard, default(KeyboardState));
+        InputSystem.Update();
+
+        Assert.That(startedCount, Is.EqualTo(1));
+        Assert.That(performedCount, Is.Zero);
+        Assert.That(canceledCount, Is.EqualTo(1));
+
+        currentTime += 3;
+
+        InputSystem.Update();
+
+        Assert.That(startedCount, Is.EqualTo(1));
+        Assert.That(performedCount, Is.Zero);
+        Assert.That(canceledCount, Is.EqualTo(1));
+    }
+
     [Test]
     [Category("Actions")]
     public void Actions_ReleasedHoldInteractionIsCancelled_WithMultipleBindings()

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -13,7 +13,7 @@ however, it has to be formatted properly to pass verification tests.
 ### Changed
 
 - The artificial `ctrl`, `shift`, and `alt` controls (which combine the left and right controls into one) on the keyboard can now be written to and no longer throw `NotSupportedException` when trying to do so ([case 1340793](https://issuetracker.unity3d.com/issues/on-screen-button-errors-on-mouse-down-slash-up-when-its-control-path-is-set-to-control-keyboard)).
-- All devices are now resynced/reset in next update after entering play mode, this is needed to read current state of devices before any intentional input is provided ([case 1231907](https://issuetracker.unity3d.com/issues/mouse-coordinates-reported-as-00-until-the-first-move)).
+- All devices are now re-synced/reset in next update after entering play mode, this is needed to read current state of devices before any intentional input is provided ([case 1231907](https://issuetracker.unity3d.com/issues/mouse-coordinates-reported-as-00-until-the-first-move)).
 - Replaced `UnityLinkerBuildPipelineData.inputDirectory` with hardcoded `Temp` folder because `inputDirectory` is deprecated.
 - Deprecated `InputSettings.filterNoiseOnCurrent`. Now noise filtering is always enabled. Device only will become `.current` if any non-noise control have changed state.
 
@@ -40,6 +40,8 @@ however, it has to be formatted properly to pass verification tests.
 - Fixed interactions such as `Press` not getting processed correctly when having multiple of them on different bindings of the same action and receiving simultaneous input on all of them ([case 1364667](https://issuetracker.unity3d.com/issues/new-input-system-stops-working-after-pressing-2-keyboard-buttons-at-the-same-time)).
   * If, for example, you bind the A and S key on the same action, put a `Press` interaction on both, and then press both keys, interactions would get missed or got stuck.
 - Fixed `InputAction.IsPressed`/`WasPressed`/`WasReleased` returning incorrect results when binding multiple buttons on the same action and pressing/releasing them simultaneously.
+- Fixed interactions involving timeouts (such as `HoldInteraction`) performing erroneous delayed triggers on actions when input is composed of multiple controls ([1251231](https://issuetracker.unity3d.com/issues/input-system-composites-hold-interaction-can-be-performed-when-no-keys-are-hold)).
+  * For example, if you bind `Shift+B` using a `OneModifierComposite` and put a `HoldInteraction` on the binding, then depending on the order in which the keys are pressed, you would sometimes see the action spuriously getting triggered when in fact no input was received.
 
 ## [1.2.0] - 2021-10-22
 

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

@@ -1397,6 +1397,9 @@ private bool IsConflictingInput(ref TriggerState trigger, int actionIndex)
                     if (actionState->bindingIndex != bindingWithHighestActuation)
                     {
                         // If there's an interaction currently driving the action, reset it.
+                        // NOTE: This will also cancel an ongoing timer. So, say we're currently 0.5 seconds into
+                        //       a 1 second "Hold" when the user shifts to a different control, then this code here
+                        //       will *cancel* the current "Hold" and restart from scratch.
                         if (actionState->interactionIndex != kInvalidIndex)
                             ResetInteractionState(trigger.mapIndex, actionState->bindingIndex, actionState->interactionIndex);
 
@@ -1668,8 +1671,7 @@ internal void StartTimeout(float seconds, ref TriggerState trigger)
             // If there's already a timeout running, cancel it first.
             ref var interactionState = ref interactionStates[interactionIndex];
             if (interactionState.isTimerRunning)
-                StopTimeout(trigger.mapIndex, interactionState.triggerControlIndex, trigger.bindingIndex,
-                    interactionIndex);
+                StopTimeout(interactionIndex);
 
             // Add new timeout.
             manager.AddStateChangeMonitorTimeout(control, this, currentTime + seconds, monitorIndex,
@@ -1679,28 +1681,26 @@ internal void StartTimeout(float seconds, ref TriggerState trigger)
             interactionState.isTimerRunning = true;
             interactionState.timerStartTime = currentTime;
             interactionState.timerDuration = seconds;
+            interactionState.timerMonitorIndex = monitorIndex;
         }
 
-        private void StopTimeout(int mapIndex, int controlIndex, int bindingIndex, int interactionIndex)
+        private void StopTimeout(int interactionIndex)
         {
-            Debug.Assert(mapIndex >= 0 && mapIndex < totalMapCount, "Map index out of range");
-            Debug.Assert(controlIndex >= 0 && controlIndex < totalControlCount, "Control index out of range");
             Debug.Assert(interactionIndex >= 0 && interactionIndex < totalInteractionCount, "Interaction index out of range");
 
-            var manager = InputSystem.s_Manager;
-            var monitorIndex =
-                ToCombinedMapAndControlAndBindingIndex(mapIndex, controlIndex, bindingIndex);
+            ref var interactionState = ref interactionStates[interactionIndex];
 
-            manager.RemoveStateChangeMonitorTimeout(this, monitorIndex, interactionIndex);
+            var manager = InputSystem.s_Manager;
+            manager.RemoveStateChangeMonitorTimeout(this, interactionState.timerMonitorIndex, interactionIndex);
 
             // Update state.
-            ref var interactionState = ref interactionStates[interactionIndex];
             interactionState.isTimerRunning = false;
             interactionState.totalTimeoutCompletionDone += interactionState.timerDuration;
             interactionState.totalTimeoutCompletionTimeRemaining =
                 Mathf.Max(interactionState.totalTimeoutCompletionTimeRemaining - interactionState.timerDuration, 0);
             interactionState.timerDuration = default;
             interactionState.timerStartTime = default;
+            interactionState.timerMonitorIndex = default;
         }
 
         /// <summary>
@@ -1750,8 +1750,7 @@ internal void ChangePhaseOfInteraction(InputActionPhase newPhase, ref TriggerSta
             // Any time an interaction changes phase, we cancel all pending timeouts.
             ref var interactionState = ref interactionStates[interactionIndex];
             if (interactionState.isTimerRunning)
-                StopTimeout(trigger.mapIndex, interactionState.triggerControlIndex, trigger.bindingIndex,
-                    trigger.interactionIndex);
+                StopTimeout(trigger.interactionIndex);
 
             // Update interaction state.
             interactionState.phase = newPhase;
@@ -2201,10 +2200,7 @@ private void ResetInteractionState(int mapIndex, int bindingIndex, int interacti
 
             // Clean up timer.
             if (interactionStates[interactionIndex].isTimerRunning)
-            {
-                var controlIndex = interactionStates[interactionIndex].triggerControlIndex;
-                StopTimeout(mapIndex, controlIndex, bindingIndex, interactionIndex);
-            }
+                StopTimeout(interactionIndex);
 
             // Reset state record.
             interactionStates[interactionIndex] =
@@ -2696,7 +2692,7 @@ internal bool ReadValueAsButton(int bindingIndex, int controlIndex)
         /// Records the current state of a single interaction attached to a binding.
         /// Each interaction keeps track of its own trigger control and phase progression.
         /// </summary>
-        [StructLayout(LayoutKind.Explicit, Size = 40)]
+        [StructLayout(LayoutKind.Explicit, Size = 48)]
         internal struct InteractionState
         {
             [FieldOffset(0)] private ushort m_TriggerControlIndex;
@@ -2708,6 +2704,7 @@ internal struct InteractionState
             [FieldOffset(24)] private double m_PerformedTime;
             [FieldOffset(32)] private float m_TotalTimeoutCompletionTimeDone;
             [FieldOffset(36)] private float m_TotalTimeoutCompletionTimeRemaining;
+            [FieldOffset(40)] private long m_TimerMonitorIndex;
 
             public int triggerControlIndex
             {
@@ -2757,6 +2754,12 @@ public float totalTimeoutCompletionTimeRemaining
                 set => m_TotalTimeoutCompletionTimeRemaining = value;
             }
 
+            public long timerMonitorIndex
+            {
+                get => m_TimerMonitorIndex;
+                set => m_TimerMonitorIndex = value;
+            }
+
             public bool isTimerRunning
             {
                 get => ((Flags)m_Flags & Flags.TimerRunning) == Flags.TimerRunning;