FIX: UI controls in MultiplayerEventSystems should not change colour unexpectedly (#1547)

Author: andrew-oc

Assets/Tests/InputSystem/Plugins/UITests.cs

@@ -2526,9 +2526,7 @@ public IEnumerator UI_CanOperateMultiplayerUILocallyUsingGamepads()
         Assert.That(players[1].eventSystem.currentSelectedGameObject, Is.SameAs(players[1].leftGameObject));
 
         Assert.That(players[0].leftChildReceiver.events, Is.Empty);
-        Assert.That(players[0].rightChildReceiver.events,
-            EventSequence(
-                OneEvent("type", EventType.Move))); // OnMove will still get called to *attempt* a move.
+        Assert.That(players[0].rightChildReceiver.events, Is.Empty);
 
         players[0].leftChildReceiver.events.Clear();
         players[0].rightChildReceiver.events.Clear();

Packages/com.unity.inputsystem/CHANGELOG.md

@@ -12,6 +12,7 @@ however, it has to be formatted properly to pass verification tests.
 
 ### Fixed
 - Fix UI sometimes ignoring the first mouse click event after losing and regaining focus ([case ISXB-127](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-127).
+- Fixed issue when using MultiplayerEventSystems where the visual state of UI controls would change due to constant toggling of CanvasGroup.interactable on and off ([case ISXB-112](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-112)).
 
 
 ## [1.4.1] - 2022-05-30

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

@@ -6,6 +6,7 @@
 using UnityEngine.InputSystem.LowLevel;
 using UnityEngine.InputSystem.Utilities;
 using UnityEngine.Serialization;
+using UnityEngine.UI;
 #if UNITY_EDITOR
 using UnityEditor;
 #endif
@@ -100,6 +101,21 @@ public CursorLockBehavior cursorLockBehavior
             set => m_CursorLockBehavior = value;
         }
 
+        /// <summary>
+        /// A root game object to support correct navigation in local multi-player UIs.
+        /// <remarks>
+        /// In local multi-player games where each player has their own UI, players should not be able to navigate into
+        /// another player's UI. Each player should have their own instance of an InputSystemUIInputModule, and this property
+        /// should be set to the root game object containing all UI objects for that player. If set, navigation using the
+        /// <see cref="InputSystemUIInputModule.move"/> action will be constrained to UI objects under that root.
+        /// </remarks>
+        /// </summary>
+        public GameObject localMultiPlayerRoot
+        {
+            get => m_LocalMultiPlayerRoot;
+            set => m_LocalMultiPlayerRoot = value;
+        }
+
         /// <summary>
         /// Called by <c>EventSystem</c> when the input module is made current.
         /// </summary>
@@ -650,12 +666,15 @@ internal void ProcessNavigation(ref NavigationModel navigationState)
                         eventData.moveVector = moveVector;
                         eventData.moveDir = moveDirection;
 
-                        ExecuteEvents.Execute(eventSystem.currentSelectedGameObject, eventData, ExecuteEvents.moveHandler);
-                        usedSelectionChange = eventData.used;
+                        if (IsMoveAllowed(eventData))
+                        {
+                            ExecuteEvents.Execute(eventSystem.currentSelectedGameObject, eventData, ExecuteEvents.moveHandler);
+                            usedSelectionChange = eventData.used;
 
-                        m_NavigationState.consecutiveMoveCount = m_NavigationState.consecutiveMoveCount + 1;
-                        m_NavigationState.lastMoveTime = time;
-                        m_NavigationState.lastMoveDirection = moveDirection;
+                            m_NavigationState.consecutiveMoveCount = m_NavigationState.consecutiveMoveCount + 1;
+                            m_NavigationState.lastMoveTime = time;
+                            m_NavigationState.lastMoveDirection = moveDirection;
+                        }
                     }
                 }
                 else
@@ -686,6 +705,45 @@ internal void ProcessNavigation(ref NavigationModel navigationState)
             }
         }
 
+        private bool IsMoveAllowed(AxisEventData eventData)
+        {
+            if (m_LocalMultiPlayerRoot == null)
+                return true;
+
+            if (eventSystem.currentSelectedGameObject == null)
+                return true;
+
+            var selectable = eventSystem.currentSelectedGameObject.GetComponent<Selectable>();
+
+            if (selectable == null)
+                return true;
+
+            Selectable navigationTarget = null;
+            switch (eventData.moveDir)
+            {
+                case MoveDirection.Right:
+                    navigationTarget = selectable.FindSelectableOnRight();
+                    break;
+
+                case MoveDirection.Up:
+                    navigationTarget = selectable.FindSelectableOnUp();
+                    break;
+
+                case MoveDirection.Left:
+                    navigationTarget = selectable.FindSelectableOnLeft();
+                    break;
+
+                case MoveDirection.Down:
+                    navigationTarget = selectable.FindSelectableOnDown();
+                    break;
+            }
+
+            if (navigationTarget == null)
+                return true;
+
+            return navigationTarget.transform.IsChildOf(m_LocalMultiPlayerRoot.transform);
+        }
+
         [FormerlySerializedAs("m_RepeatDelay")]
         [Tooltip("The Initial delay (in seconds) between an initial move action and a repeated move action.")]
         [SerializeField]
@@ -2251,6 +2309,8 @@ private struct InputActionReferenceState
         // Navigation-type input.
         private NavigationModel m_NavigationState;
 
+        [NonSerialized] private GameObject m_LocalMultiPlayerRoot;
+
         /// <summary>
         /// Controls the origin point of raycasts when the cursor is locked.
         /// </summary>

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

@@ -1,6 +1,5 @@
 #if PACKAGE_DOCS_GENERATION || UNITY_INPUT_SYSTEM_ENABLE_UI
 using UnityEngine.EventSystems;
-using UnityEngine.InputSystem.Utilities;
 
 namespace UnityEngine.InputSystem.UI
 {
@@ -10,13 +9,13 @@ namespace UnityEngine.InputSystem.UI
     /// </summary>
     /// <remarks>
     /// You can use the <see cref="playerRoot"/> property to specify a part of the hierarchy belonging to the current player.
-    /// Mouse selection will ignore any game objects not within this hierarchy. For gamepad/keyboard selection, you need to make sure that
-    /// the navigation links stay within the player's hierarchy.
+    /// Mouse selection will ignore any game objects not within this hierarchy, and all other navigation, using keyboard or
+    /// gamepad for example, will be constrained to game objects under that hierarchy.
     /// </remarks>
     [HelpURL(InputSystem.kDocUrl + "/manual/UISupport.html#multiplayer-uis")]
     public class MultiplayerEventSystem : EventSystem
     {
-        [Tooltip("If set, only process mouse events for any game objects which are children of this game object.")]
+        [Tooltip("If set, only process mouse and navigation events for any game objects which are children of this game object.")]
         [SerializeField] private GameObject m_PlayerRoot;
 
         /// <summary>
@@ -25,86 +24,35 @@ public class MultiplayerEventSystem : EventSystem
         /// <remarks>
         /// This can either be an entire <c>Canvas</c> or just part of the hierarchy of
         /// a specific <c>Canvas</c>.
-        ///
-        /// Note that if the given <c>GameObject</c> has a <c>CanvasGroup</c> component on it, its
-        /// <c>interactable</c> property will be toggled back and forth by <see cref="MultiplayerEventSystem"/>.
-        /// If no such component exists on the <c>GameObject</c>, one will be added automatically.
-        ///
-        /// Only the <c>CanvasGroup</c> corresponding to the <see cref="MultiplayerEventSystem"/> that is currently
-        /// executing its <see cref="Update"/> method (or did so last) will have <c>interactable</c> set to true.
-        /// In other words, only the UI hierarchy corresponding to the player that is currently running a UI
-        /// update (or that did so last) can be interacted with.
         /// </remarks>
         public GameObject playerRoot
         {
             get => m_PlayerRoot;
             set
             {
                 m_PlayerRoot = value;
-                InitializeCanvasGroup();
+                InitializePlayerRoot();
             }
         }
 
-        private CanvasGroup m_CanvasGroup;
-        private bool m_CanvasGroupWasAddedByUs;
-
-        private static int s_MultiplayerEventSystemCount;
-        private static MultiplayerEventSystem[] s_MultiplayerEventSystems;
-
         protected override void OnEnable()
         {
             base.OnEnable();
 
-            ArrayHelpers.AppendWithCapacity(ref s_MultiplayerEventSystems, ref s_MultiplayerEventSystemCount, this);
-
-            InitializeCanvasGroup();
-        }
-
-        private void InitializeCanvasGroup()
-        {
-            if (m_PlayerRoot != null)
-            {
-                m_CanvasGroup = m_PlayerRoot.GetComponent<CanvasGroup>();
-                if (m_CanvasGroup == null)
-                {
-                    m_CanvasGroup = m_PlayerRoot.AddComponent<CanvasGroup>();
-                    m_CanvasGroupWasAddedByUs = true;
-                }
-                else
-                    m_CanvasGroupWasAddedByUs = false;
-            }
-            else
-            {
-                m_CanvasGroup = null;
-            }
+            InitializePlayerRoot();
         }
 
-        protected override void OnDisable()
+        private void InitializePlayerRoot()
         {
-            var index = s_MultiplayerEventSystems.IndexOfReference(this);
-            if (index != -1)
-                s_MultiplayerEventSystems.EraseAtWithCapacity(ref s_MultiplayerEventSystemCount, index);
-
-            if (m_CanvasGroupWasAddedByUs)
-                Destroy(m_CanvasGroup);
-
-            m_CanvasGroup = default;
-            m_CanvasGroupWasAddedByUs = default;
+            if (m_PlayerRoot == null) return;
 
-            base.OnDisable();
+            var inputModule = GetComponent<InputSystemUIInputModule>();
+            if (inputModule != null)
+                inputModule.localMultiPlayerRoot = m_PlayerRoot;
         }
 
         protected override void Update()
         {
-            for (var i = 0; i < s_MultiplayerEventSystemCount; ++i)
-            {
-                var system = s_MultiplayerEventSystems[i];
-                if (system.m_PlayerRoot == null)
-                    continue;
-
-                system.m_CanvasGroup.interactable = system == this;
-            }
-
             var originalCurrent = current;
             current = this; // in order to avoid reimplementing half of the EventSystem class, just temporarily assign this EventSystem to be the globally current one
             try