FIX: handled events still triggering actions when switching PlayerInput#2340
Open
AswinRajGopal wants to merge 6 commits intodevelopfrom
Open
FIX: handled events still triggering actions when switching PlayerInput#2340AswinRajGopal wants to merge 6 commits intodevelopfrom
AswinRajGopal wants to merge 6 commits intodevelopfrom
Conversation
Contributor
PR Reviewer Guide 🔍(Review updated until commit 94564dc)Here are some key observations to aid the review process:
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
Contributor
PR Code Suggestions ✨🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2340 +/- ##
========================================
Coverage 77.95% 77.96%
========================================
Files 476 476
Lines 97453 97423 -30
========================================
- Hits 75971 75952 -19
+ Misses 21482 21471 -11
🚀 New features to boost your workflow:
|
AswinRajGopal
requested review from
MorganHoarau,
UnityAnthony and
jfreire-unity
Pauliusd01
requested review from
K-Tone and
Pauliusd01
and removed request for
UnityAnthony
Contributor
Test Plan
Summary of Changes & Risk AssessmentSummary of ChangesThis PR fixes a bug where physical button presses that generate multiple input events (common on controllers like DualSense) would still trigger actions even if the first event was marked as Risk Assessment
Test ScenariosFunctional Testing
Regression Testing
🔍 Regression Deep Dive (additional risks identified)
Edge Cases
💡 This test plan updates automatically when 🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
…n child objects" This reverts commit ee9aace.
Contributor
|
Persistent review updated to latest commit 94564dc |
| if (m_InputEventHandledPolicy == InputEventHandledPolicy.SuppressStateUpdates && | ||
| currentEventReadPtr->handled) | ||
| if (DelegateHelpers.InvokeCallbacksSafeUntilHandled(ref m_EventListeners, currentEventPtr, device, k_InputOnEventMarker, "InputSystem.onEvent", | ||
| m_InputEventHandledPolicy == InputEventHandledPolicy.SuppressStateUpdates)) |
Collaborator
There was a problem hiding this comment.
Are we saying here that if a single listener handles the event, we don't let other listeners process the event? Should this be part of the listener logic instead? Otherwise it's a unclear for the user why was their method hooked on InputSystem.onEvent not being triggered 🤔
Comment on lines
+1341
to
+1355
| new int[] { 0, 0, 0, 0, 1}, // started | ||
| new int[] { 0, 0, 0, 0, 1}, // performed | ||
| new int[] {0, 0, 0, 0, 0})] // cancelled | ||
| // Press event is not detected in step 1/2 with default interaction | ||
| [TestCase(InputEventHandledPolicy.SuppressActionEventNotifications, | ||
| null, | ||
| new int[] { 0, 0, 0, 0, 1}, | ||
| new int[] { 0, 0, 0, 0, 1}, | ||
| new int[] {0, 0, 0, 1, 1})] | ||
| // Press event is detected in step 2 (false positive) with explicit press interaction | ||
| // Press event is suppressed in step 1/2 with explicit press interaction | ||
| [TestCase(InputEventHandledPolicy.SuppressStateUpdates, | ||
| "press", | ||
| new int[] { 0, 0, 1, 1, 2}, | ||
| new int[] { 0, 0, 1, 1, 2}, | ||
| new int[] {0, 0, 0, 1, 1})] | ||
| new int[] { 0, 0, 0, 0, 1}, | ||
| new int[] { 0, 0, 0, 0, 1}, | ||
| new int[] {0, 0, 0, 0, 0})] |
Collaborator
There was a problem hiding this comment.
This looks like a behavior change. What is the reason to change this?
Comment on lines
+3899
to
+3922
| private void SuppressActionsForDevice(InputDevice device) | ||
| { | ||
| if (m_InputEventHandledPolicy != InputEventHandledPolicy.SuppressStateUpdates || device == null) | ||
| return; | ||
|
|
||
| var deviceIndex = device.m_DeviceIndex; | ||
| if (m_SuppressActionsForDeviceUpdate == null || deviceIndex >= m_SuppressActionsForDeviceUpdate.Length) | ||
| Array.Resize(ref m_SuppressActionsForDeviceUpdate, deviceIndex + 1); | ||
| m_SuppressActionsForDeviceUpdate[deviceIndex] = InputUpdate.s_UpdateStepCount; | ||
| } | ||
|
|
||
| internal bool ShouldSuppressActionsForDevice(InputDevice device) | ||
| { | ||
| if (m_InputEventHandledPolicy != InputEventHandledPolicy.SuppressStateUpdates || device == null) | ||
| return false; | ||
|
|
||
| var deviceIndex = device.m_DeviceIndex; | ||
| if (m_SuppressActionsForDeviceUpdate == null || deviceIndex >= m_SuppressActionsForDeviceUpdate.Length) | ||
| return false; | ||
|
|
||
| var lastUpdate = (int)m_SuppressActionsForDeviceUpdate[deviceIndex]; | ||
| var currentUpdate = (int)InputUpdate.s_UpdateStepCount; | ||
| return lastUpdate == currentUpdate || lastUpdate + 1 == currentUpdate; | ||
| } |
Collaborator
There was a problem hiding this comment.
I'm sure why this code is needed.. Can you clarify what is this doing and why we need it?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Bug: https://issuetracker.unity3d.com/issues/setting-inputeventptr-dot-handled-to-true-does-not-prevent-actions-from-triggering-when-changing-devices
When
InputUser.onUnpairedDeviceUsedmarkseventPtr.handled= true, we expect no action to fire from that samebutton press.
But on devices like DualSense, a single press can generate multiple input events. The handled flag only stops the
one event, and another event in the same press still triggers the action.
Root cause
The action system is driven by state change events. We can see different eventIds for the handled event vs the
action‑triggering event, so the action was coming from a separate input event.
Fix
InputSystem.onEvent, stop calling remaining listeners once handled becomes true.for the rest of the update (and next update).
InputActionState, skip processing control changes if the device is suppressed.Testing status & QA
Manually verified using repro project + dualsense controller.
Overall Product Risks
Comments to reviewers
SuppressStateUpdatesand a listener explicitly sets handled = true.the next event look like a new press which is now prevented.
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.