fix and test

Fixing some things u-pr caught and adding a test to validate the update.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/Editor/Configuration/HybridNetcodeConfigApplier.cs

@@ -1,7 +1,9 @@
 #if UNIFIED_NETCODE
 using Unity.NetCode;
 using UnityEditor;
+using UnityEditor.SceneManagement;
 using UnityEngine;
+using UnityEngine.SceneManagement;
 
 namespace Unity.Netcode.GameObjects.Editor.Configuration
 {
@@ -20,6 +22,11 @@ private static void OnApplicationStart()
             // Cross-assembly ordering between the two is not a documented contract.
             // Defer rather than racing it.
             EditorApplication.delayCall += OnDelayCall;
+
+            // A NetworkManager in an unopened scene is not loaded, so its tick rate cannot be read at this point.
+            // Rescan when a scene opens to pick it up.
+            EditorSceneManager.sceneOpened -= OnSceneOpened;
+            EditorSceneManager.sceneOpened += OnSceneOpened;
         }
 
         private static void OnDelayCall()
@@ -28,6 +35,11 @@ private static void OnDelayCall()
             Apply(false);
         }
 
+        private static void OnSceneOpened(Scene scene, OpenSceneMode mode)
+        {
+            Apply(false);
+        }
+
         /// <summary>
         /// Adjusts <see cref="NetCodeConfig"/> for NGO hybrid mode.
         /// </summary>
@@ -61,6 +73,11 @@ internal static void Apply(bool applyRecommended)
                 {
                     Debug.Log($"[Netcode] Applied the NGO hybrid mode defaults to '{config.name}'. These are tuned for NGO and can be changed freely; they will not be re-applied automatically. Use Project Settings > Multiplayer > Netcode for GameObjects to restore them.", config);
                 }
+
+                // Recorded even when the config already matched and nothing was written. Leaving it unrecorded would
+                // make the next domain reload a first application again, which would revert the user's next edit.
+                settings.HybridDefaultsVersion = HybridNetcodeDefaults.Version;
+                settings.SaveSettings();
             }
             else
             {
@@ -80,48 +97,92 @@ internal static void Apply(bool applyRecommended)
                 return;
             }
 
-            settings.HybridDefaultsVersion = HybridNetcodeDefaults.Version;
-            settings.SaveSettings();
-
             EditorUtility.SetDirty(config);
             AssetDatabase.SaveAssetIfDirty(config);
         }
 
         /// <summary>
-        /// True when any <see cref="NetworkManager"/> in the project has a registered prefab carrying a ghost.
+        /// True when a registered network prefab carries a ghost.
         /// </summary>
+        /// <remarks>
+        /// The <see cref="NetworkPrefabsList"/> assets are scanned rather than the loaded <see cref="NetworkManager"/>s
+        /// because a manager living in an unopened scene is not loaded and would not be found. A ghost prefab sitting
+        /// in a list is treated as intent to run hybrid mode even if no manager references that list yet.
+        /// </remarks>
         internal static bool IsHybridProject()
         {
+            foreach (var guid in AssetDatabase.FindAssets($"t:{nameof(NetworkPrefabsList)}"))
+            {
+                var prefabsList = AssetDatabase.LoadAssetAtPath<NetworkPrefabsList>(AssetDatabase.GUIDToAssetPath(guid));
+                if (HasGhost(prefabsList))
+                {
+                    return true;
+                }
+            }
+
+            // Prefabs added directly to a NetworkManager never reach a list asset, so the loaded managers still have
+            // to be checked.
             foreach (var networkManager in Resources.FindObjectsOfTypeAll<NetworkManager>())
             {
-                var prefabs = networkManager.NetworkConfig?.Prefabs;
-                if (prefabs == null)
+                if (HasGhost(networkManager))
                 {
-                    continue;
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
+        /// <summary>
+        /// True when this <see cref="NetworkManager"/> registers a prefab carrying a ghost, either directly or through
+        /// one of its assigned <see cref="NetworkPrefabsList"/> assets.
+        /// </summary>
+        /// <param name="networkManager">The manager to inspect.</param>
+        /// <returns>Whether this manager takes part in hybrid mode.</returns>
+        private static bool HasGhost(NetworkManager networkManager)
+        {
+            var prefabs = networkManager == null ? null : networkManager.NetworkConfig?.Prefabs;
+            if (prefabs == null)
+            {
+                return false;
+            }
+
+            foreach (var prefab in prefabs.Prefabs)
+            {
+                if (HasGhost(prefab))
+                {
+                    return true;
                 }
+            }
 
-                foreach (var prefab in prefabs.Prefabs)
+            foreach (var prefabsList in prefabs.NetworkPrefabsLists)
+            {
+                if (HasGhost(prefabsList))
                 {
-                    if (HasGhost(prefab))
-                    {
-                        return true;
-                    }
+                    return true;
                 }
+            }
+
+            return false;
+        }
+
+        /// <summary>
+        /// True when this <see cref="NetworkPrefabsList"/> holds a prefab carrying a ghost.
+        /// </summary>
+        /// <param name="prefabsList">The list to inspect.</param>
+        /// <returns>Whether this list takes part in hybrid mode.</returns>
+        internal static bool HasGhost(NetworkPrefabsList prefabsList)
+        {
+            if (prefabsList == null)
+            {
+                return false;
+            }
 
-                foreach (var prefabsList in prefabs.NetworkPrefabsLists)
+            foreach (var prefab in prefabsList.PrefabList)
+            {
+                if (HasGhost(prefab))
                 {
-                    if (prefabsList == null)
-                    {
-                        continue;
-                    }
-
-                    foreach (var prefab in prefabsList.PrefabList)
-                    {
-                        if (HasGhost(prefab))
-                        {
-                            return true;
-                        }
-                    }
+                    return true;
                 }
             }
 
@@ -152,9 +213,13 @@ internal static NetCodeConfig ResolveGlobalConfig()
 
         /// <summary>
         /// Returns either the current N4E tick rate or the NGO <see cref="NetworkConfig.TickRate"/>.
-        /// If no NetworkManager is found yet, it returns N4E's tick rate.
-        /// If a NetworkManager is found, then it returns NGO's tick rate.
+        /// If no NetworkManager taking part in hybrid mode is loaded, it returns N4E's tick rate.
+        /// If one is loaded, then it returns NGO's tick rate.
         /// </summary>
+        /// <remarks>
+        /// Only managers registering a ghost prefab are considered. A conventional NGO manager running at a different
+        /// tick rate has no bearing on the interval N4E should synchronize ghosts at.
+        /// </remarks>
         /// <param name="config">The config, used as the fallback when no NetworkManager can be found.</param>
         /// <returns>The tick rate to write into the config.</returns>
         private static uint ResolveTickRate(NetCodeConfig config)
@@ -163,6 +228,11 @@ private static uint ResolveTickRate(NetCodeConfig config)
             var diverged = false;
             foreach (var networkManager in Resources.FindObjectsOfTypeAll<NetworkManager>())
             {
+                if (!HasGhost(networkManager))
+                {
+                    continue;
+                }
+
                 var tickRate = networkManager.NetworkConfig?.TickRate ?? 0u;
                 if (tickRate == 0)
                 {
@@ -175,36 +245,58 @@ private static uint ResolveTickRate(NetCodeConfig config)
 
             if (diverged)
             {
-                Debug.LogWarning($"[Netcode] Found {nameof(NetworkManager)}s with differing {nameof(NetworkConfig.TickRate)} values. '{config.name}' has been set to {found}; hybrid mode expects a single tick rate across the project.", config);
+                Debug.LogWarning($"[Netcode] Found hybrid mode {nameof(NetworkManager)}s with differing {nameof(NetworkConfig.TickRate)} values. '{config.name}' has been set to {found}; hybrid mode expects a single tick rate across the network prefabs carrying a ghost.", config);
             }
 
-            // No NetworkManager to read from (a prefab-only project, or one mid-import) leaves the config as it is.
+            // Nothing to read from (a prefab-only project, one mid-import, or the manager's scene is not open yet)
+            // leaves the config as it is. Opening that scene runs this again.
             return found != 0 ? found : (uint)config.ClientServerTickRate.SimulationTickRate;
         }
     }
 
     /// <summary>
-    /// Re-runs the hybrid config pass when a prefab import could have turned this into a hybrid project.
+    /// Re-runs the hybrid config pass when an import could have turned this into a hybrid project.
     /// </summary>
+    /// <remarks>
+    /// Both a prefab gaining a GhostObject and a prefab list gaining an existing ghost prefab reach hybrid mode, so
+    /// both imports are watched.
+    /// </remarks>
     internal class HybridNetcodeConfigPostprocessor : AssetPostprocessor
     {
         private static void OnPostprocessAllAssets(string[] importedAssets, string[] deletedAssets, string[] movedAssets, string[] movedFromAssetPaths)
         {
             foreach (var assetPath in importedAssets)
             {
-                if (AssetDatabase.GetMainAssetTypeAtPath(assetPath) != typeof(GameObject))
-                {
-                    continue;
-                }
-
-                var gameObject = AssetDatabase.LoadAssetAtPath<GameObject>(assetPath);
-                if (gameObject != null && gameObject.TryGetComponent<NetworkObject>(out var networkObject) && networkObject.HasGhost)
+                if (ImportReachesHybridMode(assetPath))
                 {
                     HybridNetcodeConfigApplier.Apply(false);
                     return;
                 }
             }
         }
+
+        /// <summary>
+        /// Cheap check for whether an imported asset could have introduced a ghost, so that the full project scan in
+        /// <see cref="HybridNetcodeConfigApplier.Apply"/> is only paid when it might matter.
+        /// </summary>
+        /// <param name="assetPath">The imported asset.</param>
+        /// <returns>Whether the import is worth a rescan.</returns>
+        private static bool ImportReachesHybridMode(string assetPath)
+        {
+            var assetType = AssetDatabase.GetMainAssetTypeAtPath(assetPath);
+            if (assetType == typeof(GameObject))
+            {
+                var gameObject = AssetDatabase.LoadAssetAtPath<GameObject>(assetPath);
+                return gameObject != null && gameObject.TryGetComponent<NetworkObject>(out var networkObject) && networkObject.HasGhost;
+            }
+
+            if (assetType == typeof(NetworkPrefabsList))
+            {
+                return HybridNetcodeConfigApplier.HasGhost(AssetDatabase.LoadAssetAtPath<NetworkPrefabsList>(assetPath));
+            }
+
+            return false;
+        }
     }
 }
 #endif

com.unity.netcode.gameobjects/Tests/Editor/HybridNetcodeDefaultsTests.cs

@@ -2,6 +2,7 @@
 using NUnit.Framework;
 using Unity.NetCode;
 using Unity.Netcode.GameObjects.Editor.Configuration;
+using UnityEditor;
 using UnityEngine;
 
 namespace Unity.Netcode.EditorTests
@@ -26,6 +27,17 @@ public void TearDown()
             Object.DestroyImmediate(m_Config);
         }
 
+        /// <summary>
+        /// A config that already matches reports no change, which is why the applier records the version marker
+        /// independently of whether anything was written.
+        /// </summary>
+        [Test]
+        public void ApplyRecommendedReportsNoChangeWhenConfigAlreadyMatches()
+        {
+            Assert.IsTrue(HybridNetcodeDefaults.ApplyRecommended(m_Config, 30u), "Expected the first apply to report a change.");
+            Assert.IsFalse(HybridNetcodeDefaults.ApplyRecommended(m_Config, 30u), "Applying an already matching config should report no change.");
+        }
+
         [Test]
         public void ApplyRequiredCorrectsBothSettings()
         {
@@ -66,6 +78,29 @@ public void ApplyTickRateLocksSimulationAndNetworkRates(uint tickRate)
             Assert.IsFalse(HybridNetcodeDefaults.ApplyTickRate(m_Config, tickRate));
         }
 
+        /// <summary>
+        /// The one-shot can run before the hybrid <see cref="NetworkManager"/>'s scene is open, in which case it writes
+        /// N4E's tick rate rather than NGO's. Opening that scene runs a tick rate only pass, which has to correct the
+        /// rate without disturbing the tuned values.
+        /// </summary>
+        [Test]
+        public void TickRateOnlyPassCorrectsTheRateAndLeavesTheTunedValuesAlone()
+        {
+            const int n4eTickRate = 60;
+            const uint ngoTickRate = 30;
+
+            HybridNetcodeDefaults.ApplyRecommended(m_Config, n4eTickRate);
+            Assume.That(m_Config.ClientServerTickRate.SimulationTickRate, Is.EqualTo(n4eTickRate), "The one-shot should have written N4E's tick rate.");
+
+            Assert.IsTrue(HybridNetcodeDefaults.ApplyTickRate(m_Config, ngoTickRate));
+
+            Assert.AreEqual((int)ngoTickRate, m_Config.ClientServerTickRate.SimulationTickRate);
+            Assert.AreEqual((int)ngoTickRate, m_Config.ClientServerTickRate.NetworkTickRate);
+            Assert.AreEqual(HybridNetcodeDefaults.SnapshotPacketSize, m_Config.GhostSendSystemData.DefaultSnapshotPacketSize, "A tick rate pass must not disturb the tuned values.");
+            Assert.AreEqual(HybridNetcodeDefaults.InterpolationTimeMS, m_Config.ClientTickRate.InterpolationTimeMS);
+            Assert.AreEqual(HybridNetcodeDefaults.InterpolationTimeScaleMax, m_Config.ClientTickRate.InterpolationTimeScaleMax);
+        }
+
         [Test]
         public void ApplyRecommendedProducesTheTunedValues()
         {
@@ -146,6 +181,62 @@ public void IsHybridProjectOnlyDetectsPrefabsCarryingAGhost()
                 Object.DestroyImmediate(managerObject);
             }
         }
+
+        /// <summary>
+        /// Once the one-shot has been recorded, every later pass still drives the tick rate from the hybrid
+        /// <see cref="NetworkManager"/>. This is what corrects the rate when the manager's scene opens after the
+        /// defaults were already applied.
+        /// </summary>
+        [Test]
+        public void ApplyDrivesTheTickRateAfterTheOneShotHasBeenRecorded()
+        {
+            const uint managerTickRate = 45;
+
+            var config = HybridNetcodeConfigApplier.ResolveGlobalConfig();
+            Assume.That(config, Is.Not.Null, "This project has no NetCodeConfig to adjust.");
+            Assume.That(HybridNetcodeConfigApplier.IsHybridProject(), Is.False, "Another loaded NetworkManager already registers a ghost prefab.");
+
+            var settings = NetcodeForGameObjectsProjectSettings.instance;
+            var restoreVersion = settings.HybridDefaultsVersion;
+            var restoreSimulation = config.ClientServerTickRate.SimulationTickRate;
+            var restoreNetwork = config.ClientServerTickRate.NetworkTickRate;
+
+            var managerObject = new GameObject(nameof(ApplyDrivesTheTickRateAfterTheOneShotHasBeenRecorded));
+            var prefabObject = new GameObject("GhostPrefab");
+            var prefabsList = ScriptableObject.CreateInstance<NetworkPrefabsList>();
+            try
+            {
+                var networkManager = managerObject.AddComponent<NetworkManager>();
+                networkManager.NetworkConfig = new NetworkConfig { TickRate = managerTickRate, };
+                prefabObject.AddComponent<NetworkObject>().HasGhost = true;
+                prefabsList.Add(new NetworkPrefab { Prefab = prefabObject });
+                networkManager.NetworkConfig.Prefabs.NetworkPrefabsLists.Add(prefabsList);
+
+                // Past the one-shot, so this exercises the required plus tick rate path rather than ApplyRecommended.
+                settings.HybridDefaultsVersion = HybridNetcodeDefaults.Version;
+                config.ClientServerTickRate.SimulationTickRate = 60;
+                config.ClientServerTickRate.NetworkTickRate = 60;
+
+                HybridNetcodeConfigApplier.Apply(false);
+
+                Assert.AreEqual((int)managerTickRate, config.ClientServerTickRate.SimulationTickRate, "The tick rate should have been driven from the hybrid NetworkManager.");
+                Assert.AreEqual((int)managerTickRate, config.ClientServerTickRate.NetworkTickRate);
+            }
+            finally
+            {
+                config.ClientServerTickRate.SimulationTickRate = restoreSimulation;
+                config.ClientServerTickRate.NetworkTickRate = restoreNetwork;
+                EditorUtility.SetDirty(config);
+                AssetDatabase.SaveAssetIfDirty(config);
+
+                settings.HybridDefaultsVersion = restoreVersion;
+                settings.SaveSettings();
+
+                Object.DestroyImmediate(prefabsList);
+                Object.DestroyImmediate(prefabObject);
+                Object.DestroyImmediate(managerObject);
+            }
+        }
     }
 }
 #endif