-
Notifications
You must be signed in to change notification settings - Fork 328
FIX: Fixed multiple interactions on composite bindings (ISXB-619) #2000
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX: Fixed multiple interactions on composite bindings (ISXB-619) #2000
Conversation
b1244fa
to
7a8dcc2
Compare
Will check this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, god job on figuring this one out, some minor comments.
Assert.That(canceledInteraction, Is.Null); | ||
Assert.That(performedInteraction, Is.Null); | ||
|
||
currentTime = 0.01; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For maintainability maybe use defaultTapTime
? https://docs.unity3d.com/Packages/[email protected]/api/UnityEngine.InputSystem.InputSettings.html#UnityEngine_InputSystem_InputSettings_defaultTapTime
performedInteraction = null; | ||
|
||
// Long PressRelease AW trigger a hold | ||
currentTime = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
// Ensure that https://jira.unity3d.com/browse/ISXB-619 regress | ||
[Test] | ||
[Category("Actions")] | ||
public void Actions_WithCompositeWithMultipleInteractions_Works() |
There was a problem hiding this comment.
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
// 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 && |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Please check the iOS CI failures |
Description
When composite bindings contains multiple interactions the interaction on each sub part could enter a bad state and stop functioning.
The user use case with tap,hold on WASD. stop working after a hold of WD. All subsequent press will be a hold even if it was a tap.
Changes made
Testing
new test + local testing of the customer project
Risk
Please describe the potential risks of your changes for the reviewers.
Checklist
Before review:
Changed
,Fixed
,Added
sections.Area_CanDoX
,Area_CanDoX_EvenIfYIsTheCase
,Area_WhenIDoX_AndYHappens_ThisIsTheResult
.During merge:
NEW: ___
.FIX: ___
.DOCS: ___
.CHANGE: ___
.RELEASE: 1.1.0-preview.3
.After merge: