From c2d834315cc9ad3805bd6230fa9f35081ddfbff2 Mon Sep 17 00:00:00 2001 From: Robin Rolf Date: Thu, 16 Feb 2023 12:11:54 +0100 Subject: [PATCH] cleanup: move observer HashSet rebuild logic to IM (#3383) This cleanup step prepares for InterestManagement classes being able to customize the rebuild logic, but for that we should move it out of the NetworkServer class --- Assets/Mirror/Core/InterestManagement.cs | 100 +++++++++++++++++ Assets/Mirror/Core/NetworkServer.cs | 105 +----------------- .../Mirror/Tests/Editor/NetworkServerTest.cs | 1 - 3 files changed, 102 insertions(+), 104 deletions(-) diff --git a/Assets/Mirror/Core/InterestManagement.cs b/Assets/Mirror/Core/InterestManagement.cs index 85568317406..d18db34e1a0 100644 --- a/Assets/Mirror/Core/InterestManagement.cs +++ b/Assets/Mirror/Core/InterestManagement.cs @@ -1,5 +1,6 @@ // interest management component for custom solutions like // distance based, spatial hashing, raycast based, etc. + using System.Collections.Generic; using UnityEngine; @@ -9,6 +10,10 @@ namespace Mirror [HelpURL("https://mirror-networking.gitbook.io/docs/guides/interest-management")] public abstract class InterestManagement : MonoBehaviour { + // allocate newObservers helper HashSet + readonly HashSet newObservers = + new HashSet(); + // Awake configures InterestManagement in NetworkServer/Client // Do NOT check for active server or client here. // Awake must always set the static aoi references. @@ -96,5 +101,100 @@ public virtual void OnSpawned(NetworkIdentity identity) {} // (useful for 'only rebuild if changed' interest management algorithms) [ServerCallback] public virtual void OnDestroyed(NetworkIdentity identity) {} + + public void Rebuild(NetworkIdentity identity, bool initialize) + { + // clear newObservers hashset before using it + newObservers.Clear(); + + // not force hidden? + if (identity.visible != Visibility.ForceHidden) + { + OnRebuildObservers(identity, newObservers); + } + + // IMPORTANT: AFTER rebuilding add own player connection in any case + // to ensure player always sees himself no matter what. + // -> OnRebuildObservers might clear observers, so we need to add + // the player's own connection AFTER. 100% fail safe. + // -> fixes https://github.com/vis2k/Mirror/issues/692 where a + // player might teleport out of the ProximityChecker's cast, + // losing the own connection as observer. + if (identity.connectionToClient != null) + { + newObservers.Add(identity.connectionToClient); + } + + bool changed = false; + + // add all newObservers that aren't in .observers yet + foreach (NetworkConnectionToClient conn in newObservers) + { + // only add ready connections. + // otherwise the player might not be in the world yet or anymore + if (conn != null && conn.isReady) + { + if (initialize || !identity.observers.ContainsKey(conn.connectionId)) + { + // new observer + conn.AddToObserving(identity); + // Debug.Log($"New Observer for {gameObject} {conn}"); + changed = true; + } + } + } + + // remove all old .observers that aren't in newObservers anymore + foreach (NetworkConnectionToClient conn in identity.observers.Values) + { + if (!newObservers.Contains(conn)) + { + // removed observer + conn.RemoveFromObserving(identity, false); + // Debug.Log($"Removed Observer for {gameObject} {conn}"); + changed = true; + } + } + + // copy new observers to observers + if (changed) + { + identity.observers.Clear(); + foreach (NetworkConnectionToClient conn in newObservers) + { + if (conn != null && conn.isReady) + identity.observers.Add(conn.connectionId, conn); + } + } + + // special case for host mode: we use SetHostVisibility to hide + // NetworkIdentities that aren't in observer range from host. + // this is what games like Dota/Counter-Strike do too, where a host + // does NOT see all players by default. they are in memory, but + // hidden to the host player. + // + // this code is from UNET, it's a bit strange but it works: + // * it hides newly connected identities in host mode + // => that part was the intended behaviour + // * it hides ALL NetworkIdentities in host mode when the host + // connects but hasn't selected a character yet + // => this only works because we have no .localConnection != null + // check. at this stage, localConnection is null because + // StartHost starts the server first, then calls this code, + // then starts the client and sets .localConnection. so we can + // NOT add a null check without breaking host visibility here. + // * it hides ALL NetworkIdentities in server-only mode because + // observers never contain the 'null' .localConnection + // => that was not intended, but let's keep it as it is so we + // don't break anything in host mode. it's way easier than + // iterating all identities in a special function in StartHost. + if (initialize) + { + if (!newObservers.Contains(NetworkServer.localConnection)) + { + SetHostVisibility(identity, false); + } + } + } } } diff --git a/Assets/Mirror/Core/NetworkServer.cs b/Assets/Mirror/Core/NetworkServer.cs index e96a428a03e..39d60b062b6 100644 --- a/Assets/Mirror/Core/NetworkServer.cs +++ b/Assets/Mirror/Core/NetworkServer.cs @@ -189,7 +189,6 @@ public static void Shutdown() connections.Clear(); connectionsCopy.Clear(); handlers.Clear(); - newObservers.Clear(); // destroy all spawned objects, _then_ set inactive. // make sure .active is still true before calling this. @@ -1531,14 +1530,10 @@ static void DestroyObject(NetworkIdentity identity, DestroyMode mode) } // interest management ///////////////////////////////////////////////// + // Helper function to add all server connections as observers. // This is used if none of the components provides their own // OnRebuildObservers function. - // allocate newObservers helper HashSet only once - // internal for tests - internal static readonly HashSet newObservers = - new HashSet(); - // rebuild observers default method (no AOI) - adds all connections static void RebuildObserversDefault(NetworkIdentity identity, bool initialize) { @@ -1597,106 +1592,10 @@ public static void RebuildObservers(NetworkIdentity identity, bool initialize) // otherwise let interest management system rebuild else { - RebuildObserversCustom(identity, initialize); + aoi.Rebuild(identity, initialize); } } - // rebuild observers via interest management system - static void RebuildObserversCustom(NetworkIdentity identity, bool initialize) - { - // clear newObservers hashset before using it - newObservers.Clear(); - - // not force hidden? - if (identity.visible != Visibility.ForceHidden) - { - aoi.OnRebuildObservers(identity, newObservers); - } - - // IMPORTANT: AFTER rebuilding add own player connection in any case - // to ensure player always sees himself no matter what. - // -> OnRebuildObservers might clear observers, so we need to add - // the player's own connection AFTER. 100% fail safe. - // -> fixes https://github.com/vis2k/Mirror/issues/692 where a - // player might teleport out of the ProximityChecker's cast, - // losing the own connection as observer. - if (identity.connectionToClient != null) - { - newObservers.Add(identity.connectionToClient); - } - - bool changed = false; - - // add all newObservers that aren't in .observers yet - foreach (NetworkConnectionToClient conn in newObservers) - { - // only add ready connections. - // otherwise the player might not be in the world yet or anymore - if (conn != null && conn.isReady) - { - if (initialize || !identity.observers.ContainsKey(conn.connectionId)) - { - // new observer - conn.AddToObserving(identity); - // Debug.Log($"New Observer for {gameObject} {conn}"); - changed = true; - } - } - } - - // remove all old .observers that aren't in newObservers anymore - foreach (NetworkConnectionToClient conn in identity.observers.Values) - { - if (!newObservers.Contains(conn)) - { - // removed observer - conn.RemoveFromObserving(identity, false); - // Debug.Log($"Removed Observer for {gameObject} {conn}"); - changed = true; - } - } - - // copy new observers to observers - if (changed) - { - identity.observers.Clear(); - foreach (NetworkConnectionToClient conn in newObservers) - { - if (conn != null && conn.isReady) - identity.observers.Add(conn.connectionId, conn); - } - } - - // special case for host mode: we use SetHostVisibility to hide - // NetworkIdentities that aren't in observer range from host. - // this is what games like Dota/Counter-Strike do too, where a host - // does NOT see all players by default. they are in memory, but - // hidden to the host player. - // - // this code is from UNET, it's a bit strange but it works: - // * it hides newly connected identities in host mode - // => that part was the intended behaviour - // * it hides ALL NetworkIdentities in host mode when the host - // connects but hasn't selected a character yet - // => this only works because we have no .localConnection != null - // check. at this stage, localConnection is null because - // StartHost starts the server first, then calls this code, - // then starts the client and sets .localConnection. so we can - // NOT add a null check without breaking host visibility here. - // * it hides ALL NetworkIdentities in server-only mode because - // observers never contain the 'null' .localConnection - // => that was not intended, but let's keep it as it is so we - // don't break anything in host mode. it's way easier than - // iterating all identities in a special function in StartHost. - if (initialize) - { - if (!newObservers.Contains(localConnection)) - { - if (aoi != null) - aoi.SetHostVisibility(identity, false); - } - } - } // broadcasting //////////////////////////////////////////////////////// // helper function to get the right serialization for a connection diff --git a/Assets/Mirror/Tests/Editor/NetworkServerTest.cs b/Assets/Mirror/Tests/Editor/NetworkServerTest.cs index 0f6d5b90fb7..8562cbe87cd 100644 --- a/Assets/Mirror/Tests/Editor/NetworkServerTest.cs +++ b/Assets/Mirror/Tests/Editor/NetworkServerTest.cs @@ -1155,7 +1155,6 @@ public void ShutdownCleanup() Assert.That(NetworkServer.connections.Count, Is.EqualTo(0)); Assert.That(NetworkServer.connectionsCopy.Count, Is.EqualTo(0)); Assert.That(NetworkServer.handlers.Count, Is.EqualTo(0)); - Assert.That(NetworkServer.newObservers.Count, Is.EqualTo(0)); Assert.That(NetworkServer.spawned.Count, Is.EqualTo(0)); Assert.That(NetworkServer.localConnection, Is.Null);