FIX: Composite binding isn't triggered after ResetDevice() called during Action handler (ISXB-746) (#1893)

* Modifies ProcessControlStateChange() so ProcessButtonState() still called even if Composite already triggered

* Adds a test to cover this corner-case scenario

Co-authored-by: Paulius Dervinis <[email protected]>

Author: timkeo

Assets/Tests/InputSystem/CoreTests_Actions.cs

@@ -12029,4 +12029,99 @@ public void Actions_ActionMapDisabledDuringOnAfterSerialization()
         Assert.That(map.enabled, Is.True);
         Assert.That(map.FindAction("MyAction", true).enabled, Is.True);
     }
+
+    // ResetDevice wasn't properly clearly Composite key state, i.e. BindingState.pressTime
+    // https://jira.unity3d.com/browse/ISXB-746
+    [Test]
+    [TestCase(false)]
+    [TestCase(true)]
+    [Category("Actions")]
+    public void Actions_CompositeBindingResetWhenResetDeviceCalledWhileExecutingAction(bool useTwoModifierComposite)
+    {
+        var keyboard = InputSystem.AddDevice<Keyboard>();
+        bool actionPerformed;
+
+        // Enables "Modifier must be pressed first" behavior on all Composite Bindings
+        InputSystem.settings.shortcutKeysConsumeInput = true;
+
+        const string modifier1 = "<Keyboard>/shift";
+        const string modifier2 = "<Keyboard>/ctrl";
+        const string key = "<Keyboard>/F1";
+
+        var map = new InputActionMap();
+        var resetAction = map.AddAction("resetAction");
+
+        if (!useTwoModifierComposite)
+        {
+            resetAction.AddCompositeBinding("OneModifier")
+                .With("Modifier", modifier1)
+                .With("Binding", key);
+        }
+        else
+        {
+            resetAction.AddCompositeBinding("TwoModifiers")
+                .With("Modifier1", modifier1)
+                .With("Modifier2", modifier2)
+                .With("Binding", key);
+        }
+
+        resetAction.performed += (InputAction.CallbackContext ctx) =>
+        {
+            // Disable the Keyboard while action is being performed.
+            // This simulates an "OnFocusLost" event occurring while processing the Action, e.g. when switching primary displays or moving the main window
+            actionPerformed = true;
+            InputSystem.s_Manager.EnableOrDisableDevice(keyboard.device, false, InputManager.DeviceDisableScope.TemporaryWhilePlayerIsInBackground);
+        };
+
+        map.Enable();
+
+        actionPerformed = false;
+        Press(keyboard.leftShiftKey);
+        Press(keyboard.leftCtrlKey);
+        Press(keyboard.f1Key);
+
+        Assert.IsTrue(actionPerformed);
+
+        // Re enable the Keyboard (before keys are released) and execute Action again
+        InputSystem.s_Manager.EnableOrDisableDevice(keyboard.device, true, InputManager.DeviceDisableScope.TemporaryWhilePlayerIsInBackground);
+
+        actionPerformed = false;
+        Release(keyboard.leftShiftKey);
+        Release(keyboard.leftCtrlKey);
+        Release(keyboard.f1Key);
+
+        Press(keyboard.leftCtrlKey);
+        Press(keyboard.leftShiftKey);
+        Press(keyboard.f1Key);
+
+        Assert.IsTrue(actionPerformed);
+
+        actionPerformed = false;
+        Release(keyboard.leftCtrlKey);
+        Release(keyboard.leftShiftKey);
+        Release(keyboard.f1Key);
+
+        // Re enable the Keyboard (after keys are released) and execute Action one more time
+        InputSystem.s_Manager.EnableOrDisableDevice(keyboard.device, true, InputManager.DeviceDisableScope.TemporaryWhilePlayerIsInBackground);
+
+        Press(keyboard.leftCtrlKey);
+        Press(keyboard.leftShiftKey);
+        Press(keyboard.f1Key);
+
+        Assert.IsTrue(actionPerformed);
+
+        actionPerformed = false;
+        Press(keyboard.leftShiftKey);
+        Press(keyboard.leftCtrlKey);
+        Press(keyboard.f1Key);
+
+        // Re enable the Keyboard (before keys are released) and verify Action isn't triggered when Key pressed first
+        InputSystem.s_Manager.EnableOrDisableDevice(keyboard.device, true, InputManager.DeviceDisableScope.TemporaryWhilePlayerIsInBackground);
+
+        Press(keyboard.f1Key);
+        Press(keyboard.leftCtrlKey);
+        Press(keyboard.leftShiftKey);
+
+        Assert.IsFalse(actionPerformed);
+    }
 }

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -9,6 +9,7 @@ Due to package verification, the latest version below is the unpublished version
 however, it has to be formatted properly to pass verification tests.
 
 ## [Unreleased] - yyyy-mm-dd
+- Fixed Composite binding isn't triggered after ResetDevice() called during Action handler [ISXB-746](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-746)
 
 ## [1.8.2] - 2024-04-29
 

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

@@ -1464,37 +1464,43 @@ private void ProcessControlStateChange(int mapIndex, int controlIndex, int bindi
                     // If the binding is part of a composite, check for interactions on the composite
                     // itself and give them a first shot at processing the value change.
                     var haveInteractionsOnComposite = false;
+                    var compositeAlreadyTriggered = false;
                     if (bindingStatePtr->isPartOfComposite)
                     {
                         var compositeBindingIndex = bindingStatePtr->compositeOrCompositeBindingIndex;
                         var compositeBindingPtr = &bindingStates[compositeBindingIndex];
 
-                        // If the composite has already been triggered from the very same event, ignore it.
+                        // If the composite has already been triggered from the very same event set a flag so it isn't triggered again.
                         // Example: KeyboardState change that includes both A and W key state changes and we're looking
                         //          at a WASD composite binding. There's a state change monitor on both the A and the W
                         //          key and thus the manager will notify us individually of both changes. However, we
                         //          want to perform the action only once.
-                        if (ShouldIgnoreInputOnCompositeBinding(compositeBindingPtr, eventPtr))
-                            return;
-
-                        // Update magnitude for composite.
-                        var compositeIndex = bindingStates[compositeBindingIndex].compositeOrCompositeBindingIndex;
-                        var compositeContext = new InputBindingCompositeContext
+                        // NOTE: Do NOT ignore this Event, we still need finish processing the individual button states.
+                        if (!ShouldIgnoreInputOnCompositeBinding(compositeBindingPtr, eventPtr))
                         {
-                            m_State = this,
-                            m_BindingIndex = compositeBindingIndex
-                        };
-                        trigger.magnitude = composites[compositeIndex].EvaluateMagnitude(ref compositeContext);
-                        memory.compositeMagnitudes[compositeIndex] = trigger.magnitude;
-
-                        // Run through interactions on composite.
-                        var interactionCountOnComposite = compositeBindingPtr->interactionCount;
-                        if (interactionCountOnComposite > 0)
+                            // Update magnitude for composite.
+                            var compositeIndex = bindingStates[compositeBindingIndex].compositeOrCompositeBindingIndex;
+                            var compositeContext = new InputBindingCompositeContext
+                            {
+                                m_State = this,
+                                m_BindingIndex = compositeBindingIndex
+                            };
+                            trigger.magnitude = composites[compositeIndex].EvaluateMagnitude(ref compositeContext);
+                            memory.compositeMagnitudes[compositeIndex] = trigger.magnitude;
+
+                            // Run through interactions on composite.
+                            var interactionCountOnComposite = compositeBindingPtr->interactionCount;
+                            if (interactionCountOnComposite > 0)
+                            {
+                                haveInteractionsOnComposite = true;
+                                ProcessInteractions(ref trigger,
+                                    compositeBindingPtr->interactionStartIndex,
+                                    interactionCountOnComposite);
+                            }
+                        }
+                        else
                         {
-                            haveInteractionsOnComposite = true;
-                            ProcessInteractions(ref trigger,
-                                compositeBindingPtr->interactionStartIndex,
-                                interactionCountOnComposite);
+                            compositeAlreadyTriggered = true;
                         }
                     }
 
@@ -1503,21 +1509,31 @@ private void ProcessControlStateChange(int mapIndex, int controlIndex, int bindi
                     // one of higher magnitude) or may even lead us to switch to processing a different binding
                     // (e.g. when an input of previously greater magnitude has now fallen below the level of another
                     // ongoing input with now higher magnitude).
-                    var isConflictingInput = IsConflictingInput(ref trigger, actionIndex);
-                    bindingStatePtr = &bindingStates[trigger.bindingIndex]; // IsConflictingInput may switch us to a different binding.
+                    //
+                    // If Composite has already been triggered, skip this step; it's unnecessary and could also
+                    // cause a processing issue if we switch to another binding.
+                    var isConflictingInput = false;
+                    if (!compositeAlreadyTriggered)
+                    {
+                        isConflictingInput = IsConflictingInput(ref trigger, actionIndex);
+                        bindingStatePtr = &bindingStates[trigger.bindingIndex]; // IsConflictingInput may switch us to a different binding.
+                    }
 
                     // Process button presses/releases.
+                    // We MUST execute this processing even if Composite has already been triggered to ensure button states
+                    // are properly updated (ISXB-746)
                     if (!isConflictingInput)
                         ProcessButtonState(ref trigger, actionIndex, bindingStatePtr);
 
                     // If we have interactions, let them do all the processing. The presence of an interaction
                     // essentially bypasses the default phase progression logic of an action.
+                    // Interactions are skipped if compositeAlreadyTriggered is set.
                     var interactionCount = bindingStatePtr->interactionCount;
                     if (interactionCount > 0 && !bindingStatePtr->isPartOfComposite)
                     {
                         ProcessInteractions(ref trigger, bindingStatePtr->interactionStartIndex, interactionCount);
                     }
-                    else if (!haveInteractionsOnComposite && !isConflictingInput)
+                    else if (!haveInteractionsOnComposite && !isConflictingInput && !compositeAlreadyTriggered)
                     {
                         ProcessDefaultInteraction(ref trigger, actionIndex);
                     }