FIX: first player's UI controls stop working after a second player joins (ISXB-125) (#1561)

* FIX: Add broken test to show issue with missing stateMonitors when using multiple PlayerInput/UIInputModule instances

* FIX: first player's UI controls stop working after a second player joins

* FIX: alternative fix for UIInputModule sharing action references using explicit Clone

* FIX: UIInputModule missing in some builds

* REVIEW: move uiInputModule clone into InitializeActions

* REVIEW: UIInputModule instances only disables actions that the instance enabled

* REVIEW: replace uiInputModule local state tracking with mono behaviour enable state

* prevent uiInputModule decrementing refCount twice; SwapAction already enables/disables

* review changes. renaming for clarity

Author: jamesmcgill

Assets/Tests/InputSystem/Plugins/PlayerInputTests.cs

@@ -2142,6 +2142,74 @@ public void PlayerInput_WhenJoinActionIsAReference_JoiningIsStillPossibleAfterDe
         Assert.That(playerJoined, Is.True);
     }
 
+    // https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-125
+    [Test]
+    [Category("PlayerInput")]
+    public void PlayerInput_WhenSecondPlayerJoins_UIInputForFirstPlayerContinuesWorking()
+    {
+        var actions = ScriptableObject.CreateInstance<InputActionAsset>();
+        var playerMap = actions.AddActionMap("Player");
+        var uiMap = actions.AddActionMap("UI");
+
+        var joinAction = playerMap.AddAction("Join", binding: "<Gamepad>/{PrimaryAction}");
+        joinAction.AddBinding("<Keyboard>/space");
+
+        // Left Stick is bound to UIInputModule Navigate
+        var navigateAction = uiMap.AddAction("Navigate", binding: "<Gamepad>/leftStick", type: InputActionType.PassThrough);
+        navigateAction.AddCompositeBinding("2DVector")
+            .With("Up", "<Keyboard>/upArrow")
+            .With("Down", "<Keyboard>/downArrow")
+            .With("Left", "<Keyboard>/leftArrow")
+            .With("Right", "<Keyboard>/rightArrow");
+
+
+        var playerPrefab = new GameObject();
+        playerPrefab.SetActive(false);
+        var prefabUIModule = playerPrefab.AddComponent<InputSystemUIInputModule>();
+        prefabUIModule.AssignDefaultActions();
+        playerPrefab.AddComponent<PlayerInput>();
+        playerPrefab.GetComponent<PlayerInput>().actions = actions;
+        playerPrefab.GetComponent<PlayerInput>().uiInputModule = prefabUIModule;
+
+        var manager = new GameObject();
+        manager.SetActive(false);
+        var playerInputManager = manager.AddComponent<PlayerInputManager>();
+        playerInputManager.notificationBehavior = PlayerNotifications.InvokeCSharpEvents;
+        playerInputManager.joinAction = new InputActionProperty(InputActionReference.Create(joinAction));
+        playerInputManager.joinBehavior = PlayerJoinBehavior.JoinPlayersWhenJoinActionIsTriggered;
+        playerInputManager.playerPrefab = playerPrefab;
+        manager.SetActive(true);
+
+        var gamepad = InputSystem.AddDevice<Gamepad>();
+        var keyboard = InputSystem.AddDevice<Keyboard>();
+
+        List<PlayerInput> joinedPlayers = new List<PlayerInput>();
+        playerInputManager.onPlayerJoined += input => joinedPlayers.Add(input);
+
+        // UIInputModule instance for player 1 will be bound to Gamepad
+        PressAndRelease(gamepad.buttonSouth);
+        Assert.That(joinedPlayers.Count, Is.EqualTo(1));
+
+        // Player 1's controls are functional
+        bool player1Moved = false;
+        joinedPlayers[0].uiInputModule.move.action.performed += cxt => player1Moved = true;
+        Set(gamepad.leftStick, new Vector2(0.2f, 0.0f));
+        Assert.That(player1Moved, Is.True);
+
+        Set(gamepad.leftStick, new Vector2(0.0f, 0.0f));
+        player1Moved = false;
+
+        // UIInputModule instance for player 2 will be bound to Keyboard
+        // And this should not affect player 1's controls
+        PressAndRelease(keyboard.spaceKey);
+        Assert.That(joinedPlayers.Count, Is.EqualTo(2));
+        Assert.That(player1Moved, Is.False);
+
+        // Player 1's controls still work after player 2 joined
+        Set(gamepad.leftStick, new Vector2(0.2f, 0.0f));
+        Assert.That(player1Moved, Is.True);
+    }
+
     [Test] // Mimics what is reported in https://issuetracker.unity3d.com/product/unity/issues/guid/1347320
     [Category("PlayerInput")]
     public void PlayerInput_WhenOverridingDeviceLayout_LostDeviceShouldBeResolvedAndRepaired()

Assets/Tests/InputSystem/Plugins/UITests.cs

@@ -2837,6 +2837,45 @@ public IEnumerator UI_WhenMultipleInputModulesExist_ActionsAreNotDisabledUntilTh
         Assert.That(pointAction.enabled, Is.False);
     }
 
+    // https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-125
+    [Test]
+    [Category("UI")]
+    [TestCase(true)]
+    [TestCase(false)]
+    public void UI_WhenMultipleInputModulesExist_AssigningActionAssetDoesNotDisableOtherInstancesActions(bool player2StartsDisabled = false)
+    {
+        var player1_EventSystemGO = new GameObject();
+        player1_EventSystemGO.AddComponent<EventSystem>();
+        var player1_InputModule = player1_EventSystemGO.AddComponent<InputSystemUIInputModule>();
+        if (player2StartsDisabled)
+            player1_EventSystemGO.SetActive(false); // Enable player1 later
+
+        var player1_Asset = ScriptableObject.CreateInstance<InputActionAsset>();
+        var player1_Map = player1_Asset.AddActionMap("map");
+        var pointAction = player1_Map.AddAction("point", type: InputActionType.PassThrough, binding: "<Mouse>/position");
+        player1_InputModule.point = InputActionReference.Create(pointAction);
+
+        var player2_EventSystemGO = GameObject.Instantiate(player1_EventSystemGO);
+
+
+        if (player2StartsDisabled)
+        {
+            Assert.That(pointAction.enabled, Is.False);
+            player1_EventSystemGO.SetActive(true);
+        }
+        Assert.That(pointAction.enabled, Is.True);
+
+        var player2_Asset = ScriptableObject.CreateInstance<InputActionAsset>();
+        var player2_Map = player2_Asset.AddActionMap("map");
+        var player2_pointAction = player2_Map.AddAction("point", type: InputActionType.PassThrough, binding: "<Mouse>/position");
+        player2_EventSystemGO.GetComponent<InputSystemUIInputModule>().actionsAsset = player2_Asset;
+
+        if (player2StartsDisabled)
+            Assert.That(player2_pointAction.enabled, Is.False);
+
+        Assert.That(pointAction.enabled, Is.True);
+    }
+
     [Test]
     [Category("UI")]
     public void UI_WhenDisablingInputModule_ActionsAreNotDisabledIfTheyWereNotEnabledByTheInputModule()

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -20,6 +20,8 @@ however, it has to be formatted properly to pass verification tests.
 - Fixed an issue where the Input Action asset icon would not be visible during asset creation ([case ISXB-6](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-6)).
 - Fixed DualSense low frequency motor speed being always set to min value.
 - Fixed an issue where `ReadUnprocessedValueFromState` in PoseControl always returning default values.
+- Fix Player 1's UI controls stop working after second player joins ([case ISXB-125](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-125)))
+
 
 ## [1.4.1] - 2022-05-30
 

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

@@ -1196,7 +1196,6 @@ private void InitializeActions()
             if (m_Actions == null)
                 return;
 
-            ////REVIEW: should we *always* Instantiate()?
             // Check if we need to duplicate our actions by looking at all other players. If any
             // has the same actions, duplicate.
             for (var i = 0; i < s_AllActivePlayersCount; ++i)

Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs

@@ -857,7 +857,7 @@ private void SwapAction(ref InputActionReference property, InputActionReference
             var oldActionNull = property?.action == null;
             var oldActionEnabled = property?.action != null && property.action.enabled;
 
-            DisableInputAction(property);
+            TryDisableInputAction(property);
             property = newValue;
 
             #if DEBUG
@@ -1508,16 +1508,16 @@ private void EnableAllActions()
 
         private void DisableAllActions()
         {
-            DisableInputAction(m_PointAction);
-            DisableInputAction(m_LeftClickAction);
-            DisableInputAction(m_RightClickAction);
-            DisableInputAction(m_MiddleClickAction);
-            DisableInputAction(m_MoveAction);
-            DisableInputAction(m_SubmitAction);
-            DisableInputAction(m_CancelAction);
-            DisableInputAction(m_ScrollWheelAction);
-            DisableInputAction(m_TrackedDeviceOrientationAction);
-            DisableInputAction(m_TrackedDevicePositionAction);
+            TryDisableInputAction(m_PointAction, true);
+            TryDisableInputAction(m_LeftClickAction, true);
+            TryDisableInputAction(m_RightClickAction, true);
+            TryDisableInputAction(m_MiddleClickAction, true);
+            TryDisableInputAction(m_MoveAction, true);
+            TryDisableInputAction(m_SubmitAction, true);
+            TryDisableInputAction(m_CancelAction, true);
+            TryDisableInputAction(m_ScrollWheelAction, true);
+            TryDisableInputAction(m_TrackedDeviceOrientationAction, true);
+            TryDisableInputAction(m_TrackedDevicePositionAction, true);
         }
 
         private void EnableInputAction(InputActionReference inputActionReference)
@@ -1542,14 +1542,20 @@ private void EnableInputAction(InputActionReference inputActionReference)
             action.Enable();
         }
 
-        private static void DisableInputAction(InputActionReference inputActionReference)
+        private void TryDisableInputAction(InputActionReference inputActionReference, bool isComponentDisabling = false)
         {
             var action = inputActionReference?.action;
             if (action == null)
                 return;
 
-            if (!s_InputActionReferenceCounts.TryGetValue(action,
-                out var referenceState))
+            // Don't decrement refCount when we were not responsible for incrementing it.
+            // I.e. when we were not enabled yet. When OnDisabled is called, isActiveAndEnabled will
+            // already have been set to false. In that case we pass isComponentDisabling to check if we
+            // came from OnDisabled and therefore need to allow disabling.
+            if (!isActiveAndEnabled && !isComponentDisabling)
+                return;
+
+            if (!s_InputActionReferenceCounts.TryGetValue(action, out var referenceState))
                 return;
 
             if (referenceState.refCount - 1 == 0 && referenceState.enabledByInputModule)
@@ -2212,10 +2218,6 @@ private InputActionReference UpdateReferenceForNewAsset(InputActionReference act
             if (oldAction == null)
                 return null;
 
-            var oldActionEnabled = oldAction.enabled;
-            if (oldActionEnabled)
-                DisableInputAction(actionReference);
-
             var oldActionMap = oldAction.actionMap;
             Debug.Assert(oldActionMap != null, "Not expected to end up with a singleton action here");
 
@@ -2227,11 +2229,7 @@ private InputActionReference UpdateReferenceForNewAsset(InputActionReference act
             if (newAction == null)
                 return null;
 
-            var reference = InputActionReference.Create(newAction);
-            if (oldActionEnabled)
-                EnableInputAction(reference);
-
-            return reference;
+            return InputActionReference.Create(newAction);
         }
 
         public InputActionAsset actionsAsset