Fix attempt for ISXB-1066 where we still process a hold interaction despite having a more dominant interaction #2055
Conversation
…espite having a more dominant interaction. Test case included.
|
Current state of the fix is rough, PR mostly to open up to discussion if solution is viable |
| var index = interactionStartIndex + i; | ||
| var state = interactionStates[index]; | ||
| var interaction = interactions[index]; | ||
| if (interaction is not HoldInteraction) |
There was a problem hiding this comment.
This might not be correct, all interaction could be affected depending on the settings.
User's custom interaction might also be affected.
There was a problem hiding this comment.
Could you elaborate please? I'm not sure I understand how they might be affected
| Assert.That(canceledInteraction, Is.Null); | ||
| Assert.That(performedInteraction, Is.Null); | ||
|
|
||
| currentTime = 0.01; |
There was a problem hiding this comment.
Minor, maybe have
// Release A, W
as comment to follow the pattern here for readability?
| InputSystem.Update(); | ||
| Assert.That(canceledInteraction, Is.Null); | ||
| Assert.That(performedInteraction, Is.Null); | ||
|
|
There was a problem hiding this comment.
Excessive white space, remove?
| Assert.That(canceledInteraction, Is.Null); | ||
| Assert.That(performedInteraction, Is.Null); | ||
|
|
||
| currentTime = 11.01; |
| InputSystem.Update(); | ||
|
|
||
| currentTime = 11.02; | ||
| InputSystem.QueueStateEvent(keyboard, new KeyboardState()); |
| @@ -1528,6 +1531,11 @@ private void ProcessControlStateChange(int mapIndex, int controlIndex, int bindi | |||
| if (interactionCount > 0 && !bindingStatePtr->isPartOfComposite) | |||
| { | |||
| ProcessInteractions(ref trigger, bindingStatePtr->interactionStartIndex, interactionCount); | |||
| // We still need to process hold interactions. If we don't, then we risk having a danling hold which triggers after all buttons are unpressed. | |||
There was a problem hiding this comment.
Why wouldn't the hold be allowed to trigger? Is it expected to be canceled?
| var index = interactionStartIndex + i; | ||
| var state = interactionStates[index]; | ||
| var interaction = interactions[index]; | ||
| if (interaction is not HoldInteraction) |
There was a problem hiding this comment.
The problem I have with this is that I calls out a specific implementation (HoldInteraction) and not a property of the model. Maybe what @bmalrat suggested is similar (not sure), that if something else is added as an extension, e.g. LongHoldInteraction, it breaks again?
Description
Issue stems from having one of the interaction release early, causing the other to become the dominant interaction, then said other interaction releases. When the first is released, the second takes over and the hold interaction is never processed, leaving it to trigger despite both buttons being released.
Proposed solution is to still process the first interaction to ensure the hold is released.
Testing status & QA
Test case provided under the name
Actions_WithMultipleBindingsAndMultipleInteractions_Works
Overall Product Risks
It fixes the issue but may create a different one.
For instance, if we have two buttons, A and W, bound to the same action, we can do
Press A & W
Release A
Press A
Release W
Since both holds were released, no hold will be triggered.
Comments to reviewers
Current state of the fix is rough, PR mostly to open up to discussion if solution is viable.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: