FIX: OnAnyButtonPress not recognizing touch anymore#2401
FIX: OnAnyButtonPress not recognizing touch anymore#2401
Conversation
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
The review identified several issues with the proposed changes to GetFirstButtonPressOrNull. Most critically, the touchscreen check causes repeated "press" events during movement, breaking the expected behavior of InputSystem.onAnyButtonPress. There are also concerns regarding architectural coupling and performance overhead in multi-touch scenarios.
🤖 Helpful? 👍/👎
Packages/com.unity.inputsystem/InputSystem/Controls/InputControlExtensions.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Controls/InputControlExtensions.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Controls/InputControlExtensions.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Controls/InputControlExtensions.cs
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2401 +/- ##
========================================
Coverage 78.13% 78.13%
========================================
Files 483 483
Lines 98770 98787 +17
========================================
+ Hits 77169 77191 +22
+ Misses 21601 21596 -5 Flags with carried forward coverage won't be shown. Click here to find out more.
|
ritamerkl
changed the title
|
Will most likely take a look tomorrow |
jfreire-unity
left a comment
jfreire-unity
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
My main concern is the same as already pointed by U-PR:
Hardcoding a specific device type check (is Touchscreen) in a generic extension method creates tight coupling
If we find a way to avoid this, let's do it instead
Pauliusd01
left a comment
Pauliusd01
left a comment
There was a problem hiding this comment.
I'm just updating my status that I'm waiting for expected changes
…evious state and if that was the same
|
Hold on with reviews, I messed up. |
ritamerkl
requested review from
ekcoh and
jfreire-unity
and removed request for
jfreire-unity
|
ready for review @jfreire-unity @Pauliusd01 |
3 participants
Description
This fixes a regression where onAnyButtonPress would not work for touch. This is the ticket.
Note that this was a regression from one of my PRs: https://github.com/Unity-Technologies/InputSystem/pull/2249/changes.
Touches can happen to have no previous states to compare to when they just started, so the line of code will fail. I corrected that by excluding touches. Touch is the only control that is "created on the go" where this happens in my mind, but let me know when I am mistaken here.
Testing status & QA
Added automated test, please also test in simulator, player and device.
Overall Product Risks
Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.
Comments to reviewers
Note that this was a regression from one of my PRs: https://github.com/Unity-Technologies/InputSystem/pull/2249/changes.
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.