Skip to content

Conversation

@AswinRajGopal
Copy link
Contributor

@AswinRajGopal AswinRajGopal commented Jan 29, 2026

Description

Bug: https://issuetracker.unity3d.com/issues/setting-inputeventptr-dot-handled-to-true-does-not-prevent-actions-from-triggering-when-changing-devices
When InputUser.onUnpairedDeviceUsed marks eventPtr.handled = true, we expect no action to fire from that same
button 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 saw different eventIds for the handled event vs the
action‑triggering event, so the action was coming from a separate input event.

Fix

  • While dispatching InputSystem.onEvent, stop calling remaining listeners once handled becomes true.
  • When an event is handled (policy = SuppressStateUpdates), temporarily suppress action processing for that device
    for the rest of the update (and next update).
  • In InputActionState, skip processing control changes if the device is suppressed.

Testing status & QA

Manually verified using repro project + dualsense controller.

Overall Product Risks

  • Complexity: Low
  • Halo Effect: Low

Comments to reviewers

  • Only applies when policy is SuppressStateUpdates and a listener explicitly sets handled = true.
  • Normal input flow remains unchanged.
  • This simply makes “handled” cover the entire physical press, even if it arrives as multiple events.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

@cla-assistant-unity
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Aswin Gopal seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@u-pr
Copy link
Contributor

u-pr bot commented Jan 29, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

The PR addresses a specific logic issue with a contained fix, but introduces a utility method with a significant control flow bug that needs correction.
🏅 Score: 75

The logic to suppress actions for handled events appears correct and addresses the described bug. However, the newly added `InvokeCallbacksSafeUntilHandled` method has a bug where `ProfilerMarker.End()` is unreachable or skipped, which will corrupt profiling data.
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The ProfilerMarker.End() call (line 115) is unreachable because of the return false; statement immediately preceding it. Furthermore, when returning early (line 110), marker.End() is not called at all. This will lead to unbalanced Profiler samples (Begin without End).

        return true;
    }
}
callbacks.UnlockForChanges();
return false;
marker.End();
  • Update review

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

@u-pr
Copy link
Contributor

u-pr bot commented Jan 29, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix unbalanced profiler marker calls

The marker.End() call is currently unreachable in the return false path and missing
in the return true path, which will cause profiler imbalances. Ensure marker.End()
is called before returning in both scenarios.

Packages/com.unity.inputsystem/InputSystem/Utilities/DelegateHelpers.cs [107-116]

             if (stopOnHandled && eventPtr.handled)
             {
                 callbacks.UnlockForChanges();
+                marker.End();
                 return true;
             }
         }
         callbacks.UnlockForChanges();
+        marker.End();
         return false;
-        marker.End();
     }
Suggestion importance[1-10]: 9

__

Why: The suggested fix addresses a critical issue where marker.End() is skipped when returning early, and the existing marker.End() call is unreachable code. This ensures proper profiler balancing.

High
General
Prevent incorrect action suppression on startup

The m_SuppressActionsForDeviceUpdate array initializes with zeros. If
s_UpdateStepCount is 0 or 1 at startup, this logic will mistakenly suppress actions
for all devices (since 0 or 0+1 will match). Ignore the default 0 value to prevent
this.

Packages/com.unity.inputsystem/InputSystem/InputManager.cs [3919-3921]

         var lastUpdate = (int)m_SuppressActionsForDeviceUpdate[deviceIndex];
+        if (lastUpdate == 0)
+            return false;
+
         var currentUpdate = (int)InputUpdate.s_UpdateStepCount;
         return lastUpdate == currentUpdate || lastUpdate + 1 == currentUpdate;
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a logic flaw where the default zero-initialization of the m_SuppressActionsForDeviceUpdate array could cause false positives for action suppression during the initial updates (steps 0 and 1).

Medium
  • More suggestions

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant