FIX: OnAnyButtonPress not recognizing touch anymore#2401
FIX: OnAnyButtonPress not recognizing touch anymore#2401
Conversation
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? 👍/👎
| foreach (var control in eventPtr.EnumerateControls(Enumerate.IgnoreControlsInDefaultState, magnitudeThreshold: magnitude)) | ||
| { | ||
| if (!control.HasValueChangeInEvent(eventPtr)) | ||
| if (!control.HasValueChangeInEvent(eventPtr) && !(control.device is Touchscreen)) // touches can happen to not have a previous state to compare to after the touch started |
There was a problem hiding this comment.
The condition !(control.device is Touchscreen) bypasses the value change check for all touchscreen controls. This causes GetFirstButtonPressOrNull to return a control even if its state hasn't changed in the current event (e.g., during a touch move event where the finger is already down).
Since InputSystem.onAnyButtonPress triggers based on the return value of this method, it will now fire repeatedly for every touch move event as long as the finger is held down, rather than only on the initial press. This breaks the expected behavior of a "press" event and differs from how other devices (like Gamepads or Keyboards) behave.
If the goal is to handle the case where HasValueChangeInEvent fails due to missing previous state, consider checking if the control is specifically transitioning to a pressed state or if the event represents a TouchPhase.Began equivalent.
🤖 Helpful? 👍/👎
| foreach (var control in eventPtr.EnumerateControls(Enumerate.IgnoreControlsInDefaultState, magnitudeThreshold: magnitude)) | ||
| { | ||
| if (!control.HasValueChangeInEvent(eventPtr)) | ||
| if (!control.HasValueChangeInEvent(eventPtr) && !(control.device is Touchscreen)) // touches can happen to not have a previous state to compare to after the touch started |
There was a problem hiding this comment.
Hardcoding a specific device type check (is Touchscreen) in a generic extension method creates tight coupling. This approach also requires updating this method if other devices (e.g., Pens or specialized VR controllers) exhibit similar state-handling characteristics.
Have you considered moving this logic into HasValueChangeInEvent itself? Since the root issue is that touches may not reliably report a value change via memory comparison when starting, fixing it at the source would ensure consistent behavior for all consumers of the HasValueChangeInEvent API and keep this extension method device-agnostic.
🤖 Helpful? 👍/👎
| foreach (var control in eventPtr.EnumerateControls(Enumerate.IgnoreControlsInDefaultState, magnitudeThreshold: magnitude)) | ||
| { | ||
| if (!control.HasValueChangeInEvent(eventPtr)) | ||
| if (!control.HasValueChangeInEvent(eventPtr) && !(control.device is Touchscreen)) // touches can happen to not have a previous state to compare to after the touch started |
There was a problem hiding this comment.
The type check control.device is Touchscreen is performed for every control in the loop where HasValueChangeInEvent is false. Since all controls in a state event belong to the same device, this check can be moved outside the loop for better performance.
Additionally, bypassing the HasValueChangeInEvent filter for all touchscreen controls means that during every move event, every non-default control on the touchscreen (including other active touches) will undergo magnitude and button checks. In multi-touch scenarios, this could introduce unnecessary overhead.
🤖 Helpful? 👍/👎
| foreach (var control in eventPtr.EnumerateControls(Enumerate.IgnoreControlsInDefaultState, magnitudeThreshold: magnitude)) | ||
| { | ||
| if (!control.HasValueChangeInEvent(eventPtr)) | ||
| if (!control.HasValueChangeInEvent(eventPtr) && !(control.device is Touchscreen)) // touches can happen to not have a previous state to compare to after the touch started |
There was a problem hiding this comment.
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2401 +/- ##
===========================================
+ Coverage 77.92% 78.10% +0.18%
===========================================
Files 482 483 +1
Lines 97755 98775 +1020
===========================================
+ Hits 76175 77149 +974
- Misses 21580 21626 +46 Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.