refactor!: adding INetworkVisibility

network identity now creates a default visibility class instead of checking null each time.
This will simplify some of the checks because Identity.Visibility will never return null

BREAKING CHANGE: NetworkIdentity.Visibility can now throw if called before Object is spawned

Assets/Mirage/Runtime/INetworkVisibility.cs

@@ -0,0 +1,49 @@
+using System.Collections.Generic;
+
+namespace Mirage
+{
+    public interface INetworkVisibility
+    {
+        bool OnCheckObserver(INetworkPlayer player);
+        void OnRebuildObservers(HashSet<INetworkPlayer> newObservers, bool initialize);
+    }
+
+    /// <summary>
+    /// Default visible when no NetworkVisibility is added to the gameobject
+    /// </summary>
+    internal class AlwaysVisible : INetworkVisibility
+    {
+        private readonly ServerObjectManager _objectManager;
+        private readonly NetworkServer _server;
+
+        public AlwaysVisible(ServerObjectManager serverObjectManager)
+        {
+            _objectManager = serverObjectManager;
+            _server = serverObjectManager.Server;
+        }
+
+        public bool OnCheckObserver(INetworkPlayer player) => true;
+
+        public void OnRebuildObservers(HashSet<INetworkPlayer> newObservers, bool initialize)
+        {
+            // add all server connections
+            foreach (var player in _server.Players)
+            {
+                if (!player.SceneIsReady)
+                    continue;
+
+                // todo replace this with a better visibility system (where default checks auth/scene ready)
+                if (_objectManager.OnlySpawnOnAuthenticated && !player.IsAuthenticated)
+                    continue;
+
+                newObservers.Add(player);
+            }
+
+            // add local host connection (if any)
+            if (_server.LocalPlayer != null && _server.LocalPlayer.SceneIsReady)
+            {
+                newObservers.Add(_server.LocalPlayer);
+            }
+        }
+    }
+}

Assets/Mirage/Runtime/NetworkIdentity.cs

@@ -315,15 +315,33 @@ private NetworkBehaviour[] FindBehaviourForThisIdentity()
             return components;
         }
 
-        private NetworkVisibility _visibility;
-        public NetworkVisibility Visibility
+        private INetworkVisibility _visibility;
+        /// <summary>
+        /// Returns the NetworkVisibility behaviour on this gameObject, or a default visibility where all objects are visible.
+        /// <para>Note: NetworkVisibility must be on same gameObject has NetworkIdentity, not on a child object</para>
+        /// </summary>
+        public INetworkVisibility Visibility
         {
             get
             {
                 if (_visibility is null)
                 {
-                    _visibility = GetComponent<NetworkVisibility>();
+                    // try get behaviour, otherwise just set default class
+                    if (TryGetComponent<NetworkVisibility>(out var visibilityBehaviour))
+                        _visibility = visibilityBehaviour;
+                    else
+                    {
+                        if (ServerObjectManager == null)
+                            throw new InvalidOperationException("Can't get default Visibility before object is spawned");
+
+                        var defaultVisibility = ServerObjectManager.DefaultVisibility;
+                        if (defaultVisibility is null)
+                            throw new InvalidOperationException("DefaultVisibility was null on ObjectManager, make sure ServerObjectManager has referecne to server, and server has started");
+
+                        _visibility = defaultVisibility;
+                    }
                 }
+
                 return _visibility;
             }
         }
@@ -580,25 +598,11 @@ internal void CallStopAuthority()
         }
 
         /// <summary>
-        /// check if observer can be seen by connection.
-        /// <list type="bullet">
-        ///     <item><description>
-        ///         returns visibility.OnCheckObserver
-        ///     </description></item>
-        ///     <item><description>
-        ///         returns true if we have no NetworkVisibility, default objects are visible
-        ///     </description></item>
-        /// </list>
-        /// </summary>
-        /// <param name="player"></param>
+        /// Check if observer can be seen by player.
         /// <returns></returns>
         internal bool OnCheckObserver(INetworkPlayer player)
         {
-            if (Visibility != null)
-            {
-                return Visibility.OnCheckObserver(player);
-            }
-            return true;
+            return Visibility.OnCheckObserver(player);
         }
 
         internal void StopClient()
@@ -926,54 +930,15 @@ internal void AddObserver(INetworkPlayer player)
         }
 
         /// <summary>
-        /// Helper function to call OnRebuildObservers in all components
-        /// <para>HashSet is passed in so we can cache it!</para>
-        /// <para>Returns true if we have a NetworkVisibility, false otherwise</para>
-        /// <para>Initialize is true on first rebuild, false on consecutive rebuilds</para>
+        /// Helper function to call OnRebuildObservers on <see cref="Visibility"/> using <see cref="NetworkServer.Players"/> 
         /// </summary>
-        /// <param name="observersSet"></param>
-        /// <param name="initialize"></param>
-        /// <returns></returns>
-        internal bool GetNewObservers(HashSet<INetworkPlayer> observersSet, bool initialize)
+        /// <param name="observersSet">set to clear and fill with new observers</param>
+        /// <param name="initialize">If Object is being first spawned or refreshed later on</param>
+        internal void GetNewObservers(HashSet<INetworkPlayer> observersSet, bool initialize)
         {
             observersSet.Clear();
 
-            if (Visibility != null)
-            {
-                Visibility.OnRebuildObservers(observersSet, initialize);
-                return true;
-            }
-
-            // we have no NetworkVisibility. return false to indicate that we
-            // should use the default implementation.
-            return false;
-        }
-
-        /// <summary>
-        /// Helper function to add all server connections as observers.
-        /// This is used if none of the components provides their own
-        /// OnRebuildObservers function.
-        /// </summary>
-        internal void AddAllReadyServerConnectionsToObservers()
-        {
-            // add all server connections
-            foreach (var player in Server.Players)
-            {
-                if (!player.SceneIsReady)
-                    continue;
-
-                // todo replace this with a better visibility system (where default checks auth/scene ready)
-                if (ServerObjectManager.OnlySpawnOnAuthenticated && !player.IsAuthenticated)
-                    continue;
-
-                AddObserver(player);
-            }
-
-            // add local host connection (if any)
-            if (Server.LocalPlayer != null && Server.LocalPlayer.SceneIsReady)
-            {
-                AddObserver(Server.LocalPlayer);
-            }
+            Visibility.OnRebuildObservers(observersSet, initialize);
         }
 
         private static readonly HashSet<INetworkPlayer> newObservers = new HashSet<INetworkPlayer>();
@@ -988,29 +953,14 @@ public void RebuildObservers(bool initialize)
             var changed = false;
 
             // call OnRebuildObservers function
-            var rebuildOverwritten = GetNewObservers(newObservers, initialize);
+            GetNewObservers(newObservers, initialize);
 
-            // if player connection: ensure player always see himself no matter what.
-            // -> 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.
+            // ensure player always sees objects they own
             if (Owner != null && Owner.SceneIsReady)
             {
                 newObservers.Add(Owner);
             }
 
-            // if no NetworkVisibility component, then add all server connections.
-            if (!rebuildOverwritten)
-            {
-                // only add all connections when rebuilding the first time.
-                // second time we just keep them without rebuilding anything.
-                if (initialize)
-                {
-                    AddAllReadyServerConnectionsToObservers();
-                }
-                return;
-            }
-
             changed = AddNewObservers(initialize, changed);
 
             changed = RemoveOldObservers(changed);

Assets/Mirage/Runtime/NetworkVisibility.cs

@@ -3,14 +3,11 @@
 
 namespace Mirage
 {
-    // the name NetworkProximityCheck implies that it's only about objects in
-    // proximity to the player. But we might have room based, guild based,
-    // instanced based checks too, so NetworkVisibility is more fitting.
-    //
-    // note: we inherit from NetworkBehaviour so we can reuse .Identity, etc.
-    // note: unlike UNET, we only allow 1 proximity checker per NetworkIdentity.
+    /// <summary>
+    /// NetworkBehaviour that calculates if the gameObject should be visible to different players or not
+    /// </summary>
     [DisallowMultipleComponent]
-    public abstract class NetworkVisibility : NetworkBehaviour
+    public abstract class NetworkVisibility : NetworkBehaviour, INetworkVisibility
     {
         public delegate void VisibilityChanged(INetworkPlayer player, bool visible);
 

Assets/Mirage/Runtime/ServerObjectManager.cs

@@ -62,6 +62,8 @@ public class ServerObjectManager : MonoBehaviour
 
         private uint GetNextNetworkId() => NetIdGenerator?.GenerateNetId() ?? checked(_nextNetworkId++);
 
+        public INetworkVisibility DefaultVisibility { get; private set; }
+
         public void Start()
         {
             if (Server != null)
@@ -86,6 +88,7 @@ internal void RegisterMessageHandlers()
 
         private void OnServerStarted()
         {
+            DefaultVisibility = new AlwaysVisible(this);
             RegisterMessageHandlers();
             SpawnOrActivate();
         }
@@ -350,8 +353,8 @@ private void Respawn(NetworkIdentity identity)
         internal void ShowToPlayer(NetworkIdentity identity, INetworkPlayer player)
         {
             var visiblity = identity.Visibility;
-            if (visiblity != null)
-                visiblity.InvokeVisibilityChanged(player, true);
+            if (visiblity is NetworkVisibility networkVisibility)
+                networkVisibility.InvokeVisibilityChanged(player, true);
 
             // dont send if loading scene
             if (player.SceneIsReady)
@@ -361,8 +364,8 @@ internal void ShowToPlayer(NetworkIdentity identity, INetworkPlayer player)
         internal void HideToPlayer(NetworkIdentity identity, INetworkPlayer player)
         {
             var visiblity = identity.Visibility;
-            if (visiblity != null)
-                visiblity.InvokeVisibilityChanged(player, false);
+            if (visiblity is NetworkVisibility networkVisibility)
+                networkVisibility.InvokeVisibilityChanged(player, false);
 
             player.Send(new ObjectHideMessage { netId = identity.NetId });
         }
@@ -570,8 +573,8 @@ public void Spawn(NetworkIdentity identity, INetworkPlayer owner)
 
         internal void SendSpawnMessage(NetworkIdentity identity, INetworkPlayer player)
         {
-            logger.Assert(!OnlySpawnOnAuthenticated || player.IsAuthenticated || identity.Visibility != null,
-                "SendSpawnMessage should only be called if OnlySpanwOnAuthenticated is false, player is authenticated, or there is custom visibility");
+            logger.Assert(!OnlySpawnOnAuthenticated || player.IsAuthenticated || identity.Visibility != DefaultVisibility,
+                "SendSpawnMessage should only be called if OnlySpawnOnAuthenticated is false, player is authenticated, or there is custom visibility");
             if (logger.LogEnabled()) logger.Log($"Server SendSpawnMessage: name={identity.name} sceneId={identity.SceneId:X} netId={identity.NetId}");
 
             // one writer for owner, one for observers

Assets/Tests/Editor/NetworkIdentityCallbackTests.cs

@@ -615,32 +615,28 @@ public void GetNewObservers()
 
             // get new observers
             var observers = new HashSet<INetworkPlayer>();
-            var result = identity.GetNewObservers(observers, true);
-            Assert.That(result, Is.True);
+            identity.GetNewObservers(observers, true);
             Assert.That(observers.Count, Is.EqualTo(1));
             Assert.That(observers.Contains(comp.observer), Is.True);
         }
 
         [Test]
         public void GetNewObserversClearsHashSet()
         {
+            var sub1 = Substitute.For<INetworkPlayer>();
+            var sub2 = Substitute.For<INetworkPlayer>();
+
             // get new observers. no observer components so it should just clear
             // it and not do anything else
             var observers = new HashSet<INetworkPlayer>
             {
-                player1
+                // add values that GetNewObservers wont add itself
+               sub1, sub2
             };
-            identity.GetNewObservers(observers, true);
-            Assert.That(observers.Count, Is.EqualTo(0));
-        }
 
-        [Test]
-        public void GetNewObserversFalseIfNoComponents()
-        {
-            // get new observers. no observer components so it should be false
-            var observers = new HashSet<INetworkPlayer>();
-            var result = identity.GetNewObservers(observers, true);
-            Assert.That(result, Is.False);
+            identity.GetNewObservers(observers, true);
+            Assert.That(observers, Does.Not.Contains(sub1));
+            Assert.That(observers, Does.Not.Contains(sub2));
         }
 
         // RebuildObservers should always add the own ready connection

Assets/Tests/Runtime/Host/NetworkIdentityTests.cs

@@ -216,12 +216,13 @@ public class NetworkIdentityStartedTests : HostSetup<MockComponent>
     {
         private NetworkIdentity testIdentity;
 
-        public override void ExtraSetup()
+        public override UniTask LateSetup()
         {
             testIdentity = CreateNetworkIdentity();
             server.Started.AddListener(() => serverObjectManager.Spawn(testIdentity));
-        }
 
+            return UniTask.CompletedTask;
+        }
 
         [UnityTest]
         public IEnumerator ClientNotNullAfterSpawnInStarted() => UniTask.ToCoroutine(async () =>

Assets/Tests/Runtime/Host/ServerObjectManagerTest.cs

@@ -17,7 +17,9 @@ public void HideForPlayerTest()
 
             var player = Substitute.For<INetworkPlayer>();
 
-            NetworkIdentity identity = CreateNetworkIdentity();
+            var identity = CreateNetworkIdentity();
+            // objectManager must be set in order for Visibility too be called
+            identity.ServerObjectManager = serverObjectManager;
 
             serverObjectManager.HideToPlayer(identity, player);
 
@@ -56,7 +58,7 @@ public void UnSpawn()
         [UnityTest]
         public IEnumerator DestroyAllSpawnedOnStopTest() => UniTask.ToCoroutine(async () =>
         {
-            NetworkIdentity spawnTestObj = CreateNetworkIdentity();
+            var spawnTestObj = CreateNetworkIdentity();
             serverObjectManager.Spawn(spawnTestObj);
 
             // need to grab reference to world before Stop, becuase stop will clear reference