fix: Improved error checking for ClientScene.RegisterPrefab (#1823)

* Tests for ClientScene.RegisterPrefab * Improved error messages for ClientScene.RegisterPrefab
MirageNet · May 2, 2020 · a0aa4f9 · a0aa4f9
1 parent 5fddcc8
commit a0aa4f9
Show file tree

Hide file tree

Showing 4 changed files with 308 additions and 20 deletions.
diff --git a/Assets/Mirror/Runtime/ClientScene.cs b/Assets/Mirror/Runtime/ClientScene.cs
@@ -240,18 +240,29 @@ public static bool GetPrefab(Guid assetId, out GameObject prefab)
         /// <param name="newAssetId">An assetId to be assigned to this prefab. This allows a dynamically created game object to be registered for an already known asset Id.</param>
         public static void RegisterPrefab(GameObject prefab, Guid newAssetId)
         {
-            NetworkIdentity identity = prefab.GetComponent<NetworkIdentity>();
-            if (identity)
+            if (newAssetId == Guid.Empty)
             {
-                identity.assetId = newAssetId;
+                logger.LogError($"Could not register '{prefab.name}' with new assetId because the new assetId was empty");
+                return;
+            }
 
-                if (logger.LogEnabled()) logger.Log("Registering prefab '" + prefab.name + "' as asset:" + identity.assetId);
-                prefabs[identity.assetId] = prefab;
+            if (prefab == null)
+            {
+                logger.LogError("Could not register prefab because it was null");
+                return;
             }
-            else
+
+            NetworkIdentity identity = prefab.GetComponent<NetworkIdentity>();
+            if (identity == null)
             {
-                logger.LogError("Could not register '" + prefab.name + "' since it contains no NetworkIdentity component");
+                logger.LogError($"Could not register '{prefab.name}' since it contains no NetworkIdentity component");
+                return;
             }
+
+
+            identity.assetId = newAssetId;
+
+            RegisterPrefabIdentity(identity);
         }
 
         /// <summary>
@@ -263,23 +274,45 @@ public static void RegisterPrefab(GameObject prefab, Guid newAssetId)
         /// <param name="prefab">A Prefab that will be spawned.</param>
         public static void RegisterPrefab(GameObject prefab)
         {
+            if (prefab == null)
+            {
+                logger.LogError("Could not register prefab because it was null");
+                return;
+            }
+
             NetworkIdentity identity = prefab.GetComponent<NetworkIdentity>();
-            if (identity)
+            if (identity == null)
             {
-                if (logger.LogEnabled()) logger.Log("Registering prefab '" + prefab.name + "' as asset:" + identity.assetId);
-                prefabs[identity.assetId] = prefab;
+                logger.LogError($"Could not register '{prefab.name}' since it contains no NetworkIdentity component");
+                return;
+            }
 
-                NetworkIdentity[] identities = prefab.GetComponentsInChildren<NetworkIdentity>();
-                if (identities.Length > 1)
-                {
-                    logger.LogWarning("The prefab '" + prefab.name +
-                                     "' has multiple NetworkIdentity components. There can only be one NetworkIdentity on a prefab, and it must be on the root object.");
-                }
+            RegisterPrefabIdentity(identity);
+        }
+
+        static void RegisterPrefabIdentity(NetworkIdentity prefab)
+        {
+            if (prefab.assetId == Guid.Empty)
+            {
+                logger.LogError($"Can not Register '{prefab.name}' because it had empty assetid. If this is a scene Object use RegisterSpawnHandler instead");
+                return;
             }
-            else
+
+            if (prefab.sceneId != 0)
             {
-                logger.LogError("Could not register '" + prefab.name + "' since it contains no NetworkIdentity component");
+                logger.LogError($"Can not Register '{prefab.name}' because it has a sceneId, make sure you are passing in the original prefab and not an instance in the scene.");
+                return;
             }
+
+            NetworkIdentity[] identities = prefab.GetComponentsInChildren<NetworkIdentity>();
+            if (identities.Length > 1)
+            {
+                logger.LogWarning($"Prefab '{prefab.name}' has multiple NetworkIdentity components. There should only be one NetworkIdentity on a prefab, and it must be on the root object.");
+            }
+
+            if (logger.LogEnabled()) logger.Log($"Registering prefab '{prefab.name}' as asset:{prefab.assetId}");
+
+            prefabs[prefab.assetId] = prefab.gameObject;
         }
 
         /// <summary>

diff --git a/Assets/Mirror/Tests/Editor/ClientSceneTests.cs b/Assets/Mirror/Tests/Editor/ClientSceneTests.cs
@@ -12,12 +12,16 @@ public class ClientSceneTests
     {
         // use guid to find asset so that the path does not matter
         const string ValidPrefabAssetGuid = "33169286da0313d45ab5bfccc6cf3775";
+        const string PrefabWithChildrenAssetGuid = "a78e009e3f2dee44e8859516974ede43";
         const string InvalidPrefabAssetGuid = "78f0a3f755d35324e959f3ecdd993fb0";
+        // random guid, not used anywhere
+        const string AnotherGuidString = "5794128cdfda04542985151f82990d05";
 
         GameObject validPrefab;
+        NetworkIdentity validPrefabNetId;
+        GameObject prefabWithChildren;
         GameObject invalidPrefab;
         Guid validPrefabGuid;
-        Guid invalidPrefabGuid;
 
         Dictionary<Guid, GameObject> prefabs => ClientScene.prefabs;
         Dictionary<Guid, SpawnHandlerDelegate> spawnHandlers => ClientScene.spawnHandlers;
@@ -32,21 +36,25 @@ static GameObject LoadPrefab(string guid)
         public void OneTimeSetUp()
         {
             validPrefab = LoadPrefab(ValidPrefabAssetGuid);
+            validPrefabNetId = validPrefab.GetComponent<NetworkIdentity>();
+            prefabWithChildren = LoadPrefab(PrefabWithChildrenAssetGuid);
             invalidPrefab = LoadPrefab(InvalidPrefabAssetGuid);
             validPrefabGuid = new Guid(ValidPrefabAssetGuid);
-            invalidPrefabGuid = new Guid(ValidPrefabAssetGuid);
         }
 
         [TearDown]
         public void TearDown()
         {
             ClientScene.Shutdown();
+            // reset asset id incase they are changed by tests
+            validPrefabNetId.assetId = validPrefabGuid;
         }
 
         [OneTimeTearDown]
         public void OneTimeTearDown()
         {
             validPrefab = null;
+            prefabWithChildren = null;
             invalidPrefab = null;
         }
 
@@ -105,6 +113,149 @@ public void GetPrefab_HasOutPrefabWithCorrectGuid()
         }
 
 
+        static void CallRegisterPrefab(GameObject prefab, bool setGuid, string newGuid)
+        {
+            if (setGuid)
+            {
+                ClientScene.RegisterPrefab(prefab, new Guid(newGuid));
+            }
+            else
+            {
+                ClientScene.RegisterPrefab(prefab);
+            }
+        }
+
+        [Test]
+        [TestCase(false)]
+        [TestCase(true)]
+        public void RegisterPrefab_Prefab_AddsPrefabToDictionary(bool setGuid)
+        {
+            Guid guid = setGuid ? new Guid(AnotherGuidString) : validPrefabGuid;
+
+            CallRegisterPrefab(validPrefab, setGuid, AnotherGuidString);
+
+            Assert.IsTrue(prefabs.ContainsKey(guid));
+            Assert.AreEqual(prefabs[guid], validPrefab);
+        }
+
+        [Test]
+        public void RegisterPrefab_PrefabNewGuid_ChangePrefabsAssetId()
+        {
+            Guid guid = new Guid(AnotherGuidString);
+            ClientScene.RegisterPrefab(validPrefab, guid);
+
+            Assert.IsTrue(prefabs.ContainsKey(guid));
+            Assert.AreEqual(prefabs[guid], validPrefab);
+
+            NetworkIdentity netId = prefabs[guid].GetComponent<NetworkIdentity>();
+
+            Assert.AreEqual(netId.assetId, guid);
+        }
+
+        [Test]
+        [TestCase(false)]
+        [TestCase(true)]
+        public void RegisterPrefab_Prefab_ErrorForNullPrefab(bool setGuid)
+        {
+            LogAssert.Expect(LogType.Error, "Could not register prefab because it was null");
+            CallRegisterPrefab(null, setGuid, AnotherGuidString);
+        }
+
+        [Test]
+        [TestCase(false)]
+        [TestCase(true)]
+        public void RegisterPrefab_Prefab_ErrorForPrefabWithoutNetworkIdentity(bool setGuid)
+        {
+            LogAssert.Expect(LogType.Error, $"Could not register '{invalidPrefab.name}' since it contains no NetworkIdentity component");
+            CallRegisterPrefab(invalidPrefab, setGuid, AnotherGuidString);
+        }
+
+        static void CreateSceneObject(out GameObject runtimeObject, out NetworkIdentity networkIdentity)
+        {
+            runtimeObject = new GameObject("Runtime GameObject");
+            networkIdentity = runtimeObject.AddComponent<NetworkIdentity>();
+            // set sceneId to zero as it is set in onvalidate (does not set id at runtime)
+            networkIdentity.sceneId = 0;
+        }
+
+        [Test]
+        public void RegisterPrefab_Prefab_ErrorForEmptyGuid()
+        {
+            CreateSceneObject(out GameObject runtimeObject, out NetworkIdentity networkIdentity);
+
+            //test
+            LogAssert.Expect(LogType.Error, $"Can not Register '{runtimeObject.name}' because it had empty assetid. If this is a scene Object use RegisterSpawnHandler instead");
+            ClientScene.RegisterPrefab(runtimeObject);
+
+            // teardown
+            GameObject.DestroyImmediate(runtimeObject);
+        }
+
+        [Test]
+        public void RegisterPrefab_PrefabNewGuid_AddsRuntimeObjectToDictionary()
+        {
+            CreateSceneObject(out GameObject runtimeObject, out NetworkIdentity networkIdentity);
+
+            Guid guid = new Guid(AnotherGuidString);
+            ClientScene.RegisterPrefab(runtimeObject, guid);
+
+            Assert.IsTrue(prefabs.ContainsKey(guid));
+            Assert.AreEqual(prefabs[guid], runtimeObject);
+
+            Assert.AreEqual(networkIdentity.assetId, guid);
+
+            // teardown
+            GameObject.DestroyImmediate(runtimeObject);
+        }
+
+        [Test]
+        public void RegisterPrefab_PrefabNewGuid_ErrorForEmptyGuid()
+        {
+            LogAssert.Expect(LogType.Error, $"Could not register '{validPrefab.name}' with new assetId because the new assetId was empty");
+            ClientScene.RegisterPrefab(validPrefab, new Guid());
+        }
+
+        [Test]
+        [TestCase(false)]
+        [TestCase(true)]
+        public void RegisterPrefab_Prefab_ErrorIfPrefabHadSceneId(bool setGuid)
+        {
+            GameObject clone = GameObject.Instantiate(validPrefab);
+            NetworkIdentity netId = clone.GetComponent<NetworkIdentity>();
+            // Scene Id needs to not be zero for this test
+            netId.sceneId = 20;
+
+            LogAssert.Expect(LogType.Error, $"Can not Register '{clone.name}' because it has a sceneId, make sure you are passing in the original prefab and not an instance in the scene.");
+            CallRegisterPrefab(clone, setGuid, AnotherGuidString);
+
+            GameObject.DestroyImmediate(clone);
+        }
+
+        [Test]
+        [TestCase(false)]
+        [TestCase(true)]
+        public void RegisterPrefab_Prefab_WarningForNetworkIdentityInChildren(bool setGuid)
+        {
+            LogAssert.Expect(LogType.Warning, $"Prefab '{prefabWithChildren.name}' has multiple NetworkIdentity components. There should only be one NetworkIdentity on a prefab, and it must be on the root object.");
+            CallRegisterPrefab(prefabWithChildren, setGuid, AnotherGuidString);
+        }
+
+
+        [Test]
+        [Ignore("Not Implemented")]
+        public void RegisterPrefab_Prefab_WarningForAssetIdAlreadyExistingInPrefabsDictionary()
+        {
+            // Not Implemented
+        }
+
+        [Test]
+        [Ignore("Not Implemented")]
+        public void RegisterPrefab_Prefab_WarningForAssetIdAlreadyExistingInHandlersDictionary()
+        {
+            // Not Implemented
+        }
+
+
         [Test]
         public void UnregisterPrefab_RemovesPrefabFromDictionary()
         {

diff --git a/Assets/Mirror/Tests/Editor/TestPrefabs/PrefabWithChildrenForClientScene.prefab b/Assets/Mirror/Tests/Editor/TestPrefabs/PrefabWithChildrenForClientScene.prefab
@@ -0,0 +1,97 @@
+%YAML 1.1
+%TAG !u! tag:unity3d.com,2011:
+--- !u!1 &3117426950187087154
+GameObject:
+  m_ObjectHideFlags: 0
+  m_CorrespondingSourceObject: {fileID: 0}
+  m_PrefabInstance: {fileID: 0}
+  m_PrefabAsset: {fileID: 0}
+  serializedVersion: 6
+  m_Component:
+  - component: {fileID: 51440471024079650}
+  - component: {fileID: 5409067437793000088}
+  m_Layer: 0
+  m_Name: PrefabWithChildrenForClientScene
+  m_TagString: Untagged
+  m_Icon: {fileID: 0}
+  m_NavMeshLayer: 0
+  m_StaticEditorFlags: 0
+  m_IsActive: 1
+--- !u!4 &51440471024079650
+Transform:
+  m_ObjectHideFlags: 0
+  m_CorrespondingSourceObject: {fileID: 0}
+  m_PrefabInstance: {fileID: 0}
+  m_PrefabAsset: {fileID: 0}
+  m_GameObject: {fileID: 3117426950187087154}
+  m_LocalRotation: {x: 0, y: 0, z: 0, w: 1}
+  m_LocalPosition: {x: 0, y: 0, z: 0}
+  m_LocalScale: {x: 1, y: 1, z: 1}
+  m_Children:
+  - {fileID: 6537489145038351880}
+  m_Father: {fileID: 0}
+  m_RootOrder: 0
+  m_LocalEulerAnglesHint: {x: 0, y: 0, z: 0}
+--- !u!114 &5409067437793000088
+MonoBehaviour:
+  m_ObjectHideFlags: 0
+  m_CorrespondingSourceObject: {fileID: 0}
+  m_PrefabInstance: {fileID: 0}
+  m_PrefabAsset: {fileID: 0}
+  m_GameObject: {fileID: 3117426950187087154}
+  m_Enabled: 1
+  m_EditorHideFlags: 0
+  m_Script: {fileID: 11500000, guid: 9b91ecbcc199f4492b9a91e820070131, type: 3}
+  m_Name: 
+  m_EditorClassIdentifier: 
+  sceneId: 4227710719
+  serverOnly: 0
+  m_AssetId: 
+  hasSpawned: 0
+--- !u!1 &3264653828050749140
+GameObject:
+  m_ObjectHideFlags: 0
+  m_CorrespondingSourceObject: {fileID: 0}
+  m_PrefabInstance: {fileID: 0}
+  m_PrefabAsset: {fileID: 0}
+  serializedVersion: 6
+  m_Component:
+  - component: {fileID: 6537489145038351880}
+  - component: {fileID: 6548650892574975460}
+  m_Layer: 0
+  m_Name: Child Network Identity
+  m_TagString: Untagged
+  m_Icon: {fileID: 0}
+  m_NavMeshLayer: 0
+  m_StaticEditorFlags: 0
+  m_IsActive: 1
+--- !u!4 &6537489145038351880
+Transform:
+  m_ObjectHideFlags: 0
+  m_CorrespondingSourceObject: {fileID: 0}
+  m_PrefabInstance: {fileID: 0}
+  m_PrefabAsset: {fileID: 0}
+  m_GameObject: {fileID: 3264653828050749140}
+  m_LocalRotation: {x: 0, y: 0, z: 0, w: 1}
+  m_LocalPosition: {x: 0, y: 0, z: 0}
+  m_LocalScale: {x: 1, y: 1, z: 1}
+  m_Children: []
+  m_Father: {fileID: 51440471024079650}
+  m_RootOrder: 0
+  m_LocalEulerAnglesHint: {x: 0, y: 0, z: 0}
+--- !u!114 &6548650892574975460
+MonoBehaviour:
+  m_ObjectHideFlags: 0
+  m_CorrespondingSourceObject: {fileID: 0}
+  m_PrefabInstance: {fileID: 0}
+  m_PrefabAsset: {fileID: 0}
+  m_GameObject: {fileID: 3264653828050749140}
+  m_Enabled: 1
+  m_EditorHideFlags: 0
+  m_Script: {fileID: 11500000, guid: 9b91ecbcc199f4492b9a91e820070131, type: 3}
+  m_Name: 
+  m_EditorClassIdentifier: 
+  sceneId: 1033643234
+  serverOnly: 0
+  m_AssetId: 
+  hasSpawned: 0
diff --git a/Assets/Mirror/Tests/Editor/TestPrefabs/PrefabWithChildrenForClientScene.prefab.meta b/Assets/Mirror/Tests/Editor/TestPrefabs/PrefabWithChildrenForClientScene.prefab.meta