fix: player prefab null exception when using prefab hash via connection approval (#3042)

* fix

fixing the issue where setting the prefab hash but not assigning a player prefab would cause an exception.

* update

change log entry

* test

Updating the ConnectionApproval test to include spawning from player prefab, prefab hash, spawning no player, and failing validation.

* update

updating changelog with PR number.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -19,6 +19,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where setting a prefab hash value during connection approval but not having a player prefab assigned could cause an exception when spawning a player. (#3042)
 - Fixed issue where the `NetworkSpawnManager.HandleNetworkObjectShow` could throw an exception if one of the `NetworkObject` components to show was destroyed during the same frame. (#3030)
 - Fixed issue where the `NetworkManagerHelper` was continuing to check for hierarchy changes when in play mode. (#3026)
 - Fixed issue with newly/late joined clients and `NetworkTransform` synchronization of parented `NetworkObject` instances. (#3013)

com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs

@@ -735,42 +735,23 @@ internal void HandleConnectionApproval(ulong ownerClientId, NetworkManager.Conne
                 RemovePendingClient(ownerClientId);
 
                 var client = AddClient(ownerClientId);
-                if (!NetworkManager.DistributedAuthorityMode && response.CreatePlayerObject && NetworkManager.NetworkConfig.PlayerPrefab != null)
-                {
-                    var prefabNetworkObject = NetworkManager.NetworkConfig.PlayerPrefab.GetComponent<NetworkObject>();
-                    var playerPrefabHash = response.PlayerPrefabHash ?? prefabNetworkObject.GlobalObjectIdHash;
-
-                    // Generate a SceneObject for the player object to spawn
-                    // Note: This is only to create the local NetworkObject, many of the serialized properties of the player prefab will be set when instantiated.
-                    var sceneObject = new NetworkObject.SceneObject
-                    {
-                        OwnerClientId = ownerClientId,
-                        IsPlayerObject = true,
-                        IsSceneObject = false,
-                        HasTransform = prefabNetworkObject.SynchronizeTransform,
-                        Hash = playerPrefabHash,
-                        TargetClientId = ownerClientId,
-                        DontDestroyWithOwner = prefabNetworkObject.DontDestroyWithOwner,
-                        Transform = new NetworkObject.SceneObject.TransformData
-                        {
-                            Position = response.Position.GetValueOrDefault(),
-                            Rotation = response.Rotation.GetValueOrDefault()
-                        }
-                    };
 
-                    // Create the player NetworkObject locally
-                    var networkObject = NetworkManager.SpawnManager.CreateLocalNetworkObject(sceneObject);
+                // Server-side spawning (only if there is a prefab hash or player prefab provided)
+                if (!NetworkManager.DistributedAuthorityMode && response.CreatePlayerObject && (response.PlayerPrefabHash.HasValue || NetworkManager.NetworkConfig.PlayerPrefab != null))
+                {
+                    var playerObject = response.PlayerPrefabHash.HasValue ? NetworkManager.SpawnManager.GetNetworkObjectToSpawn(response.PlayerPrefabHash.Value, ownerClientId, response.Position.GetValueOrDefault(), response.Rotation.GetValueOrDefault())
+                        : NetworkManager.SpawnManager.GetNetworkObjectToSpawn(NetworkManager.NetworkConfig.PlayerPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash, ownerClientId, response.Position.GetValueOrDefault(), response.Rotation.GetValueOrDefault());
 
                     // Spawn the player NetworkObject locally
                     NetworkManager.SpawnManager.SpawnNetworkObjectLocally(
-                        networkObject,
+                        playerObject,
                         NetworkManager.SpawnManager.GetNetworkObjectId(),
                         sceneObject: false,
                         playerObject: true,
                         ownerClientId,
                         destroyWithScene: false);
 
-                    client.AssignPlayerObject(ref networkObject);
+                    client.AssignPlayerObject(ref playerObject);
                 }
 
                 // Server doesn't send itself the connection approved message
@@ -871,6 +852,7 @@ internal void HandleConnectionApproval(ulong ownerClientId, NetworkManager.Conne
                     }
                 }
 
+                // Exit early if no player object was spawned
                 if (!response.CreatePlayerObject || (response.PlayerPrefabHash == null && NetworkManager.NetworkConfig.PlayerPrefab == null))
                 {
                     return;

com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs

@@ -850,6 +850,12 @@ protected void ClientNetworkManagerPostStartInit()
 
         protected virtual bool LogAllMessages => false;
 
+        protected virtual bool ShouldCheckForSpawnedPlayers()
+        {
+            return true;
+        }
+
+
         /// <summary>
         /// This starts the server and clients as long as <see cref="CanStartServerAndClients"/>
         /// returns true.
@@ -938,7 +944,12 @@ protected IEnumerator StartServerAndClients()
                             AssertOnTimeout($"{nameof(CreateAndStartNewClient)} timed out waiting for all sessions to spawn Client-{m_ServerNetworkManager.LocalClientId}'s player object!\n {m_InternalErrorLog}");
                         }
                     }
-                    ClientNetworkManagerPostStartInit();
+
+                    if (ShouldCheckForSpawnedPlayers())
+                    {
+                        ClientNetworkManagerPostStartInit();
+                    }
+
                     // Notification that at this time the server and client(s) are instantiated,
                     // started, and connected on both sides.
                     yield return OnServerAndClientsConnected();
@@ -1030,7 +1041,10 @@ protected void StartServerAndClientsWithTimeTravel()
                         }
                     }
 
-                    ClientNetworkManagerPostStartInit();
+                    if (ShouldCheckForSpawnedPlayers())
+                    {
+                        ClientNetworkManagerPostStartInit();
+                    }
 
                     // Notification that at this time the server and client(s) are instantiated,
                     // started, and connected on both sides.

com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApproval.cs

@@ -1,65 +1,135 @@
 using System;
 using System.Collections;
+using System.Collections.Generic;
 using System.Text;
 using NUnit.Framework;
 using Unity.Netcode.TestHelpers.Runtime;
-using UnityEngine;
 using UnityEngine.TestTools;
 
 namespace Unity.Netcode.RuntimeTests
 {
-    internal class ConnectionApprovalTests
+    [TestFixture(PlayerCreation.Prefab)]
+    [TestFixture(PlayerCreation.PrefabHash)]
+    [TestFixture(PlayerCreation.NoPlayer)]
+    [TestFixture(PlayerCreation.FailValidation)]
+    internal class ConnectionApprovalTests : NetcodeIntegrationTest
     {
+        private const string k_InvalidToken = "Invalid validation token!";
+        public enum PlayerCreation
+        {
+            Prefab,
+            PrefabHash,
+            NoPlayer,
+            FailValidation
+        }
+        private PlayerCreation m_PlayerCreation;
+        private bool m_ClientDisconnectReasonValidated;
+
+        private Dictionary<ulong, bool> m_Validated = new Dictionary<ulong, bool>();
+
+        public ConnectionApprovalTests(PlayerCreation playerCreation)
+        {
+            m_PlayerCreation = playerCreation;
+        }
+
+        protected override int NumberOfClients => 1;
+
         private Guid m_ValidationToken;
-        private bool m_IsValidated;
 
-        [SetUp]
-        public void Setup()
+        protected override bool ShouldCheckForSpawnedPlayers()
+        {
+            return m_PlayerCreation != PlayerCreation.NoPlayer;
+        }
+
+        protected override void OnServerAndClientsCreated()
         {
-            // Create, instantiate, and host
-            Assert.IsTrue(NetworkManagerHelper.StartNetworkManager(out _, NetworkManagerHelper.NetworkManagerOperatingMode.None));
+            m_ClientDisconnectReasonValidated = false;
+            m_BypassConnectionTimeout = m_PlayerCreation == PlayerCreation.FailValidation;
+            m_Validated.Clear();
             m_ValidationToken = Guid.NewGuid();
+            var validationToken = Encoding.UTF8.GetBytes(m_ValidationToken.ToString());
+            m_ServerNetworkManager.ConnectionApprovalCallback = NetworkManagerObject_ConnectionApprovalCallback;
+            m_ServerNetworkManager.NetworkConfig.PlayerPrefab = m_PlayerCreation == PlayerCreation.Prefab ? m_PlayerPrefab : null;
+            if (m_PlayerCreation == PlayerCreation.PrefabHash)
+            {
+                m_ServerNetworkManager.NetworkConfig.Prefabs.Add(new NetworkPrefab() { Prefab = m_PlayerPrefab });
+            }
+            m_ServerNetworkManager.NetworkConfig.ConnectionApproval = true;
+            m_ServerNetworkManager.NetworkConfig.ConnectionData = validationToken;
+
+            foreach (var client in m_ClientNetworkManagers)
+            {
+                client.NetworkConfig.PlayerPrefab = m_PlayerCreation == PlayerCreation.Prefab ? m_PlayerPrefab : null;
+                if (m_PlayerCreation == PlayerCreation.PrefabHash)
+                {
+                    client.NetworkConfig.Prefabs.Add(new NetworkPrefab() { Prefab = m_PlayerPrefab });
+                }
+                client.NetworkConfig.ConnectionApproval = true;
+                client.NetworkConfig.ConnectionData = m_PlayerCreation == PlayerCreation.FailValidation ? Encoding.UTF8.GetBytes(Guid.NewGuid().ToString()) : validationToken;
+                if (m_PlayerCreation == PlayerCreation.FailValidation)
+                {
+                    client.OnClientDisconnectCallback += Client_OnClientDisconnectCallback;
+                }
+            }
+
+            base.OnServerAndClientsCreated();
         }
 
-        [UnityTest]
-        public IEnumerator ConnectionApproval()
+        private void Client_OnClientDisconnectCallback(ulong clientId)
+        {
+            m_ClientNetworkManagers[0].OnClientDisconnectCallback -= Client_OnClientDisconnectCallback;
+            m_ClientDisconnectReasonValidated = m_ClientNetworkManagers[0].LocalClientId == clientId && m_ClientNetworkManagers[0].DisconnectReason == k_InvalidToken;
+        }
+
+        private bool ClientAndHostValidated()
         {
-            NetworkManagerHelper.NetworkManagerObject.ConnectionApprovalCallback = NetworkManagerObject_ConnectionApprovalCallback;
-            NetworkManagerHelper.NetworkManagerObject.NetworkConfig.ConnectionApproval = true;
-            NetworkManagerHelper.NetworkManagerObject.NetworkConfig.PlayerPrefab = null;
-            NetworkManagerHelper.NetworkManagerObject.NetworkConfig.ConnectionData = Encoding.UTF8.GetBytes(m_ValidationToken.ToString());
-            m_IsValidated = false;
-            NetworkManagerHelper.NetworkManagerObject.StartHost();
-
-            var timeOut = Time.realtimeSinceStartup + 3.0f;
-            var timedOut = false;
-            while (!m_IsValidated)
+            if (!m_Validated.ContainsKey(m_ServerNetworkManager.LocalClientId) || !m_Validated[m_ServerNetworkManager.LocalClientId])
             {
-                yield return new WaitForSeconds(0.01f);
-                if (timeOut < Time.realtimeSinceStartup)
+                return false;
+            }
+            if (m_PlayerCreation == PlayerCreation.FailValidation)
+            {
+                return m_ClientDisconnectReasonValidated;
+            }
+            else
+            {
+                foreach (var client in m_ClientNetworkManagers)
                 {
-                    timedOut = true;
+                    if (!m_Validated.ContainsKey(client.LocalClientId) || !m_Validated[client.LocalClientId])
+                    {
+                        return false;
+                    }
                 }
             }
+            return true;
+        }
 
-            //Make sure we didn't time out
-            Assert.False(timedOut);
-            Assert.True(m_IsValidated);
+        [UnityTest]
+        public IEnumerator ConnectionApproval()
+        {
+            yield return WaitForConditionOrTimeOut(ClientAndHostValidated);
+            AssertOnTimeout("Timed out waiting for all clients to be approved!");
         }
 
         private void NetworkManagerObject_ConnectionApprovalCallback(NetworkManager.ConnectionApprovalRequest request, NetworkManager.ConnectionApprovalResponse response)
         {
             var stringGuid = Encoding.UTF8.GetString(request.Payload);
+
             if (m_ValidationToken.ToString() == stringGuid)
             {
-                m_IsValidated = true;
+                m_Validated.Add(request.ClientNetworkId, true);
+                response.Approved = true;
+            }
+            else
+            {
+                response.Approved = false;
+                response.Reason = "Invalid validation token!";
             }
 
-            response.Approved = m_IsValidated;
-            response.CreatePlayerObject = false;
+            response.CreatePlayerObject = ShouldCheckForSpawnedPlayers();
             response.Position = null;
             response.Rotation = null;
-            response.PlayerPrefabHash = null;
+            response.PlayerPrefabHash = m_PlayerCreation == PlayerCreation.PrefabHash ? m_PlayerPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash : null;
         }
 
 
@@ -78,13 +148,6 @@ public void VerifyUniqueNetworkConfigPerRequest()
 
             Assert.True(currentHash != newHash, $"Hashed {nameof(NetworkConfig)} values {currentHash} and {newHash} should not be the same!");
         }
-
-        [TearDown]
-        public void TearDown()
-        {
-            // Stop, shutdown, and destroy
-            NetworkManagerHelper.ShutdownNetworkManager();
-        }
-
     }
 }
+