From 7a8dcc2bc2c926b4d5d33064a9ad2d14db695ef2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Malrat?= Date: Tue, 10 Sep 2024 16:39:03 -0400 Subject: [PATCH 1/2] Fixed multiple interactions on composite bindings --- Assets/Tests/InputSystem/CoreTests_Actions.cs | 106 ++++++++++++++++++ Packages/com.unity.inputsystem/CHANGELOG.md | 5 + .../InputSystem/Actions/InputActionState.cs | 14 ++- .../Actions/InputBindingResolver.cs | 42 ++++--- 4 files changed, 146 insertions(+), 21 deletions(-) diff --git a/Assets/Tests/InputSystem/CoreTests_Actions.cs b/Assets/Tests/InputSystem/CoreTests_Actions.cs index 3106ccf80f..799e98a0dd 100644 --- a/Assets/Tests/InputSystem/CoreTests_Actions.cs +++ b/Assets/Tests/InputSystem/CoreTests_Actions.cs @@ -10041,6 +10041,112 @@ public void Actions_CanCreateCompositesWithMultipleBindings() Assert.That(value, Is.EqualTo(new Vector2(-1, -1).normalized).Using(Vector2EqualityComparer.Instance)); } + // Ensure that https://jira.unity3d.com/browse/ISXB-619 regress + [Test] + [Category("Actions")] + public void Actions_WithCompositeWithMultipleInteractions_Works() + { + // Will ensure that : + // PressRelease AW trigger a tap + // Long PressRelease AW trigger a hold + // PressRelease AW trigger a tap + + var keyboard = InputSystem.AddDevice(); + var action = new InputAction(); + action.AddCompositeBinding("Dpad", interactions: "tap,hold(duration=2)") + .With("Up", "/w") + .With("Down", "/s") + .With("Left", "/a") + .With("Right", "/d"); + action.Enable(); + + IInputInteraction performedInteraction = null; + IInputInteraction canceledInteraction = null; + action.performed += ctx => + { + performedInteraction = ctx.interaction; + }; + action.canceled += ctx => + { + canceledInteraction = ctx.interaction; + }; + + // PressRelease AW trigger a tap + currentTime = 0; + InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.A, Key.W)); + InputSystem.Update(); + + // nothing triggered + Assert.That(canceledInteraction, Is.Null); + Assert.That(performedInteraction, Is.Null); + + currentTime = 0.01; + InputSystem.QueueStateEvent(keyboard, new KeyboardState()); + InputSystem.Update(); + + // tap should be triggered + Assert.That(canceledInteraction, Is.Null); + Assert.That(performedInteraction, Is.TypeOf(typeof(TapInteraction))); + performedInteraction = null; + + // Long PressRelease AW trigger a hold + currentTime = 1; + InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.A, Key.W)); + InputSystem.Update(); + + // tap should be canceled + currentTime = 2; + InputSystem.Update(); + + Assert.That(canceledInteraction, Is.TypeOf(typeof(TapInteraction))); + Assert.That(performedInteraction, Is.Null); + canceledInteraction = null; + + // hold should be trigered + currentTime = 4; + InputSystem.Update(); + Assert.That(canceledInteraction, Is.Null); + Assert.That(performedInteraction, Is.TypeOf(typeof(HoldInteraction))); + performedInteraction = null; + + // hold should be canceled + currentTime = 5; + InputSystem.QueueStateEvent(keyboard, new KeyboardState()); + InputSystem.Update(); + Assert.That(canceledInteraction, Is.TypeOf(typeof(HoldInteraction))); + Assert.That(performedInteraction, Is.Null); + canceledInteraction = null; + + // Should be no other remaining events + currentTime = 10; + InputSystem.Update(); + Assert.That(canceledInteraction, Is.Null); + Assert.That(performedInteraction, Is.Null); + + // PressRelease AW trigger a tap to ensure that is still working + currentTime = 11; + InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.A, Key.W)); + InputSystem.Update(); + + Assert.That(canceledInteraction, Is.Null); + Assert.That(performedInteraction, Is.Null); + + currentTime = 11.01; + InputSystem.QueueStateEvent(keyboard, new KeyboardState()); + InputSystem.Update(); + + Assert.That(canceledInteraction, Is.Null); + Assert.That(performedInteraction, Is.TypeOf(typeof(TapInteraction))); + performedInteraction = null; + canceledInteraction = null; + + // Should be no other remaining events + currentTime = 100; + InputSystem.Update(); + Assert.That(canceledInteraction, Is.Null); + Assert.That(performedInteraction, Is.Null); + } + [Test] [Category("Actions")] public void Actions_WithMultipleComposites_CancelsIfCompositeIsReleased() diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md index 7ee8b5165f..bec337c5b5 100644 --- a/Packages/com.unity.inputsystem/CHANGELOG.md +++ b/Packages/com.unity.inputsystem/CHANGELOG.md @@ -8,6 +8,11 @@ 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] - yyyy-mm-dd + +### Fixed +- Fixed Multiple interactions could breaks on Composite Binding. [ISXB-619](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-619) + ## [1.11.0] - 2024-09-10 ### Fixed diff --git a/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs b/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs index ae66343dfd..2784abb86e 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs @@ -2313,12 +2313,14 @@ internal void ChangePhaseOfInteraction(InputActionPhase newPhase, ref TriggerSta // Exception: if it was performed and we're to remain in started state, set the interaction // to started. Note that for that phase transition, there are no callbacks being // triggered (i.e. we don't call 'started' every time after 'performed'). - if (newPhase == InputActionPhase.Performed && actionStates[actionIndex].interactionIndex != trigger.interactionIndex) - { - // We performed but we're not the interaction driving the action. We want to stay performed to make - // sure that if the interaction that is currently driving the action cancels, we get to perform - // the action. If we go back to waiting here, then the system can't tell that there's another interaction - // ready to perform (in fact, that has already performed). + if (newPhase == InputActionPhase.Performed && + actionIndex != -1 && !actionStates[actionIndex].isPerformed && + actionStates[actionIndex].interactionIndex != trigger.interactionIndex) + { + // If the action was not already performed and we performed but we're not the interaction driving the action. + // We want to stay performed to make sure that if the interaction that is currently driving the action + // cancels, we get to perform the action. If we go back to waiting here, then the system can't tell + // that there's another interaction ready to perform (in fact, that has already performed). } else if (newPhase == InputActionPhase.Performed && phaseAfterPerformed != InputActionPhase.Waiting) { diff --git a/Packages/com.unity.inputsystem/InputSystem/Actions/InputBindingResolver.cs b/Packages/com.unity.inputsystem/InputSystem/Actions/InputBindingResolver.cs index f10eb7c974..8bfc195e9e 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Actions/InputBindingResolver.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Actions/InputBindingResolver.cs @@ -320,25 +320,37 @@ public unsafe void AddActionMap(InputActionMap actionMap) } // Instantiate interactions. - var interactionString = unresolvedBinding.effectiveInteractions; - if (!string.IsNullOrEmpty(interactionString)) + if (isPartOfComposite) { - // Add interactions from binding. - firstInteractionIndex = InstantiateWithParameters(InputInteraction.s_Interactions, interactionString, - ref interactions, ref totalInteractionCount, actionMap, ref unresolvedBinding); - if (firstInteractionIndex != InputActionState.kInvalidIndex) - numInteractions = totalInteractionCount - firstInteractionIndex; + // Composite's part use composite interactions + if (currentCompositeBindingIndex != InputActionState.kInvalidIndex) + { + firstInteractionIndex = bindingStatesPtr[currentCompositeBindingIndex].interactionStartIndex; + numInteractions = bindingStatesPtr[currentCompositeBindingIndex].interactionCount; + } } - if (!string.IsNullOrEmpty(action.m_Interactions)) + else { - // Add interactions from action. - var index = InstantiateWithParameters(InputInteraction.s_Interactions, action.m_Interactions, - ref interactions, ref totalInteractionCount, actionMap, ref unresolvedBinding); - if (index != InputActionState.kInvalidIndex) + var interactionString = unresolvedBinding.effectiveInteractions; + if (!string.IsNullOrEmpty(interactionString)) + { + // Add interactions from binding. + firstInteractionIndex = InstantiateWithParameters(InputInteraction.s_Interactions, interactionString, + ref interactions, ref totalInteractionCount, actionMap, ref unresolvedBinding); + if (firstInteractionIndex != InputActionState.kInvalidIndex) + numInteractions = totalInteractionCount - firstInteractionIndex; + } + if (!string.IsNullOrEmpty(action.m_Interactions)) { - if (firstInteractionIndex == InputActionState.kInvalidIndex) - firstInteractionIndex = index; - numInteractions += totalInteractionCount - index; + // Add interactions from action. + var index = InstantiateWithParameters(InputInteraction.s_Interactions, action.m_Interactions, + ref interactions, ref totalInteractionCount, actionMap, ref unresolvedBinding); + if (index != InputActionState.kInvalidIndex) + { + if (firstInteractionIndex == InputActionState.kInvalidIndex) + firstInteractionIndex = index; + numInteractions += totalInteractionCount - index; + } } } From 2160812e295794d6cfac52450f9066d0c8298891 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Malrat?= Date: Fri, 13 Sep 2024 09:21:37 -0400 Subject: [PATCH 2/2] addressed pr comments --- Assets/Tests/InputSystem/CoreTests_Actions.cs | 20 +++++++++---------- .../InputSystem/Actions/InputActionState.cs | 4 ++-- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/Assets/Tests/InputSystem/CoreTests_Actions.cs b/Assets/Tests/InputSystem/CoreTests_Actions.cs index 799e98a0dd..ba26bc52b6 100644 --- a/Assets/Tests/InputSystem/CoreTests_Actions.cs +++ b/Assets/Tests/InputSystem/CoreTests_Actions.cs @@ -10080,7 +10080,7 @@ public void Actions_WithCompositeWithMultipleInteractions_Works() Assert.That(canceledInteraction, Is.Null); Assert.That(performedInteraction, Is.Null); - currentTime = 0.01; + currentTime += InputSystem.settings.defaultTapTime / 2.0f; // half of the tap time to ensure that it performs. InputSystem.QueueStateEvent(keyboard, new KeyboardState()); InputSystem.Update(); @@ -10090,27 +10090,27 @@ public void Actions_WithCompositeWithMultipleInteractions_Works() performedInteraction = null; // Long PressRelease AW trigger a hold - currentTime = 1; + currentTime += 1; InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.A, Key.W)); InputSystem.Update(); // tap should be canceled - currentTime = 2; + currentTime += InputSystem.settings.defaultTapTime * 4.0; InputSystem.Update(); Assert.That(canceledInteraction, Is.TypeOf(typeof(TapInteraction))); Assert.That(performedInteraction, Is.Null); canceledInteraction = null; - // hold should be trigered - currentTime = 4; + // After (defaultTapTime*4 + 2) seconds hold should be performed with duration=2 + currentTime += 2; InputSystem.Update(); Assert.That(canceledInteraction, Is.Null); Assert.That(performedInteraction, Is.TypeOf(typeof(HoldInteraction))); performedInteraction = null; // hold should be canceled - currentTime = 5; + currentTime += 1; InputSystem.QueueStateEvent(keyboard, new KeyboardState()); InputSystem.Update(); Assert.That(canceledInteraction, Is.TypeOf(typeof(HoldInteraction))); @@ -10118,20 +10118,20 @@ public void Actions_WithCompositeWithMultipleInteractions_Works() canceledInteraction = null; // Should be no other remaining events - currentTime = 10; + currentTime += 5; InputSystem.Update(); Assert.That(canceledInteraction, Is.Null); Assert.That(performedInteraction, Is.Null); // PressRelease AW trigger a tap to ensure that is still working - currentTime = 11; + currentTime += 1; InputSystem.QueueStateEvent(keyboard, new KeyboardState(Key.A, Key.W)); InputSystem.Update(); Assert.That(canceledInteraction, Is.Null); Assert.That(performedInteraction, Is.Null); - currentTime = 11.01; + currentTime += InputSystem.settings.defaultTapTime / 2.0f; InputSystem.QueueStateEvent(keyboard, new KeyboardState()); InputSystem.Update(); @@ -10141,7 +10141,7 @@ public void Actions_WithCompositeWithMultipleInteractions_Works() canceledInteraction = null; // Should be no other remaining events - currentTime = 100; + currentTime += 100; InputSystem.Update(); Assert.That(canceledInteraction, Is.Null); Assert.That(performedInteraction, Is.Null); diff --git a/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs b/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs index 2784abb86e..3ba1503d49 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs @@ -2215,7 +2215,7 @@ internal void ChangePhaseOfInteraction(InputActionPhase newPhase, ref TriggerSta // See if it affects the phase of an associated action. var actionIndex = bindingStates[bindingIndex].actionIndex; // We already had to tap this array and entry in ProcessControlStateChange. - if (actionIndex != -1) + if (actionIndex != kInvalidIndex) { if (actionStates[actionIndex].phase == InputActionPhase.Waiting) { @@ -2314,7 +2314,7 @@ internal void ChangePhaseOfInteraction(InputActionPhase newPhase, ref TriggerSta // to started. Note that for that phase transition, there are no callbacks being // triggered (i.e. we don't call 'started' every time after 'performed'). if (newPhase == InputActionPhase.Performed && - actionIndex != -1 && !actionStates[actionIndex].isPerformed && + actionIndex != kInvalidIndex && !actionStates[actionIndex].isPerformed && actionStates[actionIndex].interactionIndex != trigger.interactionIndex) { // If the action was not already performed and we performed but we're not the interaction driving the action.