Skip to content

NEW: (ISXB-1524, ISX-2325, revisiting ISXB-1396) Fix for rebinding issues related to incorrect event suppression + Changed Rebinding Sample #2168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 60 commits into
base: develop
Choose a base branch
from

Conversation

ekcoh
Copy link
Collaborator

@ekcoh ekcoh commented Apr 16, 2025

Description

Fix originates from users reporting issues with rebinding functionality and differences between Xbox gamepad and
DualShock/DualSense (ISXB-1396 - which this PR basically undo changes from: https://github.com/Unity-Technologies/InputSystem/pull/2137/files). The problem surfaces when a control, e.g. button is targeted in rebinding that is already associated with an action doing something else outside the context of rebinding, but generally unintentional triggering of actions in relation to the rebinding process.

According to support engineers and customer, improved rebinding UI sample (commit which was improved to address this issue works well with Xbox gamepad but not with Sony gamepads. This have been verified on Windows. On macOS also Xbox gamepads have issues.
It turns out the difference between Xbox and DualShock/DualSense has nothing to do with the
gamepads but is a function of the underlying backend behaviour and/or filtering behaviour since lack of filtering will generate new samples solely based on analog control noise and repeat button states effectively bypassing current event suppression mechanism. This is also true if periodic state readings are being made without change filtering. This problem was filed as ISXB-1524. Additionally the current rebinding sample hides these issues due to poor visibility and little resemblance with rebinding menus in real games. This was filed as ISX-2325.

The problem
The existing InputEvent.handled (currently used to suppress events during "listening") flags marks an event as being "handled" which blocks event/state propagation. This is problematic if the associated InputEvent is not representing an event but a state reading. The reason for this can be shown by the example below. Consider an initial state where a button is in state 0 (not pressed), this corresponds to some event 'a'. A press event 'b' changes the state of the button to 1, but this event is for some reason suppressed, for example during rebinding. This blocks the event from propagating as a state change which is according to documentation of InputEventPtr.handled. This leads to "event" 'c' which is just a periodic reading that doesn't reflect a change at all to be seen as a change since 'b' was suppressed but 'c' wasn't suppressed. This implies that event 'c' would evaluate to a "press" since it is a state change from the perspective of the action state, but not from the state of the button. This is illustrated in figure below.

-0----1------1----0----------------> time (Button state in reading)
-0----0------1----0----------------> time (Action state)
-a----b------c----d----------------> time (Events)
------s----------------------------> suppression
   
      ^      ^
      |      |
      |      Logical press detection happens here due to suppression
      |
      Actual press happens here

If suppression instead had been allowed to propagate to update state, but not fire notifications outside internal data representation, the scenario would have been different since event/reading 'c' would not imply a state change:

-0----1------1----0----------------> time (Button state in reading)
-0----1------1----0----------------> time (Action state)
-a----b------c----d----------------> time (Events)
------s----------------------------> suppression
   
      ^
      |
      |
      |
      Actual press happens here but notification is suppressed

Previous possible work around (Should not be used - will not work)

A slight improvement was suggested to affected users based on OnMatchWaitForAnother(x), which seems to solve
the issue if time x is large enough (larger than a frame cycle). The reason this suggestion seem to work is due to input buffers being emptied/swapped on frame boundaries, typically leading to release by user before timeout. Since this functionality is time-based it do not provide a reliable solution device agnostic solution and this workaround should not be used in production code. This is easily seen by using this function and pressing a button in rebinding UI and never releasing it. As soon as suppression stops, the action bound to the rebind target will still fire.

Proposed solution

It's currently assumed that event suppression logic in Input System is incorrect and hence it is being investigated as part of this PR. Essentially event suppression should only suppress events from firing but still allow state propagation updates since otherwise it is just postponing/deferring the problem. It's difficult to change this behavior without risking changing behaviour with side-effects in existing projects. Hence the current working assumption is that this need to be a setting and/or runtime property of the system to allow switching modes. Such a solution could temporarily switch mode during rebinding to solve the reported problem which is what has been implemented on this PR. This have been implemented as InputManager.inputEventHandledPolicy (internal) property to control how the system processes handled events and the RebindingUI sample have been updated to use this via the new fluent-API extensions WithSuppressedActionPropagation() which is the only public API introduced by this PR at this point. This is now used in the sample which has also been reworked to give better visibility related to actions firing and now uses a "game mode" and a "menu mode" for mimic a real game better.

It has been proven that uncommenting suppression early-exit within InputManager.OnUpdate eliminates an action callback when e.g. holding a button during rebinding, even when using OnMatchWaitForAnother(x). This indicates that we need to allow interaction FSMs to update based on state changes but their output should be suppressed. This implies we either need to modify behavior of current suppress flag or introduce yet another suppress mechanism. On this PR the rebinding suppress actions only while listening to avoid breaking existing games using the existing solution. Hence the WithSuppressedActionPropagation is an opt-in.

Note that this PR adds 1 new API and hence also updates minor version of the Input System to v1.15.0.

Updated rebinding sample

The rebinding sample has been updated and should be tested as part of this PR (It was required to properly investigate and fix this bug and sample code. Note that screenshots may not be up to date with final solution. Apart from using the new solution to avoid side-effects during rebinding the sample now also has a "game mode" allowing you to move, rotate (look), change color (interact) and change shape (use) to mimic trivial game like features. When pressing default ESC or Start an in-game menu is shown allowing you to do rebinding. Also note that UI colours and navigation links where fixed to allow running the sample without a mouse or keyboard present.

Rebinding sample game mode:
Screenshot 2025-06-25 at 11 03 57

Rebinding sample rebinding menu mode:
Screenshot 2025-06-25 at 11 05 49

Note that look (mouse delta) have been added for completeness even if a user may decide to not include it and there is typically no other delta controls available with common desktop equipment.

Testing status & QA

The current fix has been tested on macOS and Windows 10 and Window 11 using Xbox gamepad, DualShock gamepad and DualSense gamepad. Additionally a rather complex parameterised test Events_ShouldRespectHandledPolicyUponUpdateAndSuppressedPressTransition have been added testing expected outcomes for both InputEventSuppressPolicy.SuppressEventsand InputEventSuppressPolicy.SuppressActions for press and release interactions, default interactions as well as action polled event evaluations for press and release. Note that polled state events will not be affected by this setting.

Overall Product Risks

  • Complexity: Medium
  • Halo Effect: Small - Event suppression is policy is only changed in the context of rebinding "Wait/listen" mode and then reverts back to previous behavior.

Comments to reviewers

Main effort should be on solving customer issue but also to look for regressed behavior due to changer in event suppression logic.

The easiest way I have found to test this is as follows:

  1. Create a debug script to get observability of phases on some action, e.g.
// ActionDebug.cs
using UnityEngine;
using UnityEngine.InputSystem;

public class ActionDebug : MonoBehaviour
{
    public InputActionReference trigger;

    // Start is called once before the first execution of Update after the MonoBehaviour is created
    void Start()
    {
        trigger.action.performed += ActionOnPerformed;
        trigger.action.canceled += ActionOnCanceled;
        trigger.action.started += ActionOnStarted;
        trigger.action.Enable();
    }

    private void ActionOnStarted(InputAction.CallbackContext obj)
    {
        Debug.Log("Action Started");
    }

    private void ActionOnCanceled(InputAction.CallbackContext obj)
    {
        Debug.Log("Action Canceled");
    }

    private void ActionOnPerformed(InputAction.CallbackContext obj)
    {
        Debug.Log("Action Performed");
    }

    // Update is called once per frame
    void Update()
    {
        if (trigger.action.WasPerformedThisFrame())
            Debug.Log("Action Performed (Polled Event)");
    }
}
  1. Update the input action asset in the RebindingUI sample to have an action "Test" that triggers on some gamepad button, e.g. Triangle on a DualSense. Add the script from step (1) to some game object within the Rebinding UI scene.

  2. Run the Rebinding UI sample and rebind a button action to the same button as bound to Test action.

Observe behavior and compare to previous behavior. It's critical to test with both polled and event based gamepads, e.g. Xbox + Dualsense/Dualshock on Windows or any of them on macOS.

The above approach can be used on both previous and current PR.

Also note that for test/verification that is not comparative, the reworked rebinding samples is recommended to test this through.

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 EventHandledPolicy_ShouldReflectUserSetting, Events_ShouldRespectHandledPolicyUponUpdate.
    • 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.

After merge:

  • Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

@ekcoh ekcoh added the work in progress Indicates that the PR is work in progress and any review efforts can be post-poned. label Apr 16, 2025
@ekcoh
Copy link
Collaborator Author

ekcoh commented Apr 28, 2025

Tried running tests with this setting on all the time yields failures on:
CoreTests.Actions_InteractiveRebinding_CanSuppressEventsWhileListening (Not part of PR but very related to fix)
CoreTests.EventHandledPolicy_ShouldReflectUserSetting (Part of PR)
CoreTests.Events_ShouldRespectHandledPolicyUponUpdate (Part of PR)

@ekcoh ekcoh removed the work in progress Indicates that the PR is work in progress and any review efforts can be post-poned. label Jun 25, 2025
@ekcoh ekcoh marked this pull request as ready for review June 25, 2025 15:25
@ekcoh ekcoh changed the title FIX: (ISXB-1524, ISX-2325) Rebinding issues related to incorrect event suppression + Reworked Rebinding Sample (WIP) FIX: (ISXB-1524, ISX-2325) Rebinding issues related to incorrect event suppression + Reworked Rebinding Sample Jun 25, 2025
@ekcoh ekcoh changed the title FIX: (ISXB-1524, ISX-2325) Rebinding issues related to incorrect event suppression + Reworked Rebinding Sample FIX: (ISXB-1524, ISX-2325, revisiting ISXB-1396) Rebinding issues related to incorrect event suppression + Reworked Rebinding Sample Jun 25, 2025
@ekcoh
Copy link
Collaborator Author

ekcoh commented Jun 25, 2025

Observations that might need to be addressed:

  • Update: A 10 seconds timeout is now part of the sample (but not visualised) rebinding UI sample would benefit from a timeout if nothing can be detected during a "wait"
  • The rebinding UI sample would benefit from control swaps when assigning a control that is already assigned. This have been asked for in discussion/forum.

@ekcoh ekcoh changed the title FIX: (ISXB-1524, ISX-2325, revisiting ISXB-1396) Rebinding issues related to incorrect event suppression + Reworked Rebinding Sample NEW: (ISXB-1524, ISX-2325, revisiting ISXB-1396) Rebinding issues related to incorrect event suppression + Reworked Rebinding Sample Jun 25, 2025
@ekcoh ekcoh changed the title NEW: (ISXB-1524, ISX-2325, revisiting ISXB-1396) Rebinding issues related to incorrect event suppression + Reworked Rebinding Sample NEW: (ISXB-1524, ISX-2325, revisiting ISXB-1396) Fix for rebinding issues related to incorrect event suppression + Changed Rebinding Sample Jun 25, 2025
@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented Jun 26, 2025

Looks great, still looking into it but just one thing I noticed immediately. It seems like other types of input, other than expected, are not blocked when rebinding. I can still navigate the UI using WASD when rebinding the Look Vector2

26.06.2025.-.Unity.205.mp4

@ekcoh
Copy link
Collaborator Author

ekcoh commented Jun 26, 2025

Looks great, still looking into it but just one thing I noticed immediately. It seems like other types of inputs, other than expected, are not blocked when rebinding. I can still navigate the UI using WASD when rebinding the Look Vector2

26.06.2025.-.Unity.205.mp4

Thanks, I believe I noticed this myself but forgot about it. I guess the change I reverted from Jak that disabled the UI map prevented that but I will see if there is a better solution that blocks all without requiring users to do anything. Basically just marking any input event handled during this phase should be enough, will revisit this aspect when back at work. Great to file any quirks you find like this and we can sort them out. I also listed a few above.

ekcoh added 2 commits June 26, 2025 12:35
…g when the rebinding screen has no UI. Added missing exception checks for attempting to reconfigure rebinding during rebinding.
…nologies/InputSystem into ekcoh/rebinding-issues-events
@ekcoh
Copy link
Collaborator Author

ekcoh commented Jun 26, 2025

Another issue/quirk that I noticed that needs fixing is that when rebinding there will be double submit required to enter rebinding mode again if the bound control is not the submit control. This is likely since we enter rebinding with submit, action state is freezes with the submit action state "high", which means that it will not go "low" just by exiting rebinding wait mode. The next press on submit control will transition low to high which means it essentially doesn't change anything. But when released we go low again and then subsequent press will work since we detect low->high. This might call for a more involved fix in InputActionState.

ekcoh added 5 commits June 26, 2025 14:13
…t might not be needed. Added proper UI disable state to RebindMenu since RebindOverlay is essentially another "window" grabbing input context. Hence the sample should behave like this since its the most correct way instead of suppressing UI input.
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.

2 participants