fix: Migrate OnClientConnectedCallback invocation into StartHost [MTT-4972] (Unity-Technologies#2277)

* fix
Migrate InvokeOnClientConnectedCallback into StartHost (for host only).

* test
Updating the NetworkManagerTests to validate this PR, it also validates that this update does not impact anything when scene management is disabled.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -11,6 +11,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed the issue where `NetworkManager.OnClientConnectedCallback` was being invoked before in-scene placed `NetworkObject`s had been spawned when starting `NetworkManager` as a host. (#2277)
 - Creating a `FastBufferReader` with `Allocator.None` will not result in extra memory being allocated for the buffer (since it's owned externally in that scenario). (#2265)
 
 ## [1.1.0] - 2022-10-21

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

@@ -1188,6 +1188,11 @@ public bool StartHost()
 
             SpawnManager.ServerSpawnSceneObjectsOnStartSweep();
 
+            // This assures that any in-scene placed NetworkObject is spawned and
+            // any associated NetworkBehaviours' netcode related properties are
+            // set prior to invoking OnClientConnected.
+            InvokeOnClientConnectedCallback(LocalClientId);
+
             OnServerStarted?.Invoke();
 
             return true;
@@ -2226,7 +2231,6 @@ internal void HandleConnectionApproval(ulong ownerClientId, ConnectionApprovalRe
                 {
                     LocalClient = client;
                     SpawnManager.UpdateObservedNetworkObjects(ownerClientId);
-                    InvokeOnClientConnectedCallback(ownerClientId);
                 }
 
                 if (!response.CreatePlayerObject || (response.PlayerPrefabHash == null && NetworkConfig.PlayerPrefab == null))

testproject/Assets/Tests/Runtime/NetworkManagerTests.cs

@@ -1,17 +1,95 @@
 using NUnit.Framework;
+using UnityEngine;
+using UnityEngine.SceneManagement;
+using Unity.Netcode;
 using Unity.Netcode.TestHelpers.Runtime;
-
+using System.Collections;
 
 namespace TestProject.RuntimeTests
 {
+    [TestFixture(UseSceneManagement.SceneManagementDisabled)]
+    [TestFixture(UseSceneManagement.SceneManagementEnabled)]
     public class NetworkManagerTests : NetcodeIntegrationTest
     {
-        protected override int NumberOfClients => 1;
+        private const string k_SceneToLoad = "InSceneNetworkObject";
+        protected override int NumberOfClients => 0;
+
+        public enum UseSceneManagement
+        {
+            SceneManagementEnabled,
+            SceneManagementDisabled
+        }
+
+        private bool m_EnableSceneManagement;
+        private NetworkObject m_NetworkObject;
+        private bool m_NetworkObjectWasSpawned;
+        private bool m_NetworkBehaviourIsHostWasSet;
+        private bool m_NetworkBehaviourIsClientWasSet;
+        private bool m_NetworkBehaviourIsServerWasSet;
+        private int m_NumberOfTimesInvoked;
+        private AsyncOperation m_AsyncOperation;
+        private NetworkObjectTestComponent m_NetworkObjectTestComponent;
+
+        private bool m_UseSceneManagement;
+
+        public NetworkManagerTests(UseSceneManagement useSceneManagement)
+        {
+            m_UseSceneManagement = useSceneManagement == UseSceneManagement.SceneManagementEnabled;
+        }
+
+        private void OnClientConnectedCallback(NetworkObject networkObject, int numberOfTimesInvoked, bool isHost, bool isClient, bool isServer)
+        {
+            m_NetworkObject = networkObject;
+            m_NetworkObjectWasSpawned = networkObject.IsSpawned;
+            m_NetworkBehaviourIsHostWasSet = isHost;
+            m_NetworkBehaviourIsClientWasSet = isClient;
+            m_NetworkBehaviourIsServerWasSet = isServer;
+            m_NumberOfTimesInvoked = numberOfTimesInvoked;
+        }
+
+        private bool TestComponentFound()
+        {
+            if (!m_AsyncOperation.isDone)
+            {
+                return false;
+            }
+
+            m_NetworkObjectTestComponent = Object.FindObjectOfType<NetworkObjectTestComponent>();
+            if (m_NetworkObjectTestComponent == null)
+            {
+                return false;
+            }
+            return true;
+        }
+
+        protected override IEnumerator OnSetup()
+        {
+            m_AsyncOperation = SceneManager.LoadSceneAsync(k_SceneToLoad, LoadSceneMode.Additive);
+            yield return WaitForConditionOrTimeOut(TestComponentFound);
+            AssertOnTimeout($"Failed to find {nameof(NetworkObjectTestComponent)} after loading test scene {k_SceneToLoad}");
+        }
+
+        protected override IEnumerator OnTearDown()
+        {
+            SceneManager.UnloadSceneAsync(SceneManager.GetSceneByName(k_SceneToLoad));
+            yield return s_DefaultWaitForTick;
+        }
+
+        protected override void OnServerAndClientsCreated()
+        {
+            m_ServerNetworkManager.NetworkConfig.EnableSceneManagement = m_EnableSceneManagement;
+            m_NetworkObjectTestComponent.ConfigureClientConnected(m_ServerNetworkManager, OnClientConnectedCallback);
+        }
 
         [Test]
-        public void ValidateHostLocalClient()
+        public void ValidateHostSettings()
         {
             Assert.IsTrue(m_ServerNetworkManager.LocalClient != null);
+            Assert.IsTrue(m_NetworkObjectWasSpawned, $"{m_NetworkObject.name} was not spawned when OnClientConnectedCallback was invoked!");
+            Assert.IsTrue(m_NetworkBehaviourIsHostWasSet, $"IsHost was not true when OnClientConnectedCallback was invoked!");
+            Assert.IsTrue(m_NetworkBehaviourIsClientWasSet, $"IsClient was not true when OnClientConnectedCallback was invoked!");
+            Assert.IsTrue(m_NumberOfTimesInvoked == 1, $"OnClientConnectedCallback was invoked {m_NumberOfTimesInvoked} as opposed to just once!");
+            Assert.IsTrue(m_NetworkBehaviourIsServerWasSet, $"IsServer was not true when OnClientConnectedCallback was invoked!");
         }
     }
 }

testproject/Assets/Tests/Runtime/NetworkSceneManager/NetworkObjectTestComponent.cs

@@ -27,6 +27,23 @@ public static void Reset()
             DespawnedInstances.Clear();
         }
 
+        private Action<NetworkObject, int, bool, bool, bool> m_ActionClientConnected;
+        private int m_NumberOfTimesInvoked;
+        public void ConfigureClientConnected(NetworkManager networkManager, Action<NetworkObject, int, bool, bool, bool> clientConnected)
+        {
+            networkManager.OnClientConnectedCallback += NetworkManager_OnClientConnectedCallback;
+            m_ActionClientConnected = clientConnected;
+        }
+
+        private void NetworkManager_OnClientConnectedCallback(ulong obj)
+        {
+            m_NumberOfTimesInvoked++;
+            if (m_ActionClientConnected != null)
+            {
+                m_ActionClientConnected.Invoke(NetworkObject, m_NumberOfTimesInvoked, IsHost, IsClient, IsServer);
+            }
+        }
+
         // When disabling on spawning we only want this to happen on the initial spawn.
         // This is used to track this so the server only does it once upon spawning.
         public bool ObjectWasDisabledUponSpawn;