fix: improving error handling for Client spawning

- should not only give 1 error for failing to spawn instead of 2
- now throws SpawnObjectException instead of InvalidOperationException when failing to spawn
- GetPrefab throws ArgumentException if 0 is given
- GetPrefab throws SpawnObjectException if prefab not found

BREAKING CHANGE: ClientObjectManager.GetPrefab now throws instead of returning null

Assets/Mirage/Runtime/ClientObjectManager.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Text;
 using Cysharp.Threading.Tasks;
 using Mirage.Logging;
 using Mirage.RemoteCalls;
@@ -61,7 +62,7 @@ public void Start()
             }
             else
             {
-                Debug.LogWarning($"Client is null for ClientObjectManager on {this.gameObject.name}");
+                Debug.LogWarning($"Client is null for ClientObjectManager on {gameObject.name}");
             }
         }
 
@@ -204,16 +205,17 @@ private void RegisterSpawnPrefabs()
         /// </summary>
         /// <param name="prefabHash">asset id of the prefab</param>
         /// <returns>true if prefab was registered</returns>
+        /// <exception cref="ArgumentException">Thrown when <paramref name="prefabHash"/> is 0</exception>
+        /// <exception cref="SpawnObjectException">Thrown prefab </exception>
         public NetworkIdentity GetPrefab(int prefabHash)
         {
             if (prefabHash == 0)
-                return null;
+                throw new ArgumentException("prefabHash was 0", nameof(prefabHash));
 
             if (_prefabs.TryGetValue(prefabHash, out var identity))
-            {
                 return identity;
-            }
-            return null;
+
+            throw new SpawnObjectException($"No prefab for {prefabHash:X}. did you forget to add it to the ClientObjectManager?");
         }
 
         /// <summary>
@@ -417,9 +419,8 @@ private void ApplySpawnPayload(NetworkIdentity identity, SpawnMessage msg)
         internal void OnSpawn(SpawnMessage msg)
         {
             if (msg.prefabHash == null && msg.sceneId == null)
-            {
-                throw new InvalidOperationException($"OnSpawn has empty prefabHash and sceneId for netId: {msg.netId}");
-            }
+                throw new SpawnObjectException($"Empty prefabHash and sceneId for netId: {msg.netId}");
+
             if (logger.LogEnabled()) logger.Log($"Client spawn handler instantiating netId={msg.netId} prefabHash={msg.prefabHash:X} sceneId={msg.sceneId:X} pos={msg.position}");
 
             // was the object already spawned?
@@ -433,11 +434,8 @@ internal void OnSpawn(SpawnMessage msg)
                     : SpawnPrefab(msg);
             }
 
-            if (identity == null)
-            {
-                //object could not be found.
-                throw new InvalidOperationException($"Could not spawn prefabHash={msg.prefabHash:X} scene={msg.sceneId:X} netId={msg.netId}");
-            }
+            // should never happen, Spawn methods above should throw instead
+            Debug.Assert(identity != null);
 
             ApplySpawnPayload(identity, msg);
 
@@ -448,51 +446,51 @@ internal void OnSpawn(SpawnMessage msg)
 
         private NetworkIdentity SpawnPrefab(SpawnMessage msg)
         {
+            // try spawn handler first, then prefab after
             if (_spawnHandlers.TryGetValue(msg.prefabHash.Value, out var handler) && handler != null)
             {
-                var obj = handler(msg);
+                if (logger.LogEnabled()) logger.Log($"Client spawn with custom handler: [netId:{msg.netId} prefabHash:{msg.prefabHash:X} pos:{msg.position} rotation: {msg.rotation}]");
+
+                var obj = handler.Invoke(msg);
                 if (obj == null)
-                {
-                    logger.LogWarning($"Client spawn handler for {msg.prefabHash:X} returned null");
-                    return null;
-                }
+                    throw new SpawnObjectException($"Spawn handler for prefabHash={msg.prefabHash:X} returned null");
                 return obj;
             }
+
             var prefab = GetPrefab(msg.prefabHash.Value);
-            if (!(prefab is null))
-            {
-                // we need to set position and rotation here incase that their values can be used form awake/onenable
-                var pos = msg.position ?? prefab.transform.position;
-                var rot = msg.rotation ?? prefab.transform.rotation;
-                var obj = Instantiate(prefab, pos, rot);
-                if (logger.LogEnabled())
-                {
-                    logger.Log($"Client spawn handler instantiating [netId:{msg.netId} asset ID:{msg.prefabHash:X} pos:{msg.position} rotation: {msg.rotation}]");
-                }
 
-                return obj;
-            }
-            logger.LogError("Failed to spawn server object, did you forget to add it to the ClientObjectManager? prefabHash=" + msg.prefabHash + " netId=" + msg.netId);
-            return null;
+            if (logger.LogEnabled()) logger.Log($"Client spawn from prefab: [netId:{msg.netId} prefabHash:{msg.prefabHash:X} pos:{msg.position} rotation: {msg.rotation}]");
+
+            // we need to set position and rotation here incase that their values are used from awake/onenable
+            var pos = msg.position ?? prefab.transform.position;
+            var rot = msg.rotation ?? prefab.transform.rotation;
+            return Instantiate(prefab, pos, rot);
         }
 
         internal NetworkIdentity SpawnSceneObject(SpawnMessage msg)
         {
             var spawned = SpawnSceneObject(msg.sceneId.Value);
-            if (spawned == null)
+            if (spawned != null)
             {
-                logger.LogError($"Spawn scene object not found for {msg.sceneId:X} SpawnableObjects.Count={spawnableObjects.Count}");
+                if (logger.LogEnabled()) logger.Log($"Client spawn from scene object [netId:{msg.netId}] [sceneId:{msg.sceneId:X}] obj:{spawned}");
+                return spawned;
+            }
 
-                // dump the whole spawnable objects dict for easier debugging
-                if (logger.LogEnabled())
-                {
-                    foreach (var kvp in spawnableObjects)
-                        logger.Log($"Spawnable: SceneId={kvp.Key} name={kvp.Value.name}");
-                }
+            // failed to spawn
+            var errorMsg = $"Failed to spawn scene object sceneId={msg.sceneId:X}";
+            // dump the whole spawnable objects dict for easier debugging
+            if (logger.LogEnabled())
+            {
+                var builder = new StringBuilder();
+                builder.AppendLine($"{errorMsg} SpawnableObjects.Count={spawnableObjects.Count}");
+
+                foreach (var kvp in spawnableObjects)
+                    builder.AppendLine($"Spawnable: SceneId={kvp.Key} name={kvp.Value.name}");
+
+                logger.Log(builder.ToString());
             }
 
-            if (logger.LogEnabled()) logger.Log($"Client spawn for [netId:{msg.netId}] [sceneId:{msg.sceneId:X}] obj:{spawned}");
-            return spawned;
+            throw new SpawnObjectException(errorMsg);
         }
 
         private NetworkIdentity SpawnSceneObject(ulong sceneId)

Assets/Mirage/Runtime/SpawnObjectException.cs

@@ -0,0 +1,15 @@
+using System;
+
+namespace Mirage
+{
+    /// <summary>
+    /// Exception thrown when spawning fails
+    /// </summary>
+    [Serializable]
+    public class SpawnObjectException : Exception
+    {
+        public SpawnObjectException(string message) : base(message)
+        {
+        }
+    }
+}

Assets/Tests/Runtime/ClientServer/ClientObjectManagerTest.cs

@@ -2,7 +2,6 @@
 using System.Collections;
 using Cysharp.Threading.Tasks;
 using NUnit.Framework;
-using UnityEngine;
 using UnityEngine.TestTools;
 using Object = UnityEngine.Object;
 
@@ -11,18 +10,16 @@ namespace Mirage.Tests.Runtime.ClientServer
     [TestFixture]
     public class ClientObjectManagerTest : ClientServerSetup<MockComponent>
     {
-        private GameObject playerReplacement;
-
         [Test]
         public void OnSpawnAssetSceneIDFailureExceptionTest()
         {
             var msg = new SpawnMessage();
-            var ex = Assert.Throws<InvalidOperationException>(() =>
+            var ex = Assert.Throws<SpawnObjectException>(() =>
             {
                 clientObjectManager.OnSpawn(msg);
             });
 
-            Assert.That(ex.Message, Is.EqualTo($"OnSpawn has empty prefabHash and sceneId for netId: {msg.netId}"));
+            Assert.That(ex.Message, Is.EqualTo($"Empty prefabHash and sceneId for netId: {msg.netId}"));
         }
 
         [UnityTest]
@@ -112,19 +109,27 @@ private void TestUnspawnDelegate(NetworkIdentity identity)
         }
 
         [Test]
-        public void GetPrefabEmptyNullTest()
+        public void ThrwosWhenGetPrefabIsGivenZero()
         {
-            var result = clientObjectManager.GetPrefab(0);
+            var exception = Assert.Throws<ArgumentException>(() =>
+            {
+                var result = clientObjectManager.GetPrefab(0);
+            });
 
-            Assert.That(result, Is.Null);
+            var expected = new ArgumentException("prefabHash was 0", "prefabHash");
+            Assert.That(exception, Has.Message.EqualTo(expected.Message));
         }
 
         [Test]
-        public void GetPrefabNotFoundNullTest()
+        public void ThrowsWhenGetPrefabNotFound()
         {
-            var result = clientObjectManager.GetPrefab(NewUniqueHash());
+            var prefabHash = NewUniqueHash();
+            var exception = Assert.Throws<SpawnObjectException>(() =>
+            {
+                var result = clientObjectManager.GetPrefab(prefabHash);
+            });
 
-            Assert.That(result, Is.Null);
+            Assert.That(exception, Has.Message.EqualTo($"No prefab for {prefabHash:X}. did you forget to add it to the ClientObjectManager?"));
         }
 
         //Used to ensure the test has a unique non empty guid

Assets/Tests/Runtime/Host/ClientObjectManagerHostTest.cs

@@ -11,18 +11,6 @@ namespace Mirage.Tests.Runtime.Host
     [TestFixture]
     public class ClientObjectManagerHostTest : HostSetup<MockComponent>
     {
-        [Test]
-        public void OnSpawnAssetSceneIDFailureExceptionTest()
-        {
-            var msg = new SpawnMessage();
-            var ex = Assert.Throws<InvalidOperationException>(() =>
-            {
-                clientObjectManager.OnSpawn(msg);
-            });
-
-            Assert.That(ex.Message, Is.EqualTo($"OnSpawn has empty prefabHash and sceneId for netId: {msg.netId}"));
-        }
-
         [UnityTest]
         public IEnumerator GetPrefabTest() => UniTask.ToCoroutine(async () =>
         {
@@ -106,22 +94,6 @@ private void TestUnspawnDelegate(NetworkIdentity identity)
             Object.Destroy(identity.gameObject);
         }
 
-        [Test]
-        public void GetPrefabEmptyNullTest()
-        {
-            var result = clientObjectManager.GetPrefab(0);
-
-            Assert.That(result, Is.Null);
-        }
-
-        [Test]
-        public void GetPrefabNotFoundNullTest()
-        {
-            var result = clientObjectManager.GetPrefab(NewUniqueHash());
-
-            Assert.That(result, Is.Null);
-        }
-
         //Used to ensure the test has a unique non empty guid
         private int NewUniqueHash()
         {