Minor tweaks to sample, expanded test coverage, temporary avoid deserialization in Input Manager

Author: ekcoh

Assets/Samples/RebindingUI/GameManager.cs

@@ -52,6 +52,8 @@ private void OnMenu(InputAction.CallbackContext obj)
 
     private void OnExitMenu(InputAction.CallbackContext obj)
     {
+        // TODO We cannot do this without first cancelling rebinding
+
         if (!menu.activeInHierarchy)
             return;
 

Assets/Samples/RebindingUI/RebindingMaterial.mat

@@ -79,7 +79,7 @@ Material:
     - _UVSec: 0
     - _ZWrite: 1
     m_Colors:
-    - _Color: {r: 1, g: 0, b: 1.1e-44, a: 1}
+    - _Color: {r: 0.9995672, g: 0.9212032, b: 0.016187144, a: 1}
     - _EmissionColor: {r: 0.27450982, g: 0.078431375, b: 0.19607843, a: 1}
   m_BuildTextureStacks: []
   m_AllowLocking: 1

Assets/Samples/RebindingUI/RebindingUISampleScene.unity

@@ -892,7 +892,7 @@ MonoBehaviour:
     m_HorizontalOverflow: 0
     m_VerticalOverflow: 0
     m_LineSpacing: 1
-  m_Text: Move
+  m_Text: Navigate
 --- !u!222 &323438533
 CanvasRenderer:
   m_ObjectHideFlags: 0
@@ -2255,7 +2255,7 @@ MonoBehaviour:
   m_Script: {fileID: 11500000, guid: b5c8f13dfafeb5445b872565802d1e44, type: 3}
   m_Name: 
   m_EditorClassIdentifier: 
-  action: {fileID: -4334443723673744087, guid: 100b460020a15704692cc0ec34331d8a, type: 3}
+  action: {fileID: -64187129634329061, guid: 100b460020a15704692cc0ec34331d8a, type: 3}
   activeColor: {r: 0, g: 1, b: 0, a: 1}
   inactiveColor: {r: 0, g: 0, b: 0, a: 1}
   duration: 1

Assets/Tests/InputSystem/CoreTests_Events.cs

@@ -1216,14 +1216,6 @@ public void Events_CanPreventEventsFromBeingProcessed()
         Assert.That(device.rightTrigger.ReadValue(), Is.EqualTo(0.0).Within(0.00001));
     }
 
-    class SuppressedActionEventData
-    {
-        public bool markNextEventHandled;
-        public int startedCount;
-        public int performedCount;
-        public int canceledCount;
-    }
-
     [Test]
     [Category("Events")]
     public void EventHandledPolicy_ShouldReflectUserSetting()
@@ -1245,14 +1237,40 @@ public void EventHandledPolicy_ShouldReflectUserSetting()
         Assert.That(InputSystem.s_Manager.inputEventHandledPolicy, Is.EqualTo(InputEventHandledPolicy.SuppressStateUpdates));
     }
 
+    class SuppressedActionEventData
+    {
+        public bool MarkNextEventHandled;
+        public int StartedCount;
+        public int PerformedCount;
+        public int CanceledCount;
+    }
+
+    // Note that each element in the expected value arrays correspond to accumulated count per test step.
+    [TestCase(InputEventHandledPolicy.SuppressStateUpdates, // policy
+        null, // interactions
+        new int[] { 0, 0, 1, 1, 2}, // started
+        new int[] { 0, 0, 1, 1, 2}, // performed
+        new int[] {0, 0, 0, 1, 1})] // cancelled
+    [TestCase(InputEventHandledPolicy.SuppressActionUpdates,
+        null,
+        new int[] { 0, 0, 0, 0, 1},
+        new int[] { 0, 0, 0, 0, 1},
+        new int[] {0, 0, 0, 0, 0})]
     [TestCase(InputEventHandledPolicy.SuppressStateUpdates,
-        new int[] { 0, 0, 1, 1}, new int[] {0, 0, 0, 1})]
+        "press",
+        new int[] { 0, 0, 1, 1, 2},
+        new int[] { 0, 0, 1, 1, 2},
+        new int[] {0, 0, 0, 1, 1})]
     [TestCase(InputEventHandledPolicy.SuppressActionUpdates,
-        new int[] { 0, 0, 0, 0}, new int[] {0, 0, 0, 0})]
+        "press",
+        new int[] { 0, 0, 0, 0, 1},
+        new int[] { 0, 0, 0, 0, 1},
+        new int[] {0, 0, 0, 0, 0})]
     [Category("Events")]
-    [Description("ISXB-1524 Events suppressed has side-effects on actions when based on polling")]
-    public void Events_ShouldRespectHandledPolicyUponUpdate(InputEventHandledPolicy policy,
-        int[] expectedProcessed, int[] expectedCancelled) // EDIT
+    [Description("ISXB-1524, ISXB-1396 Events suppressed has side-effects on actions")]
+    public void Events_ShouldRespectHandledPolicyUponUpdateAndSuppressedPressTransition(
+        InputEventHandledPolicy policy, string interactions,
+        int[] expectedStarted, int[] expectedPerformed, int[] expectedCancelled)
     {
         // Update setting to match desired scenario
         InputSystem.s_Manager.inputEventHandledPolicy = policy;
@@ -1264,42 +1282,91 @@ public void Events_ShouldRespectHandledPolicyUponUpdate(InputEventHandledPolicy
             (inputEvent, _) =>
         {
             // If we mark the event handled, the system should skip it and not
-            // let it go to the device.
-            inputEvent.handled = data.markNextEventHandled;
+            // let it go to the device (SuppressStateUpdates) or let it propagate
+            // but not fire actions (SuppressActionUpdates).
+            inputEvent.handled = data.MarkNextEventHandled;
         };
 
         var device = InputSystem.AddDevice<Gamepad>();
-        var action = new InputAction(type: InputActionType.Button, binding: "<Gamepad>/buttonNorth");
+        var action = new InputAction(type: InputActionType.Button,
+            binding: "<Gamepad>/buttonNorth",
+            interactions: interactions);
         action.Enable();
-        action.started += _ => ++ data.startedCount;
-        action.performed += _ => ++ data.performedCount;
-        action.canceled += _ => ++ data.canceledCount;
+        action.started += _ => ++ data.StartedCount;
+        action.performed += _ => ++ data.PerformedCount;
+        action.canceled += _ => ++ data.CanceledCount;
 
         // Ensure state is updated/initialized
         InputSystem.QueueStateEvent(device, new GamepadState() { leftStick = new Vector2(0.01f, 0.0f) });
         InputSystem.Update();
-        Assert.That(data.performedCount, Is.EqualTo(expectedProcessed[0]));
-        Assert.That(data.canceledCount, Is.EqualTo(expectedCancelled[0]));
-
-        // Press button north with event suppression active
-        data.markNextEventHandled = true;
-        InputSystem.QueueStateEvent(device, new GamepadState() { leftStick = new Vector2(0.00f, 0.01f) }.WithButton(GamepadButton.North));
+        Assert.That(data.StartedCount, Is.EqualTo(expectedStarted[0]));
+        Assert.That(data.PerformedCount, Is.EqualTo(expectedPerformed[0]));
+        Assert.That(data.CanceledCount, Is.EqualTo(expectedCancelled[0]));
+        var performedThisFrame = expectedPerformed[0] != 0;
+        Assert.That(action.WasPerformedThisFrame, Is.EqualTo(performedThisFrame));
+        Assert.That(action.WasPressedThisFrame, Is.EqualTo(performedThisFrame));
+        var releasedThisFrame = expectedCancelled[0] != 0;
+        Assert.That(action.WasReleasedThisFrame, Is.EqualTo(releasedThisFrame));
+        // TODO Assert.That(Gamepad.current.buttonNorth.wasPressedThisFrame, Is.False); <-- TODO Needs separate handling, just suppress?
+
+        // Press button north and left stick with event suppression active
+        data.MarkNextEventHandled = true;
+        InputSystem.QueueStateEvent(device, new GamepadState() { leftStick = new Vector2(1.00f, 0.01f) }
+            .WithButton(GamepadButton.North));
         InputSystem.Update();
-        Assert.That(data.performedCount, Is.EqualTo(expectedProcessed[1]));
-        Assert.That(data.canceledCount, Is.EqualTo(expectedCancelled[1]));
-
-        // Simulate a periodic reading, this will trigger performed count
-        data.markNextEventHandled = false;
-        InputSystem.QueueStateEvent(device, new GamepadState() { leftStick = new Vector2(0.01f, 0.00f) }.WithButton(GamepadButton.North));
+        Assert.That(data.StartedCount, Is.EqualTo(expectedStarted[1]));
+        Assert.That(data.PerformedCount, Is.EqualTo(expectedPerformed[1]));
+        Assert.That(data.CanceledCount, Is.EqualTo(expectedCancelled[1]));
+        performedThisFrame = expectedPerformed[1] - expectedPerformed[0] > 0;
+        Assert.That(action.WasPerformedThisFrame, Is.EqualTo(performedThisFrame));
+        Assert.That(action.WasPressedThisFrame, Is.EqualTo(performedThisFrame));
+        releasedThisFrame = expectedCancelled[1] - expectedCancelled[0] > 0;
+        Assert.That(action.WasReleasedThisFrame, Is.EqualTo(releasedThisFrame));
+        // TODO Assert.That(Gamepad.current.buttonNorth.wasPressedThisFrame, Is.EqualTo(expectedPerformed[1] - expectedPerformed[0] > 0));
+
+        // Simulate a periodic reading (e.g. driven by noise or irrelevant control), this will trigger performed count.
+        // Note that for SuppressStateUpdates (default), this would trigger a state change since North button
+        // transitions from 0 to 1 which is considered a press.
+        data.MarkNextEventHandled = false;
+        InputSystem.QueueStateEvent(device, new GamepadState() { leftStick = new Vector2(0.99f, 0.00f) }
+            .WithButton(GamepadButton.North));
         InputSystem.Update();
-        Assert.That(data.performedCount, Is.EqualTo(expectedProcessed[2])); // Firing without actual change
-        Assert.That(data.canceledCount, Is.EqualTo(expectedCancelled[2]));
-
-        // Release button north
+        Assert.That(data.StartedCount, Is.EqualTo(expectedStarted[2]));
+        Assert.That(data.PerformedCount, Is.EqualTo(expectedPerformed[2])); // Firing without actual change
+        Assert.That(data.CanceledCount, Is.EqualTo(expectedCancelled[2]));
+        performedThisFrame = expectedPerformed[2] - expectedPerformed[1] > 0;
+        Assert.That(action.WasPerformedThisFrame, Is.EqualTo(performedThisFrame));
+        Assert.That(action.WasPressedThisFrame, Is.EqualTo(performedThisFrame));
+        releasedThisFrame = expectedCancelled[2] - expectedCancelled[1] > 0;
+        Assert.That(action.WasReleasedThisFrame, Is.EqualTo(releasedThisFrame));
+        // TODO Assert.That(Gamepad.current.buttonNorth.wasPressedThisFrame, Is.EqualTo(expectedPerformed[2] - expectedPerformed[1] > 0));
+
+        // Release button north and stick while no longer being suppressed. This may result in a release if
+        // previous event was completely ignored without updating interaction state.
         InputSystem.QueueStateEvent(device, new GamepadState() { leftStick = new Vector2(0.00f, 0.01f) });
         InputSystem.Update();
-        Assert.That(data.performedCount, Is.EqualTo(expectedProcessed[3]));
-        Assert.That(data.canceledCount, Is.EqualTo(expectedCancelled[3]));
+        Assert.That(data.StartedCount, Is.EqualTo(expectedStarted[3]));
+        Assert.That(data.PerformedCount, Is.EqualTo(expectedPerformed[3]));
+        Assert.That(data.CanceledCount, Is.EqualTo(expectedCancelled[3]));
+        performedThisFrame = expectedPerformed[3] - expectedPerformed[2] > 0;
+        Assert.That(action.WasPerformedThisFrame, Is.EqualTo(performedThisFrame));
+        Assert.That(action.WasPressedThisFrame, Is.EqualTo(performedThisFrame));
+        releasedThisFrame = expectedCancelled[3] - expectedCancelled[2] > 0;
+        Assert.That(action.WasReleasedThisFrame, Is.EqualTo(releasedThisFrame));
+        // TODO Assert.That(Gamepad.current.buttonNorth.wasPressedThisFrame, Is.EqualTo(expectedPerformed[3] - expectedPerformed[2] > 0));
+
+        // Press button north and stick again while not being suppressed.
+        InputSystem.QueueStateEvent(device, new GamepadState() { leftStick = new Vector2(0.99f, 0.00f) }
+            .WithButton(GamepadButton.North));
+        InputSystem.Update();
+        Assert.That(data.StartedCount, Is.EqualTo(expectedStarted[4]));
+        Assert.That(data.PerformedCount, Is.EqualTo(expectedPerformed[4]));
+        Assert.That(data.CanceledCount, Is.EqualTo(expectedCancelled[4]));
+        performedThisFrame = expectedPerformed[4] - expectedPerformed[3] > 0;
+        Assert.That(action.WasPerformedThisFrame, Is.EqualTo(performedThisFrame));
+        Assert.That(action.WasPressedThisFrame, Is.EqualTo(performedThisFrame));
+        releasedThisFrame = expectedCancelled[4] - expectedCancelled[3] > 0;
+        Assert.That(action.WasReleasedThisFrame, Is.EqualTo(releasedThisFrame));
     }
 
     [StructLayout(LayoutKind.Explicit, Size = 2)]

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

@@ -1520,7 +1520,7 @@ private void ProcessControlStateChange(int mapIndex, int controlIndex, int bindi
                     }
 
                     // Check if we should suppress interaction processing
-                    var suppressInteractionProcessing = (eventPtr != null) && eventPtr.handled &&
+                    var suppressActionProcessing = (eventPtr != null) && eventPtr.handled &&
                         InputSystem.s_Manager.inputEventHandledPolicy == InputEventHandledPolicy.SuppressActionUpdates;
 
                     // Check if we have multiple concurrent actuations on the same action. This may lead us
@@ -1531,21 +1531,24 @@ private void ProcessControlStateChange(int mapIndex, int controlIndex, int bindi
                     var isConflictingInput = IsConflictingInput(ref trigger, actionIndex);
                     bindingStatePtr = &bindingStates[trigger.bindingIndex]; // IsConflictingInput may switch us to a different binding.
 
+                    // TODO Potentially we need to move suppression down to indicators/callbacks, this may be too blunt
+
                     // Process button presses/releases.
-                    if (!isConflictingInput)
+                    if (!isConflictingInput && !suppressActionProcessing)
                         ProcessButtonState(ref trigger, actionIndex, bindingStatePtr);
 
                     // If we have interactions, let them do all the processing. The presence of an interaction
                     // essentially bypasses the default phase progression logic of an action.
                     var interactionCount = bindingStatePtr->interactionCount;
                     if (interactionCount > 0 && !bindingStatePtr->isPartOfComposite)
                     {
-                        ProcessInteractions(ref trigger, bindingStatePtr->interactionStartIndex, interactionCount);
+                        if (!suppressActionProcessing)
+                            ProcessInteractions(ref trigger, bindingStatePtr->interactionStartIndex, interactionCount);
                     }
                     else if (!haveInteractionsOnComposite && !isConflictingInput)
                     {
-                        //if (!suppressInteractionProcessing)
-                        ProcessDefaultInteraction(ref trigger, actionIndex);
+                        if (!suppressActionProcessing) // <-- This solves it for default interaction
+                            ProcessDefaultInteraction(ref trigger, actionIndex);
                     }
                 }
                 finally
@@ -1573,6 +1576,7 @@ private void ProcessButtonState(ref TriggerState trigger, int actionIndex, Bindi
             if (controlActuation <= pressPoint * ButtonControl.s_GlobalDefaultButtonReleaseThreshold)
                 bindingStatePtr->pressTime = 0d;
 
+            // TODO Point of interest (polled events)
             var actuation = trigger.magnitude;
             var actionState = &actionStates[actionIndex];
             if (!actionState->isPressed && actuation >= pressPoint)
@@ -1947,7 +1951,7 @@ private void ProcessDefaultInteraction(ref TriggerState trigger, int actionIndex
                         var threshold = controls[trigger.controlIndex] is ButtonControl button ? button.pressPointOrDefault : ButtonControl.s_GlobalDefaultButtonPressPoint;
                         if (actuation >= threshold)
                         {
-                            // CALLBACK HERE
+                            // CALLBACK HERE!
                             ChangePhaseOfAction(InputActionPhase.Performed, ref trigger,
                                 phaseAfterPerformedOrCanceled: InputActionPhase.Performed);
                         }
@@ -2365,8 +2369,8 @@ private bool ChangePhaseOfAction(InputActionPhase newPhase, ref TriggerState tri
 
             // Ignore if action is disabled.
             var actionState = &actionStates[actionIndex];
-            if (actionState->isDisabled)
-                return true;
+            if (actionState->isDisabled /*|| InputSystem.s_Manager.inputEventHandledPolicy == InputEventHandledPolicy.SuppressActionUpdates*/)
+                return true; // <--- Could be relevant
 
             // We mark the action as in-processing while we execute its phase transitions and perform
             // callbacks. The callbacks may alter system state such that the action may get disabled
@@ -2406,7 +2410,7 @@ private bool ChangePhaseOfAction(InputActionPhase newPhase, ref TriggerState tri
                 }
                 else if (actionState->phase != newPhase || newPhase == InputActionPhase.Performed) // We allow Performed to trigger repeatedly.
                 {
-                    // CALLBACK HERE
+                    // CALLBACK HERE!
                     ChangePhaseOfActionInternal(actionIndex, actionState, newPhase, ref trigger,
                         isDisablingAction: newPhase == InputActionPhase.Canceled && phaseAfterPerformedOrCanceled == InputActionPhase.Disabled);
                     if (!actionState->inProcessing)

Packages/com.unity.inputsystem/InputSystem/InputManager.cs

@@ -4161,7 +4161,7 @@ internal void RestoreStateWithoutDevices(SerializedState state)
             scrollDeltaBehavior = state.scrollDeltaBehavior;
             m_Metrics = state.metrics;
             m_PollingFrequency = state.pollingFrequency;
-            m_InputEventHandledPolicy = state.inputEventHandledPolicy;
+            m_InputEventHandledPolicy = InputEventHandledPolicy.SuppressStateUpdates; // TODO Make sure we always restore: state.inputEventHandledPolicy;
 
             if (m_Settings != null)
                 Object.DestroyImmediate(m_Settings);

Packages/com.unity.inputsystem/InputSystem/InputManagerStateMonitors.cs

@@ -426,7 +426,7 @@ internal unsafe void FireStateChangeNotifications(int deviceIndex, double intern
                 // Need to reset it back to false as we may have more signalled state monitors that
                 // aren't in the same group (i.e. have independent inputs).
                 if (eventPtr->handled)
-                    eventPtr->handled = false;
+                    eventPtr->handled = previouslyHandled;
 
                 signals.ClearBit(i);
             }