From 03ea1f676f5eff270dc6fcb523247061f97d488a Mon Sep 17 00:00:00 2001 From: Simon Wittber Date: Wed, 23 Oct 2024 09:55:09 +0800 Subject: [PATCH 1/3] Revert "FIX: Composite binding isn't triggered after ResetDevice() called during Action handler (ISXB-746) (#1893)" This reverts commit 0ddd534d86ea0b7bb2d48b48fd47af951a93aef9. --- Assets/Tests/InputSystem/CoreTests_Actions.cs | 95 ------------------- .../InputSystem/Actions/InputActionState.cs | 62 +++++------- 2 files changed, 23 insertions(+), 134 deletions(-) diff --git a/Assets/Tests/InputSystem/CoreTests_Actions.cs b/Assets/Tests/InputSystem/CoreTests_Actions.cs index ba26bc52b6..0aa3d09fbe 100644 --- a/Assets/Tests/InputSystem/CoreTests_Actions.cs +++ b/Assets/Tests/InputSystem/CoreTests_Actions.cs @@ -12405,99 +12405,4 @@ 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(); - bool actionPerformed; - - // Enables "Modifier must be pressed first" behavior on all Composite Bindings - InputSystem.settings.shortcutKeysConsumeInput = true; - - const string modifier1 = "/shift"; - const string modifier2 = "/ctrl"; - const string key = "/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); - } } diff --git a/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs b/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs index cd17fdd942..79cfa1b8cb 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs @@ -1476,43 +1476,37 @@ 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 set a flag so it isn't triggered again. + // If the composite has already been triggered from the very same event, ignore it. // 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. - // NOTE: Do NOT ignore this Event, we still need finish processing the individual button states. - if (!ShouldIgnoreInputOnCompositeBinding(compositeBindingPtr, eventPtr)) - { - // 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; + if (ShouldIgnoreInputOnCompositeBinding(compositeBindingPtr, eventPtr)) + return; - // Run through interactions on composite. - var interactionCountOnComposite = compositeBindingPtr->interactionCount; - if (interactionCountOnComposite > 0) - { - haveInteractionsOnComposite = true; - ProcessInteractions(ref trigger, - compositeBindingPtr->interactionStartIndex, - interactionCountOnComposite); - } - } - else + // 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) { - compositeAlreadyTriggered = true; + haveInteractionsOnComposite = true; + ProcessInteractions(ref trigger, + compositeBindingPtr->interactionStartIndex, + interactionCountOnComposite); } } @@ -1521,31 +1515,21 @@ 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). - // - // 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. - } + var 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 && !compositeAlreadyTriggered) + else if (!haveInteractionsOnComposite && !isConflictingInput) { ProcessDefaultInteraction(ref trigger, actionIndex); } From 5921275331a97af5f3e9c3169f67f2136c84dee1 Mon Sep 17 00:00:00 2001 From: Simon Wittber Date: Wed, 23 Oct 2024 09:59:17 +0800 Subject: [PATCH 2/3] updated changelog --- Packages/com.unity.inputsystem/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md index 79e1ff562f..957a5ae3b9 100644 --- a/Packages/com.unity.inputsystem/CHANGELOG.md +++ b/Packages/com.unity.inputsystem/CHANGELOG.md @@ -9,7 +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 - +- Reverted changes from 0ddd534d8 (ISXB-746) which introduced a regression [ISXB-1127](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1127) ## [1.11.2] - 2024-10-16 From f3031bedb8a25d2501bf6df781df37f9e8d57ac3 Mon Sep 17 00:00:00 2001 From: Simon Wittber Date: Mon, 28 Oct 2024 13:51:18 +0800 Subject: [PATCH 3/3] changelog update --- Packages/com.unity.inputsystem/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md index 56651a46a5..edfb394b39 100644 --- a/Packages/com.unity.inputsystem/CHANGELOG.md +++ b/Packages/com.unity.inputsystem/CHANGELOG.md @@ -12,7 +12,7 @@ however, it has to be formatted properly to pass verification tests. ### Fixed - Fixed an issue causing the Action context menu to not show on right click when right clicking an action in the Input Action Editor [ISXB-1134](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1134). -- Reverted changes from 0ddd534d8 (ISXB-746) which introduced a regression. [ISXB-1127](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1127) +- Reverted changes from 0ddd534d8 (ISXB-746) which introduced a regression [ISXB-1127](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1127). - Fixed `ArgumentNullException: Value cannot be null.` during the migration of Project-wide Input Actions from `InputManager.asset` to `InputSystem_Actions.inputactions` asset which lead do the lost of the configuration [ISXB-1105](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1105) - Fixed pointerId staying the same when simultaneously releasing and then pressing in the same frame on mobile using touch. [ISXB-1006](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-845)