FIX: Enabling single actions leading to errors when enabling their map (#1061).

Author: Rene Damm

Assets/Tests/InputSystem/CoreTests_Actions.cs

@@ -1161,53 +1161,33 @@ public void Actions_CanByPassControlActuationChecks_UsingPasshtroughAction()
         var action = new InputAction(type: InputActionType.PassThrough, binding: "<Gamepad>/*stick");
         action.Enable();
 
-        using (var trace = new InputActionTrace())
+        using (var trace = new InputActionTrace(action))
         {
-            trace.SubscribeTo(action);
-
             Set(gamepad.leftStick, new Vector2(0.123f, 0.234f));
 
-            var actions = trace.ToArray();
-            Assert.That(actions, Has.Length.EqualTo(1));
-            Assert.That(actions[0].phase, Is.EqualTo(InputActionPhase.Performed));
-            Assert.That(actions[0].control, Is.SameAs(gamepad.leftStick));
-            Assert.That(actions[0].ReadValue<Vector2>(),
-                Is.EqualTo(new StickDeadzoneProcessor().Process(new Vector2(0.123f, 0.234f)))
-                    .Using(Vector2EqualityComparer.Instance));
+            Assert.That(trace,
+                Performed(action, gamepad.leftStick, new StickDeadzoneProcessor().Process(new Vector2(0.123f, 0.234f))));
 
             trace.Clear();
 
             Set(gamepad.leftStick, new Vector2(0.234f, 0.345f));
 
-            actions = trace.ToArray();
-            Assert.That(actions, Has.Length.EqualTo(1));
-            Assert.That(actions[0].phase, Is.EqualTo(InputActionPhase.Performed));
-            Assert.That(actions[0].control, Is.SameAs(gamepad.leftStick));
-            Assert.That(actions[0].ReadValue<Vector2>(),
-                Is.EqualTo(new StickDeadzoneProcessor().Process(new Vector2(0.234f, 0.345f)))
-                    .Using(Vector2EqualityComparer.Instance));
+            Assert.That(trace,
+                Performed(action, gamepad.leftStick, new StickDeadzoneProcessor().Process(new Vector2(0.234f, 0.345f))));
 
             trace.Clear();
 
             Set(gamepad.rightStick, new Vector2(0.123f, 0.234f));
 
-            actions = trace.ToArray();
-            Assert.That(actions, Has.Length.EqualTo(1));
-            Assert.That(actions[0].phase, Is.EqualTo(InputActionPhase.Performed));
-            Assert.That(actions[0].control, Is.SameAs(gamepad.rightStick));
-            Assert.That(actions[0].ReadValue<Vector2>(),
-                Is.EqualTo(new StickDeadzoneProcessor().Process(new Vector2(0.123f, 0.234f)))
-                    .Using(Vector2EqualityComparer.Instance));
+            Assert.That(trace,
+                Performed(action, gamepad.rightStick, new StickDeadzoneProcessor().Process(new Vector2(0.123f, 0.234f))));
 
             trace.Clear();
 
             Set(gamepad.rightStick, Vector2.zero);
 
-            actions = trace.ToArray();
-            Assert.That(actions, Has.Length.EqualTo(1));
-            Assert.That(actions[0].phase, Is.EqualTo(InputActionPhase.Performed));
-            Assert.That(actions[0].control, Is.SameAs(gamepad.rightStick));
-            Assert.That(actions[0].ReadValue<Vector2>(), Is.EqualTo(Vector2.zero).Using(Vector2EqualityComparer.Instance));
+            Assert.That(trace,
+                Performed(action, gamepad.rightStick, Vector2.zero));
         }
     }
 
@@ -3588,6 +3568,69 @@ public void Actions_CanEnableActionThatHasNoControls()
         Assert.That(action2.enabled, Is.True);
     }
 
+    [Test]
+    [Category("Actions")]
+    public void Actions_CanMixEnablingSingleActionsAndEntireActionMaps()
+    {
+        var gamepad = InputSystem.AddDevice<Gamepad>();
+
+        var asset = ScriptableObject.CreateInstance<InputActionAsset>();
+        var map1 = new InputActionMap("map1");
+        var map2 = new InputActionMap("map2");
+        var action1 = map1.AddAction("action1", binding: "<Gamepad>/buttonSouth");
+        var action2 = map1.AddAction("action2", binding: "<Gamepad>/buttonNorth");
+        var action3 = map2.AddAction("action3", binding: "<Gamepad>/buttonSouth");
+        var action4 = map2.AddAction("action4", binding: "<Gamepad>/buttonNorth");
+        asset.AddActionMap(map1);
+        asset.AddActionMap(map2);
+
+        action3.Enable();
+        map1.Enable();
+
+        using (var trace1 = new InputActionTrace(action1))
+        using (var trace2 = new InputActionTrace(action2))
+        using (var trace3 = new InputActionTrace(action3))
+        using (var trace4 = new InputActionTrace(action4))
+        {
+            PressAndRelease(gamepad.buttonSouth);
+
+            Assert.That(trace1, Started(action1).AndThen(Performed(action1)).AndThen(Canceled(action1)));
+            Assert.That(trace2, Is.Empty);
+            Assert.That(trace3, Started(action3).AndThen(Performed(action3)).AndThen(Canceled(action3)));
+            Assert.That(trace4, Is.Empty);
+
+            trace1.Clear();
+            trace2.Clear();
+            trace3.Clear();
+            trace4.Clear();
+
+            map1.Disable();
+            map2.Enable();
+
+            PressAndRelease(gamepad.buttonSouth);
+
+            Assert.That(trace1, Is.Empty);
+            Assert.That(trace2, Is.Empty);
+            Assert.That(trace3, Started(action3).AndThen(Performed(action3)).AndThen(Canceled(action3)));
+            Assert.That(trace4, Is.Empty);
+
+            trace1.Clear();
+            trace2.Clear();
+            trace3.Clear();
+            trace4.Clear();
+
+            map1.Enable();
+            map2.Disable();
+
+            PressAndRelease(gamepad.buttonSouth);
+
+            Assert.That(trace1, Started(action1).AndThen(Performed(action1)).AndThen(Canceled(action1)));
+            Assert.That(trace2, Is.Empty);
+            Assert.That(trace3, Is.Empty);
+            Assert.That(trace4, Is.Empty);
+        }
+    }
+
     [Test]
     [Category("Actions")]
     public void Actions_CanTargetSingleDeviceWithMultipleActions()
@@ -5724,7 +5767,6 @@ public void Actions_CompositesInDifferentMapsTiedToSameControlsWork()
         asset.AddActionMap(map2);
         asset.Enable();
 
-
         InputControl performedControl1 = null;
         InputControl performedControl2 = null;
         action1.performed += ctx => performedControl1 = ctx.control;

Assets/Tests/InputSystem/CoreTests_Actions_Interactions.cs

@@ -12,7 +12,10 @@ internal partial class CoreTests
     [Preserve]
     class InteractionThatOnlyPerforms : IInputInteraction<float>
     {
+        // Get rid of unused field warning.
+        #pragma warning disable CS0649
         public bool stayPerformed;
+        #pragma warning restore CS0649
 
         public void Process(ref InputInteractionContext context)
         {

Assets/Tests/InputSystem/CoreTests_Editor.cs

@@ -2210,6 +2210,31 @@ public void Editor_InputEventsOccurringWhileGoingIntoPlayMode_AreDiscarded()
         Assert.That(runtime.m_EventCount, Is.EqualTo(0));
     }
 
+    [Test]
+    [Category("Editor")]
+    public void Editor_LeavingPlayMode_DestroysAllActionStates()
+    {
+        InputSystem.AddDevice<Gamepad>();
+
+        // Enter play mode.
+        InputSystem.OnPlayModeChange(PlayModeStateChange.ExitingEditMode);
+        InputSystem.OnPlayModeChange(PlayModeStateChange.EnteredPlayMode);
+
+        var action = new InputAction(binding: "<Gamepad>/buttonSouth");
+        action.Enable();
+
+        Assert.That(InputActionState.s_GlobalList.length, Is.EqualTo(1));
+        Assert.That(InputSystem.s_Manager.m_StateChangeMonitors.Length, Is.GreaterThan(0));
+        Assert.That(InputSystem.s_Manager.m_StateChangeMonitors[0].count, Is.EqualTo(1));
+
+        // Exit play mode.
+        InputSystem.OnPlayModeChange(PlayModeStateChange.ExitingPlayMode);
+        InputSystem.OnPlayModeChange(PlayModeStateChange.EnteredEditMode);
+
+        Assert.That(InputActionState.s_GlobalList.length, Is.Zero);
+        Assert.That(InputSystem.s_Manager.m_StateChangeMonitors[0].listeners[0].control, Is.Null); // Won't get removed, just cleared.
+    }
+
     ////TODO: tests for InputAssetImporter; for this we need C# mocks to be able to cut us off from the actual asset DB
 }
 #endif // UNITY_EDITOR

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 Due to package verification, the latest version below is the unpublished version and the date is meaningless.
 however, it has to be formatted properly to pass verification tests.
 
+## [Unreleased]
+
+### Fixed
+
+#### Actions
+
+- Mixing the enabling&disabling of single actions (as, for example, performed by `InputSystemUIInputModule`) with enabling&disabling of entire action maps (as, for example, performed by `PlayerInput`) no longer leaves to unresponsive input and `"should not reach here"` assertions ([forum thread](https://forum.unity.com/threads/error-while-switching-between-action-maps.825204/)).
+- Leaving play mode no longer leaves state change monitors lingering around from enabled actions.
+
 ## [1.0.0-preview.5] - 2020-02-14
 
 ### Changed

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

@@ -110,6 +110,7 @@ internal unsafe class InputActionState : IInputStateChangeMonitor, ICloneable, I
         public BindingState* bindingStates => memory.bindingStates;
         public InteractionState* interactionStates => memory.interactionStates;
         public int* controlIndexToBindingIndex => memory.controlIndexToBindingIndex;
+        public int* enabledControls => memory.enabledControls;
 
         private bool m_OnBeforeUpdateHooked;
         private bool m_OnAfterUpdateHooked;
@@ -158,6 +159,10 @@ private void Destroy(bool isFinalizing = false)
                 {
                     var map = maps[i];
 
+                    // Remove state change monitors.
+                    if (map.enabled)
+                        DisableControls(i, mapIndices[i].controlStartIndex, mapIndices[i].controlCount);
+
                     if (map.m_Asset != null)
                         map.m_Asset.m_SharedStateForAllMaps = null;
 
@@ -467,17 +472,19 @@ public void EnableAllActions(InputActionMap map)
             Debug.Assert(map.m_Actions != null, "Map must have actions");
             Debug.Assert(maps.Contains(map), "Map must be contained in state");
 
+            // Enable all controls in map that aren't already enabled.
             EnableControls(map);
 
-            // Put all actions into waiting state.
+            // Put all actions that aren't already enbaled into waiting state.
             var mapIndex = map.m_MapIndexInState;
             Debug.Assert(mapIndex >= 0 && mapIndex < totalMapCount, "Map index on InputActionMap is out of range");
             var actionCount = mapIndices[mapIndex].actionCount;
             var actionStartIndex = mapIndices[mapIndex].actionStartIndex;
             for (var i = 0; i < actionCount; ++i)
             {
                 var actionIndex = actionStartIndex + i;
-                actionStates[actionIndex].phase = InputActionPhase.Waiting;
+                if (actionStates[actionIndex].isDisabled)
+                    actionStates[actionIndex].phase = InputActionPhase.Waiting;
             }
             map.m_EnabledActionsCount = actionCount;
 
@@ -662,6 +669,7 @@ private void DisableControls(InputAction action)
 
         ////REVIEW: can we have a method on InputManager doing this in bulk?
 
+        ////NOTE: This must not enable only a partial set of controls on a binding (currently we have no setup that would lead to that)
         private void EnableControls(int mapIndex, int controlStartIndex, int numControls)
         {
             Debug.Assert(controls != null, "State must have controls");
@@ -673,12 +681,20 @@ private void EnableControls(int mapIndex, int controlStartIndex, int numControls
             for (var i = 0; i < numControls; ++i)
             {
                 var controlIndex = controlStartIndex + i;
+
+                // We don't want to add multiple state monitors for the same control. This can happen if enabling
+                // single actions is mixed with enabling actions maps containing them.
+                if (IsControlEnabled(controlIndex))
+                    continue;
+
                 var bindingIndex = controlIndexToBindingIndex[controlIndex];
                 var mapControlAndBindingIndex = ToCombinedMapAndControlAndBindingIndex(mapIndex, controlIndex, bindingIndex);
-
-                if (bindingStates[bindingIndex].wantsInitialStateCheck)
-                    bindingStates[bindingIndex].initialStateCheckPending = true;
+                var bindingStatePtr = &bindingStates[bindingIndex];
+                if (bindingStatePtr->wantsInitialStateCheck)
+                    bindingStatePtr->initialStateCheckPending = true;
                 manager.AddStateChangeMonitor(controls[controlIndex], this, mapControlAndBindingIndex);
+
+                SetControlEnabled(controlIndex, true);
             }
         }
 
@@ -693,15 +709,38 @@ private void DisableControls(int mapIndex, int controlStartIndex, int numControl
             for (var i = 0; i < numControls; ++i)
             {
                 var controlIndex = controlStartIndex + i;
+                if (!IsControlEnabled(controlIndex))
+                    continue;
+
                 var bindingIndex = controlIndexToBindingIndex[controlIndex];
                 var mapControlAndBindingIndex = ToCombinedMapAndControlAndBindingIndex(mapIndex, controlIndex, bindingIndex);
-
-                if (bindingStates[bindingIndex].wantsInitialStateCheck)
-                    bindingStates[bindingIndex].initialStateCheckPending = false;
+                var bindingStatePtr = &bindingStates[bindingIndex];
+                if (bindingStatePtr->wantsInitialStateCheck)
+                    bindingStatePtr->initialStateCheckPending = false;
                 manager.RemoveStateChangeMonitor(controls[controlIndex], this, mapControlAndBindingIndex);
+
+                SetControlEnabled(controlIndex, false);
             }
         }
 
+        private bool IsControlEnabled(int controlIndex)
+        {
+            var intIndex = controlIndex / 32;
+            var intMask = 1 << (controlIndex % 32);
+            return (enabledControls[intIndex] & intMask) != 0;
+        }
+
+        private void SetControlEnabled(int controlIndex, bool state)
+        {
+            var intIndex = controlIndex / 32;
+            var intMask = 1 << (controlIndex % 32);
+
+            if (state)
+                enabledControls[intIndex] |= intMask;
+            else
+                enabledControls[intIndex] &= ~intMask;
+        }
+
         private void HookOnBeforeUpdate()
         {
             if (m_OnBeforeUpdateHooked)
@@ -2860,7 +2899,8 @@ public struct UnmanagedMemory : IDisposable
                 compositeCount * sizeof(float) + // compositeMagnitudes
                 controlCount * sizeof(int) + // controlIndexToBindingIndex
                 actionCount * sizeof(ushort) * 2 + // actionBindingIndicesAndCounts
-                bindingCount * sizeof(ushort); // actionBindingIndices
+                bindingCount * sizeof(ushort) + // actionBindingIndices
+                (controlCount + 31) / 32 * sizeof(int); // enabledControlsArray
 
             /// <summary>
             /// Trigger state of all actions added to the state.
@@ -2903,6 +2943,8 @@ public struct UnmanagedMemory : IDisposable
 
             public float* compositeMagnitudes;
 
+            public int* enabledControls;
+
             /// <summary>
             /// Array of pair of ints, one pair for each action (same index as <see cref="actionStates"/>). First int
             /// is count of bindings on action, second int is index into <see cref="actionBindingIndices"/> where
@@ -2953,6 +2995,7 @@ public void Allocate(int mapCount, int actionCount, int bindingCount, int contro
                 controlIndexToBindingIndex = (int*)ptr; ptr += controlCount * sizeof(int);
                 actionBindingIndicesAndCounts = (ushort*)ptr; ptr += actionCount * sizeof(ushort) * 2;
                 actionBindingIndices = (ushort*)ptr; ptr += bindingCount * sizeof(ushort);
+                enabledControls = (int*)ptr; ptr += (controlCount + 31) / 32 * sizeof(int);
             }
 
             public void Dispose()
@@ -2997,6 +3040,7 @@ public void CopyDataFrom(UnmanagedMemory memory)
                 UnsafeUtility.MemCpy(controlIndexToBindingIndex, memory.controlIndexToBindingIndex, memory.controlCount * sizeof(int));
                 UnsafeUtility.MemCpy(actionBindingIndicesAndCounts, memory.actionBindingIndicesAndCounts, memory.actionCount * sizeof(ushort) * 2);
                 UnsafeUtility.MemCpy(actionBindingIndices, memory.actionBindingIndices, memory.bindingCount * sizeof(ushort));
+                UnsafeUtility.MemCpy(enabledControls, memory.enabledControls, (memory.controlCount + 31) / 32 * sizeof(int));
             }
 
             public UnmanagedMemory Clone()
@@ -3030,7 +3074,7 @@ public UnmanagedMemory Clone()
         ///
         /// Both of these needs are served by this global list.
         /// </remarks>
-        private static InlinedArray<GCHandle> s_GlobalList;
+        internal static InlinedArray<GCHandle> s_GlobalList;
         internal static InlinedArray<Action<object, InputActionChange>> s_OnActionChange;
 
         private void AddToGlobaList()

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

@@ -1130,6 +1130,7 @@ public void AddDevice(InputDevice device)
         }
 
         ////TODO: this path should really put the device on the list of available devices
+        ////TODO: this path should discover disconnected devices
         public InputDevice AddDevice(InputDeviceDescription description)
         {
             ////REVIEW: is throwing here really such a useful thing?

Packages/com.unity.inputsystem/InputSystem/Plugins/PlayerInput/PlayerInput.cs

@@ -407,6 +407,7 @@ public bool neverAutoSwitchControlSchemes
             }
         }
 
+        ////REVIEW: this is inconsistent; currentControlScheme is a string, this is an InputActionMap
         /// <summary>
         /// The currently enabled action map.
         /// </summary>