-
Notifications
You must be signed in to change notification settings - Fork 324
FIX: Tap Interaction is performed after it times out (ISXB-627) #2172
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
base: develop
Are you sure you want to change the base?
FIX: Tap Interaction is performed after it times out (ISXB-627) #2172
Conversation
489bd15
to
874397e
Compare
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.
Pull Request Overview
This PR fixes an issue where the Tap Interaction for analog controls (e.g. Gamepad sticks/triggers) would erroneously restart immediately after timing out.
- Added a flag (canceledFromTimerExpired) to prevent immediate reactivation after the timeout.
- Updated the Process method to incorporate this flag and reset it when appropriate.
- Updated the CHANGELOG to document the fix.
Reviewed Changes
Copilot reviewed 19 out of 23 changed files in this pull request and generated no comments.
File | Description |
---|---|
Packages/com.unity.inputsystem/InputSystem/Actions/Interactions/TapInteraction.cs | Added a timeout flag and logic to prevent restarting after tap timeout. |
Packages/com.unity.inputsystem/CHANGELOG.md | Updated the changelog with a description of the tap interaction fix. |
Files not reviewed (4)
- Assets/Samples/SimpleDemo/SimpleDemo_UsingPlayerInputSettings.lighting: Language not supported
- ProjectSettings/MemorySettings.asset: Language not supported
- ProjectSettings/MultiplayerManager.asset: Language not supported
- ProjectSettings/SceneTemplateSettings.json: Language not supported
Comments suppressed due to low confidence (1)
Packages/com.unity.inputsystem/InputSystem/Actions/Interactions/TapInteraction.cs:91
- Consider resetting the 'canceledFromTimerExpired' flag in the Reset() method to ensure that no stale state persists between interactions.
public void Reset()
Also adds a test to make sure the tap interaction doesn't restart.
874397e
to
c7c82e3
Compare
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.
Great fix, test case and changes looks good to me.
thanks for proactively approving before I requested the review! :) |
@Pauliusd01 ping me on Slack if you have any issues trying to reproduce the bug. Essentially, avoid the PlayerInput component usage since it's broken even without a fix (I'm not sure if we have a bug for it) |
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, previoulsy I'd get tap interactions on start and end of holding the trigger now I only get it at the start
Description
Fixes an issue surfaced by https://jira.unity3d.com/browse/ISXB-627
Tap interaction for analog controls such as Gamepad sticks and triggers would keep getting restarted and canceled once above the press point. And could potentially get
performed
after it's first timeout.Now, once the Tap Interactions timer expires, it can only be "restarted" once if goes below the release point.
Testing status & QA
I added a automated test to confirm the behavior and validated it with the ISX-627 project (adapted) as well.
Overall Product Risks
Comments to reviewers
Or alternatively, creating a project with an InputActionAsset that has 2 different actions for a Gamepad trigger: one with a Hold interaction and one with a Tap interaction and see that the problem doesn't reproduce.
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: