Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 106 additions & 0 deletions Assets/Tests/InputSystem/CoreTests_Actions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing a test with this quite elaborate sequence which I believe is key to avoid regressive behaviour when there are actually interdependencies between the interaction patterns

{
// Will ensure that :
// PressRelease AW trigger a tap
// Long PressRelease AW trigger a hold
// PressRelease AW trigger a tap

var keyboard = InputSystem.AddDevice<Keyboard>();
var action = new InputAction();
action.AddCompositeBinding("Dpad", interactions: "tap,hold(duration=2)")
.With("Up", "<Keyboard>/w")
.With("Down", "<Keyboard>/s")
.With("Left", "<Keyboard>/a")
.With("Right", "<Keyboard>/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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fine as is since I would image default would never go that far

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the test I initialized hold with hold(duration=2) I will add some comments

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()
Expand Down
3 changes: 3 additions & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ 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)

### Changed
- Renamed editor Resources directories to PackageResources to fix package validation warnings.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Would prefer actionIndex >= 0 for readability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to use the kInvalidIndex

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)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}

Expand Down