(Cleanup) (feedback needed) Remove redundant list of ServerCharacters (#271)

* Remove redundant list of ServerCharacters

* added missing Using

* Revert and cleanup comments

* Removing need for ServerCharacter list by iterating on player connections

* remove unneeded usings

* Refactoring to use a static list of player ServerCharacters

* simplifying diff

* Update Assets/BossRoom/Scripts/Server/Game/Character/PlayerServerCharacter.cs

* moving to serialized field rather than GetComponent, and acting properly inert on client.

Co-authored-by: Fernando Cortez <[email protected]>
Co-authored-by: David Woodruff <[email protected]>

Author: 3 people

Assets/BossRoom/Prefabs/Character/Player.prefab

@@ -13,6 +13,19 @@ MonoBehaviour:
   m_Name: 
   m_EditorClassIdentifier: 
   m_FeedbackPrefab: {fileID: 9137928905311479176, guid: 5d22c1d86e0e5604cbe14004bf924827, type: 3}
+--- !u!114 &-4877982356580531930
+MonoBehaviour:
+  m_ObjectHideFlags: 0
+  m_CorrespondingSourceObject: {fileID: 0}
+  m_PrefabInstance: {fileID: 0}
+  m_PrefabAsset: {fileID: 0}
+  m_GameObject: {fileID: 6009713983291384756}
+  m_Enabled: 1
+  m_EditorHideFlags: 0
+  m_Script: {fileID: 11500000, guid: a46d628dbb19f12449867d14fa5268b0, type: 3}
+  m_Name: 
+  m_EditorClassIdentifier: 
+  m_CachedServerCharacter: {fileID: 6012286037226018870}
 --- !u!114 &2576537793715222015
 MonoBehaviour:
   m_ObjectHideFlags: 0
@@ -210,6 +223,17 @@ Transform:
   m_CorrespondingSourceObject: {fileID: 4600110157238723791, guid: 0d2d836e2e83b754fa1a1c4022d6d65d, type: 3}
   m_PrefabInstance: {fileID: 7831782662127126385}
   m_PrefabAsset: {fileID: 0}
+--- !u!114 &6012286037226018870 stripped
+MonoBehaviour:
+  m_CorrespondingSourceObject: {fileID: 4602672899881656135, guid: 0d2d836e2e83b754fa1a1c4022d6d65d, type: 3}
+  m_PrefabInstance: {fileID: 7831782662127126385}
+  m_PrefabAsset: {fileID: 0}
+  m_GameObject: {fileID: 6009713983291384756}
+  m_Enabled: 1
+  m_EditorHideFlags: 0
+  m_Script: {fileID: 11500000, guid: 920a440eb254ba348915767fd046027a, type: 3}
+  m_Name: 
+  m_EditorClassIdentifier: 
 --- !u!114 &7751377510591478590 stripped
 MonoBehaviour:
   m_CorrespondingSourceObject: {fileID: 514105321093282895, guid: 0d2d836e2e83b754fa1a1c4022d6d65d, type: 3}

Assets/BossRoom/Scripts/Server/Game/AIState/IdleAIState.cs

@@ -1,3 +1,4 @@
+using MLAPI;
 using UnityEngine;
 
 namespace BossRoom.Server
@@ -33,7 +34,8 @@ protected void DetectFoes()
             float detectionRangeSqr = detectionRange * detectionRange;
             Vector3 position = m_Brain.GetMyServerCharacter().transform.position;
 
-            foreach (ServerCharacter character in ServerCharacter.GetAllActiveServerCharacters())
+            // in this game, NPCs only attack players (and never other NPCs), so we can just iterate over the players to see if any are nearby
+            foreach (var character in PlayerServerCharacter.GetPlayerServerCharacters())
             {
                 if (m_Brain.IsAppropriateFoe(character) && (character.transform.position - position).sqrMagnitude <= detectionRangeSqr)
                 {

Assets/BossRoom/Scripts/Server/Game/Character/PlayerServerCharacter.cs

@@ -0,0 +1,54 @@
+using System.Collections;
+using System.Collections.Generic;
+using MLAPI;
+using UnityEngine;
+
+namespace BossRoom.Server
+{
+    /// <summary>
+    /// Attached to the player-characters' prefab, this maintains a list of active ServerCharacter objects for players.
+    /// </summary>
+    /// <remarks>
+    /// This is an optimization. In server code you can already get a list of players' ServerCharacters by
+    /// iterating over the active connections and calling GetComponent() on their PlayerObject. But we need
+    /// to iterate over all players quite often -- the monsters' IdleAIState does so in every Update() --
+    /// and all those GetComponent() calls add up! So this optimization lets us iterate without calling
+    /// GetComponent(). This will be refactored with a ScriptableObject-based approach on player collection.
+    /// </remarks>
+    [RequireComponent(typeof(ServerCharacter))]
+    public class PlayerServerCharacter : NetworkBehaviour
+    {
+        static List<ServerCharacter> s_ActivePlayers = new List<ServerCharacter>();
+
+        [SerializeField]
+        ServerCharacter m_CachedServerCharacter;
+
+        public override void NetworkStart()
+        {
+            if( !IsServer )
+            {
+                enabled = false;
+            }
+        }
+
+        void OnEnable()
+        {
+            s_ActivePlayers.Add(m_CachedServerCharacter);
+        }
+
+        void OnDisable()
+        {
+            s_ActivePlayers.Remove(m_CachedServerCharacter);
+        }
+
+        /// <summary>
+        /// Returns a list of all active players' ServerCharacters. Treat the list as read-only!
+        /// The list will be empty on the client.
+        /// </summary>
+        public static List<ServerCharacter> GetPlayerServerCharacters()
+        {
+
+            return s_ActivePlayers;
+        }
+    }
+}

Assets/BossRoom/Scripts/Server/Game/Character/PlayerServerCharacter.cs.meta

@@ -42,12 +42,6 @@ public bool IsNpc
         // Cached component reference
         private ServerCharacterMovement m_Movement;
 
-        /// <summary>
-        /// Temp place to store all the active characters (to avoid having to
-        /// perform insanely-expensive GameObject.Find operations during Update)
-        /// </summary>
-        private static List<ServerCharacter> s_ActiveServerCharacters = new List<ServerCharacter>();
-
         private void Awake()
         {
             m_Movement = GetComponent<ServerCharacterMovement>();
@@ -60,21 +54,6 @@ private void Awake()
             }
         }
 
-        private void OnEnable()
-        {
-            s_ActiveServerCharacters.Add(this);
-        }
-
-        private void OnDisable()
-        {
-            s_ActiveServerCharacters.Remove(this);
-        }
-
-        public static List<ServerCharacter> GetAllActiveServerCharacters()
-        {
-            return s_ActiveServerCharacters;
-        }
-
         public override void NetworkStart()
         {
             if (!IsServer) { enabled = false; }

Assets/BossRoom/Scripts/Server/Game/Character/ServerCharacter.cs

@@ -248,15 +248,9 @@ bool IsAnyPlayerNearbyAndVisible()
 
             // iterate through clients and only return true if a player is in range
             // and is not occluded by a blocking collider.
-            foreach (KeyValuePair<ulong, NetworkClient> idToClient in NetworkManager.Singleton.ConnectedClients)
+            foreach (var serverCharacter in PlayerServerCharacter.GetPlayerServerCharacters())
             {
-                if (idToClient.Value.PlayerObject == null)
-                {
-                    // skip over any connection that doesn't have a PlayerObject yet
-                    continue;
-                }
-
-                var playerPosition = idToClient.Value.PlayerObject.transform.position;
+                var playerPosition = serverCharacter.transform.position;
                 var direction = playerPosition - spawnerPosition;
 
                 if (direction.sqrMagnitude > squaredProximityDistance)

Assets/BossRoom/Scripts/Server/Game/Entity/ServerWaveSpawner.cs

@@ -174,12 +174,10 @@ private void OnHeroLifeStateChanged(LifeState prevLifeState, LifeState lifeState
             if (lifeState == LifeState.Fainted)
             {
                 // Check the life state of all players in the scene
-                foreach (var p in NetworkManager.Singleton.ConnectedClientsList )
+                foreach (var serverCharacter in PlayerServerCharacter.GetPlayerServerCharacters())
                 {
-                    if (p.PlayerObject == null) { continue; }
-                    // if any player is alive just retrun
-                    var netState = p.PlayerObject.GetComponent<NetworkCharacterState>();
-                    if ( netState.NetworkLifeState.Value == LifeState.Alive ) { return; }
+                    // if any player is alive just retun
+                    if (serverCharacter.NetState && serverCharacter.NetState.NetworkLifeState.Value == LifeState.Alive) { return; }
                 }
 
                 // If we made it this far, all players are down! switch to post game