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

[ekcoh]

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.

Additionally also provides fixes to the following bugs found during development:

• ISXB-1588 RebindActionUI does not show binding if no event is triggered or after a scene reload.
• ISXB-1593 GamepadIconsExample does not show DualSense trigger button icons after being rebound to the same control.

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 play a simple mini-game: move, look, fire, change mode, access menu. Also added new (InputActionLabel) controls showing what keys accesses the in-game menu dynamically. Also note that UI colours and navigation links were fixed to allow running the sample without a mouse or keyboard present. Added light-bar (light/color output) and haptic (rumble) feedback to the mini-game to show-case these features and how to manage them in a game-like setting which found issues not visible in the trivial existing sample. Added "Look" rebinding even though there may be no suitable control to change to for consistency. The sample now also adds "multi-panel" challenges by being more similar to a real example which typically might have multiple UI views.

Rebinding sample game mode:

Rebinding sample rebinding menu mode:

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.

Added Samples_RebindingUI_InvokeUnityEventForwardsEventto verify functionality of new example component used to dismiss the rebinding menu via action without code.

Found these issues while implementing this that are not addressed by this PR but have workaround:

• ISXB-1586 - A fixed rate-adoption workaround has been implemented in the updated rebinding sample FeedbackController.
• ISXB-1587 - A fixed rate-adoption workaround has been implemented in the updated rebinding sample FeedbackController.

Note that the above feedback controller affects the sample "mini-game" with light and haptic rumble effects when connected devices support it, e.g. DualSense.

Observations that might be additional bugs (not filed yet) that likely should be filed.

• [macOS] The updated sample uses Cursor.lockState = CursorLockMode.Locked; while not being paused and hiding the cursor. At least when running in the editor on macOS this seem to generate an unexpected "jump" in reported mouse delta on first delta received resulting in an unexpected change of direction. From then on, everything is fine.
• [macOS/WebGL] When built for WebGL (macOS Safari) it seems that viewport scaling is incorrect.
• [macOS/WebGL] When built for WebGL (macOS safari) there seem to be inconsistent movement to mouse delta making the sample barely playable.

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)");
    }
}

2. 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.

3. 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]

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)

[Pauliusd01]

Two minor things so far:

• There's a typo in the Canvas - RebindMenu - Keyboard - Help object, "somthing" -> "something"
• Shouldn't UI Navigate be disabled during gameplay? It is currently active with both menu on and off

[ekcoh]

Two minor things so far:

• There's a typo in the Canvas - RebindMenu - Keyboard - Help object, "somthing" -> "something"

Typo fixed in https://github.com/Unity-Technologies/InputSystem/pull/2168/commits/4T3829dad4624d3fc51d13d4303467959de153916

• Shouldn't UI Navigate be disabled during gameplay?

It is currently active with both menu on and off
It doesn't serve any purpose at the movement, but my intent was to keep it on to fish for issues. Also if we add in-game UI as part of extending this to on-screen touch controls later we want it on. However if you @Pauliusd01 prefer it's disabled now I can make that fix quickly?

Update: I made the change, they are now disabled. Also made it a setting on the RebindUIGameManager so it can easily be changed later. See 5d40eb4

[Pauliusd01]

Two minor things so far:

• There's a typo in the Canvas - RebindMenu - Keyboard - Help object, "somthing" -> "something"

Typo fixed in https://github.com/Unity-Technologies/InputSystem/pull/2168/commits/4T3829dad4624d3fc51d13d4303467959de153916

• Shouldn't UI Navigate be disabled during gameplay?

It is currently active with both menu on and off It doesn't serve any purpose at the movement, but my intent was to keep it on to fish for issues. Also if we add in-game UI as part of extending this to on-screen touch controls later we want it on. However if you @Pauliusd01 prefer it's disabled now I can make that fix quickly?

Update: I made the change, they are now disabled. Also made it a setting on the RebindUIGameManager so it can easily be changed later. See 5d40eb4

I'm fine with your judgment just figured it was worth mentioning if It's supposed to represent what people want/should do

[ekcoh]

I agree, now its off so it makes sense given the current feature set.

[ritamerkl]

First I looked at the sample, and it's excellent.
I think it is great to see when which action is fired in the top panel.
I just have three minor remarks:

• I do like the UI action panel on the top right, which shows which UI action was triggered, but could lead to confusion on how that relates to rebinding (at least for the user) - also during rebinding the UI actions are still fired- maybe it's worth to add a bit of clarity here (maybe by only showing the UI Actions during a rebinding process / active menu?)
• the descriptive texts on the left and right are strictly seen not necessary IMO, since the example is pretty self-explaining
• the look with Mouse works slow for me, maybe great to speed it up to make it a bit easier to handle

[Copilot]

Pull Request Overview

This PR adds a new input-event suppression policy to allow state updates while still suppressing action callbacks during rebinding, and it overhauls the rebinding sample to better mimic real-game UI with game/menu modes and new helper components.

• Introduces InputEventHandledPolicy in InputManager and propagates it through the event loop and action state.
• Adds the fluent API WithActionEventNotificationsBeingSuppressed() and updates the sample to opt into it.
• Reworks the Rebinding UI sample: version bump, game/menu modes, dynamic labels, cancel buttons, timeout info, and new helper scripts.

Comments suppressed due to low confidence (4)

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs:374

• Public C# properties should use PascalCase. Rename isSuppressed to IsSuppressed for consistency with .NET naming conventions.

        public bool isSuppressed => m_Suppressed;

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionRebindingExtensions.cs:1560

• [nitpick] The sentence is awkward and redundant. Consider rephrasing to "Note that not all input should necessarily be suppressed."

            /// Note that all input shouldn't necessarily should be suppressed. For example, it can be desirable to have UI that

Assets/Samples/RebindingUI/RebindActionUI.cs:199

• The variable bindingId is undefined in this scope (renamed to id). Update the log to reference id instead or reintroduce bindingId.

                Debug.LogError($"Cannot find binding with ID '{bindingId}' on '{action}'", this);

Assets/Samples/RebindingUI/RebindSaveLoad.cs:1

• Removed the using UnityEngine; and using UnityEngine.InputSystem; directives, causing MonoBehaviour and PlayerPrefs to be unresolved. Reintroduce the necessary using statements.

namespace UnityEngine.InputSystem.Samples.RebindUI

[ritamerkl]

I think copilot found some good points

[ritamerkl]

I think you found a good and clear solution to the rebinding mess.
I only found two smaller nitpicks.

[ritamerkl]

What's the reason to outsource m_Counter from the Test? I see this could be problematic as soon as it's used by any other test. Or set it to 0 on starting the test?

[ritamerkl]

I am missing a more descriptive exception.