Conversation
ekcoh
added
the
work in progress
ekcoh
changed the title
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2412 +/- ##
===========================================
+ Coverage 78.13% 78.16% +0.03%
===========================================
Files 483 761 +278
Lines 98770 139016 +40246
===========================================
+ Hits 77169 108668 +31499
- Misses 21601 30348 +8747 Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@u-pr Can you check if all previously raised concerns have been addressed in this PR? |
|
Yes, all previously raised concerns have been successfully addressed in the latest changes:
The new tests in 🤖 Helpful? 👍/👎 |
| // does not cause the action to start back up. For pass-through actions, that is different | ||
| // as *any* value change performs the action. So here, we see *both* a cancellation and then | ||
| // immediately a performing of the action. | ||
| // |
There was a problem hiding this comment.
This should be rephrased into a decision when decision exists after review/discussion.
ekcoh
changed the title
|
FYI: likely will only have time to take a look at this on the latter half of next week |
|
@Pauliusd01 I can do some more testing myself to smoke this further and reach out to XR. We also have a decision to make which might call for additional changes to this. |
ekcoh
removed
the
work in progress
I could not find info about that particular case in the doc. Even if it make sense, it's not obvious that "any value" also means system-synthetic event. Might be worth documenting this subtlety.
I do not think we should change it as some users might be relying on the existing behaviour. It also gives a chance to sync any user-owned state linked to that passthrough action. Though, users could also reset such state through the Was that a long standing issue or requested change of behaviour? I might be missing some context here. I'm just having a hard time figuring out the user story behind it. Even if getting rid of the |
MorganHoarau
left a comment
MorganHoarau
left a comment
There was a problem hiding this comment.
Just a few minor comments.
Good automated test coverage, every time I had ideas to improve a test, the next one was covering it.
I like that approach because it moves the suppression ownership to the appropriate layer (InputActionState.TriggerState) and makes the system more resilient without adding complexity.
| northAction.Enable(); | ||
|
|
||
| // Handle events that press buttonSouth, but let buttonNorth events through. | ||
| InputSystem.onEvent += (eventPtr, device) => |
There was a problem hiding this comment.
Not sure to understand why we need to subscribe an empty lambda here...
There was a problem hiding this comment.
Good catch, something interesting happened here, will revisit :)
| new GamepadState().WithButton(GamepadButton.South)); // new press | ||
| InputSystem.Update(); | ||
|
|
||
| Assert.That(performedCount, Is.EqualTo(1), |
There was a problem hiding this comment.
Should not be needed because covered by other test in other scenarios, but I would have added the check for Assert.That(action.WasPressedThisFrame(), Is.True); to get the expected symmetry and specific scenario covered.
There was a problem hiding this comment.
Agreed, good catch, I'll get it added
| ### Fixed | ||
|
|
||
| - Fixed `InputEventPtr.handled` not preventing actions from triggering when switching devices. The default event handled policy has been changed from `SuppressStateUpdates` (now deprecated) to `SuppressActionEventNotifications`, which keeps device state synchronized while suppressing action callbacks for handled events. [ISXB-1097](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1097) | ||
| - Fixed all `InputAction.WasXxxThisFrame()` and `WasXxxThisDynamicUpdate()` polling APIs to use per-action suppression tracking instead of a map-wide flag. Previously, when multiple events arrived in the same frame with mixed handled/unhandled states, the last event's suppression state could incorrectly affect all actions in the map. [ISXB-1097](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1097) |
There was a problem hiding this comment.
Move to Changed section?
There was a problem hiding this comment.
Agreed on the first one, the second one seems to instead be a long lasting bug which gave different result deepening on what map and action belonged to.
This comment was marked as off-topic.
This comment was marked as off-topic.
Test Plan(Custom_review updated until commit c0924e6)Updated: 2024-05-14 - Expanded regression scenarios for polling APIs and verified device reset side-effects.
Summary of Changes & Risk AssessmentSummary of ChangesThis PR resolves ISXB-1097 where handled events failed to suppress actions during device switching. The fix changes the default 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 |
Fair point about the fact that projects in the wild could be relying on the side-effect of an additional Performed callback (non-consistent) for Passthrough. That is why I didn't change that part now but ensured its called out in the test. Agree we could potentially also clarify this in the manual or scripting manual since it's undocumented - and honestly not something I was aware of before this PR. Since it is inconsistent I am not sure its intentional or a mistake from the past that it performs differently, personally I suspect the latter since it is called out as a "quirk" in the test case - which made want to raise this to discuss since its also not documented. I am not sure what a user would even though with the additional Performed callback carrying a false synthetic state. There is no request for change of behavior - I am just calling it out since it makes no sense to me. I am completely fine keeping it as is for now. @MorganHoarau Did I understand it correctly that you where ok with the bigger behaviour change of this PR I am suggesting - changing the default event consumption mode? |
|
I notice now this PR looks kind of broken after it was merged with latest changes from develop :/ |
…ent handling policy SuppressStateUpdates since broken by design.
…rame, wasPerformedThisFrame, wasReleasedThisFrame, wasCompletedThisFrame.
…spect to event suppression.
…ndledPolicy.cs Co-authored-by: u-pr[bot] <205906871+u-pr[bot]@users.noreply.github.com>
…stead of per action.
ekcoh
force-pushed
the
isxb-1097-event-suppression-fix
branch
from
cb8829c to
c0924e6
Compare
|
Fixed previous bad merge, this now ok to review and test again |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Persistent custom_review updated to latest commit c0924e6 |
3 participants
Description
Summary
Fixes ISXB-1097: Setting
InputEventPtr.handled = truedoes not prevent actions from triggering when switching devices.Root Cause
The default event handled policy (
SuppressStateUpdates) discards handled events entirely, including their state updates. This desynchronizes the Input System's internal device state from the actual source state. When the user switches to a different device, the non-handled event arrives while the previous device still has stale state, causing spurious action triggers.Fix
SuppressStateUpdatestoSuppressActionEventNotifications. Under the new default, handled events still propagate state updates (keeping device state synchronized) but suppress action callback notifications (Started/Performed/Canceled). The pull-based action APIs (WasPressedThisFrame,WasPerformedThisFrame) are also gated by the suppression flag and returnfalsefor suppressed events.SuppressStateUpdateswith[Obsolete](warning, not error) and an explanatory message. Existing code continues to compile but will receive warnings.handledflag inResetDevice: the synthetic reset event usedeventId = -1as a sentinel, which set all bits including the handled flag (bit 31 ofeventId). Changed toInputEvent.InvalidEventIdso the handled bit is not accidentally set.RebindingOperationto only override the global event handled policy when the user makes an explicit choice viaWithActionEventNotificationsBeingSuppressed(). Previously, the uninitialized field defaulted toSuppressStateUpdates, silently overriding whatever policy was configured globally.WasXxxThisFrame()andWasXxxThisDynamicUpdate()polling APIs now check a per-actionSuppressedflag onTriggerState(bit 7 of the existing flags byte) instead of a sharedm_Suppressedbool onInputActionState. The flag is stamped when each action's phase changes, so it correctly reflects whether the specific event that triggered that action was handled. This fixes false positives/negatives when multiple events with mixed handled/unhandled states arrive in the same frame. Also fixesWasReleasedThisFrame(),WasCompletedThisFrame(), and theirDynamicUpdatevariants which previously did not check suppression at all.Behavioral Changes
m_Suppressed)control.isPressedafter handled pressfalsetrue(reflects actual device state)action.WasPressedThisFrame()after handled pressfalse(no state update)false(gated byIsSuppressed)action.WasReleasedThisFrame()after handled releasefalsefalse(now gated byIsSuppressed)action.WasCompletedThisFrame()after handled eventfalsefalse(now gated byIsSuppressed)Migration for Existing Projects
Existing projects are not affected by this change. The event handled policy is serialized as part of
InputManagerstate, so projects that were using the old default (SuppressStateUpdates) will retain it upon deserialization.To opt in to the new behavior in an existing project, set the policy explicitly:
Or equivalently:
Projects explicitly using
SuppressStateUpdateswill receive an[Obsolete]compiler warning guiding them to switch.Open Design Question
When a device is reset (
InputSystem.ResetDevice), pass-through actions currently emit bothCanceledandPerformed(0f)— because pass-through actions fire on any value change, including the reset-to-default. This is consistent with the pass-through contract but inconsistent with button/value actions (which only emitCanceled). If we decide to suppress thePerformed(0f)on reset, the synthetic reset event should be explicitly marked ashandledrather than relying on sentinel values. See inline design note inCoreTests_Actions.cs.Testing status & QA
Have run editor, playmode and player tests locally to verify functional behaviour essentially verifying:
Events_HandledEventsShouldNotTriggerActionsWhenSwitchingDevicespasses (direct ISXB-1097 regression test — device switching)Events_HandledPressEdgeInMultiEventFrameShouldNotTriggerActionspasses (ISXB-1097 regression test — multi-event frame, e.g. DualSense 600 Hz)Events_AllWasXxxThisFrameAPIsRespectEventSuppressionpasses (ISXB-1097 regression test — all WasXxx polling APIs gated by per-action suppression)Events_PerActionSuppressionWithMixedHandledEventspasses (ISXB-1097 regression test — mixed handled/unhandled events in same frame, per-action correctness)EventHandledPolicy_ShouldReflectUserSettingpasses with updated defaultsActions_InteractiveRebinding_CanSuppressEventsWhileListeningpasses for bothSuppressStateUpdatesandSuppressActionEventNotificationspoliciesActions_ResettingDevice_CancelsOngoingActionsThatAreDrivenByItpasses (pass-through Performed(0f) behavior)Did basic testing with Rebinding example.
Tested repro case associated with ticket with DualSense remains to be done.
Suggest that before merge we also cover:
Overall Product Risks
Comments to reviewers
Apart from reviewing solution and test cases I also want us to decide on Open Design Question. Not emitting Performed makes much more sense and makes value types consistent in my opinion, but it would imply another behaviour change.
Checklist
Before review:
Changed,Fixed,Addedsections.Events_HandledEventsShouldNotTriggerActionsWhenSwitchingDevices,Events_HandledPressEdgeInMultiEventFrameShouldNotTriggerActions,Events_AllWasXxxThisFrameAPIsRespectEventSuppression,Events_PerActionSuppressionWithMixedHandledEventsDuring merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.