code cleanup, visibility handling, better tests

Assets/Mirror/Components/InterestManagement/SpatialHashing/FastSpatialInterestManagement.cs

@@ -1,38 +1,59 @@
 using System;
 using System.Collections.Generic;
+using System.Runtime.CompilerServices;
 using Mirror;
 using UnityEngine;
 
-public class FastSpatialInterestManagement : InterestManagementBase {
+public class FastSpatialInterestManagement : InterestManagementBase
+{
     [Tooltip("The maximum range that objects will be visible at.")]
     public int visRange = 30;
 
-    private int TileSize => visRange / 3;
+    // we use a 9 neighbour grid.
+    // so we always see in a distance of 2 grids.
+    // for example, our own grid and then one on top / below / left / right.
+    //
+    // this means that grid resolution needs to be distance / 2.
+    // so for example, for distance = 30 we see 2 cells = 15 * 2 distance.
+    //
+    // on first sight, it seems we need distance / 3 (we see left/us/right).
+    // but that's not the case.
+    // resolution would be 10, and we only see 1 cell far, so 10+10=20.
+    int TileSize => visRange / 2;
 
     // the grid
-    private Dictionary<Vector2Int, HashSet<NetworkIdentity>> grid =
+    Dictionary<Vector2Int, HashSet<NetworkIdentity>> grid =
         new Dictionary<Vector2Int, HashSet<NetworkIdentity>>();
 
-    class Tracked {
+    class Tracked
-        public bool uninitialized;
+    {
-        public Vector2Int position;
+        public Vector2Int Position;
-        public Transform transform;
+        public Transform Transform;
-        public NetworkIdentity identity;
+        public NetworkIdentity Identity;
+        public Visibility PreviousVisibility;
+
+        public Vector2Int GridPosition(int tileSize)
+        {
+            Vector3 transformPos = Transform.position;
+            return Vector2Int.RoundToInt(new Vector2(transformPos.x, transformPos.z) / tileSize);
+        }
     }
 
-    private Dictionary<NetworkIdentity, Tracked> tracked = new Dictionary<NetworkIdentity, Tracked>();
+    Dictionary<NetworkIdentity, Tracked> trackedIdentities = new Dictionary<NetworkIdentity, Tracked>();
 
-    public override void Rebuild(NetworkIdentity identity, bool initialize) {
+    public override void Rebuild(NetworkIdentity identity, bool initialize)
-        // do nothing, we update every frame.
+    {
+        // do nothing, we rebuild globally and individually in OnSpawned
     }
 
-    public override bool OnCheckObserver(NetworkIdentity identity, NetworkConnectionToClient newObserver) {
+    public override bool OnCheckObserver(NetworkIdentity identity, NetworkConnectionToClient newObserver)
+    {
         // we build initial state during the normal loop too
         return false;
     }
 
-    // update everyone's position in the grid
+    internal void LateUpdate()
-    internal void LateUpdate() {
+    {
         // only on server
         if (!NetworkServer.active) return;
 
@@ -40,162 +61,312 @@ internal void LateUpdate() {
     }
 
     // When a new entity is spawned
-    public override void OnSpawned(NetworkIdentity identity) {
+    public override void OnSpawned(NetworkIdentity identity)
-        // (limitation: we never expect identity.visibile to change)
+    {
-        if (identity.visible != Visibility.Default) {
+        // host visibility shim to make sure unseen entities are hidden, we initialize actual visibility later
-            return;
+        if (NetworkClient.active)
-        }
+        {
-
-        // host visibility shim to make sure unseen entities are hidden
-        if (NetworkClient.active) {
             SetHostVisibility(identity, false);
         }
 
-        if (identity.connectionToClient != null) {
+        if (identity.connectionToClient != null)
+        {
             // client always sees itself
             AddObserver(identity.connectionToClient, identity);
         }
 
-        tracked.Add(identity, new Tracked {
+        Tracked tracked = new Tracked
-            uninitialized = true,
+        {
-            position = new Vector2Int(int.MaxValue, int.MaxValue), // invalid
+            Transform = identity.transform,
-            transform = identity.transform,
+            Identity = identity,
-            identity = identity,
+            PreviousVisibility = identity.visibility,
-        });
+        };
+
+        tracked.Position = tracked.GridPosition(TileSize);
+        trackedIdentities.Add(identity, tracked);
+        RebuildAdd(
+            identity,
+            InvalidPosition,
+            tracked.Position,
+            true
+        );
     }
 
+    Vector2Int InvalidPosition => new Vector2Int(int.MaxValue,
+        int.MaxValue);
+
     // when an entity is despawned/destroyed
-    public override void OnDestroyed(NetworkIdentity identity) {
+    public override void OnDestroyed(NetworkIdentity identity)
+    {
         // (limitation: we never expect identity.visibile to change)
-        if (identity.visible != Visibility.Default) {
+        if (identity.visibility != Visibility.Default)
+        {
             return;
         }
 
-        var obj = tracked[identity];
+        Tracked obj = trackedIdentities[identity];
-        tracked.Remove(identity);
+        trackedIdentities.Remove(identity);
 
-        if (!obj.uninitialized) {
+        // observers are cleaned up automatically when destroying, we just need to remove it from our grid
-            // observers are cleaned up automatically when destroying, we just need to remove it from our grid
+        grid[obj.Position].Remove(identity);
-            grid[obj.position].Remove(identity);
-        }
     }
 
-    private void RebuildAll() {
+    private void RebuildAll()
-        // loop over all entities and check if their positions changed
+    {
-        foreach (var trackedEntity in tracked.Values) {
+        // loop over all identities and check if their positions changed
-            Vector2Int pos =
+        foreach (Tracked tracked in trackedIdentities.Values)
-                Vector2Int.RoundToInt(
+        {
-                    new Vector2(trackedEntity.transform.position.x, trackedEntity.transform.position.z) / TileSize);
+            // calculate the current grid position
-            if (pos != trackedEntity.position) {
+            Vector3 transformPos = tracked.Transform.position;
-                // if the position changed, move entity about
+            Vector2Int currentPosition = Vector2Int.RoundToInt(new Vector2(transformPos.x, transformPos.z) / TileSize);
-                Vector2Int oldPos = trackedEntity.position;
+            bool visibilityChanged = tracked.Identity.visibility != tracked.PreviousVisibility;
-                trackedEntity.position = pos;
+            // Visibility change to default is done before we run the normal grid update, since
-                // First: Remove from old grid position, but only if it was ever in the grid
+            if (visibilityChanged && tracked.Identity.visibility == Visibility.Default)
-                if (!trackedEntity.uninitialized) {
+            {
-                    RebuildRemove(trackedEntity.identity, oldPos, pos);
+                if (tracked.PreviousVisibility == Visibility.ForceHidden)
+                {
+                    // Hidden To Default
+                    AddObserversHiddenToDefault(tracked.Identity, tracked.Position);
+                }
+                else
+                {
+                    // Shown To Default
+                    RemoveObserversShownToDefault(tracked.Identity, tracked.Position);
+                }
+            }
+            // if the position changed, move entity about
+            if (currentPosition != tracked.Position)
+            {
+                Vector2Int oldPosition = tracked.Position;
+                tracked.Position = currentPosition;
+                // First: Remove from old grid position, but only if it was ever in the grid to begin with
+                RebuildRemove(tracked.Identity, oldPosition, currentPosition);
+
+                // Then add to new grid tile
+                RebuildAdd(
+                    tracked.Identity,
+                    oldPosition,
+                    currentPosition,
+                    false
+                );
+            }
+
+            // after updating the grid, if the visibility has changed
+            if (visibilityChanged)
+            {
+                switch (tracked.Identity.visibility)
+                {
+                    case Visibility.Default:
+                        // handled above
+                        break;
+                    case Visibility.ForceHidden:
+                        ClearObservers(tracked.Identity);
+                        break;
+                    case Visibility.ForceShown:
+                        AddObserversAllReady(tracked.Identity);
+                        break;
+                    default:
+                        throw new ArgumentOutOfRangeException();
                 }
 
-                RebuildAdd(trackedEntity.identity, oldPos, pos, trackedEntity.uninitialized);
+                tracked.PreviousVisibility = tracked.Identity.visibility;
-                trackedEntity.uninitialized = false;
             }
         }
     }
 
-    private void RebuildRemove(NetworkIdentity entity, Vector2Int oldPosition, Vector2Int newPosition) {
+    private void RebuildRemove(NetworkIdentity changedIdentity, Vector2Int oldPosition, Vector2Int newPosition)
+    {
         // sanity check
-        if (!grid[oldPosition].Remove(entity)) {
+        if (!grid[oldPosition].Remove(changedIdentity))
+        {
             throw new InvalidOperationException("entity was not in the provided grid");
         }
 
         // for all tiles the entity could see at the old position
-        for (int x = -1; x <= 1; x++) {
+        for (int x = -1; x <= 1; x++)
-            for (int y = -1; y <= 1; y++) {
+        {
-                var tilePos = oldPosition + new Vector2Int(x, y);
+            for (int y = -1; y <= 1; y++)
+            {
+                Vector2Int tilePos = oldPosition + new Vector2Int(x, y);
                 // optimization: don't remove on overlapping tiles
                 if (Mathf.Abs(tilePos.x - newPosition.x) <= 1 &&
-                    Mathf.Abs(tilePos.y - newPosition.y) <= 1) {
+                    Mathf.Abs(tilePos.y - newPosition.y) <= 1)
+                {
                     continue;
                 }
 
-                if (!grid.TryGetValue(tilePos, out HashSet<NetworkIdentity> tile)) {
+                if (!grid.TryGetValue(tilePos, out HashSet<NetworkIdentity> tile))
+                {
                     continue;
                 }
 
                 // update observers for all identites the entity could see and all players that could see it
-                foreach (NetworkIdentity identity in tile) {
+                foreach (NetworkIdentity gridIdentity in tile)
-                    // dont touch yourself (hah.)
+                {
-                    if (identity == entity) {
+                    if (gridIdentity == changedIdentity)
+                    {
+                        // Don't do anything with yourself
                         continue;
                     }
 
-                    // if the identity is a player, remove the entity from it
+                    // we only modify observers here if the visibility is default, ForceShown/ForceHidden are handled differently
-                    if (identity.connectionToClient != null) {
+
-                        RemoveObserver(identity.connectionToClient, entity);
+                    // if the gridIdentity is a player, it can't see changedIdentity anymore
+                    if (gridIdentity.connectionToClient != null && changedIdentity.visibility == Visibility.Default)
+                    {
+                        RemoveObserver(gridIdentity.connectionToClient, changedIdentity);
                     }
 
-                    // if the entity is a player, remove the identity from it
+                    // if the changedIdentity is a player, it can't see gridIdentity anymore
-                    if (entity.connectionToClient != null) {
+                    if (changedIdentity.connectionToClient != null && gridIdentity.visibility == Visibility.Default)
-                        RemoveObserver(entity.connectionToClient, identity);
+                    {
+                        RemoveObserver(changedIdentity.connectionToClient, gridIdentity);
                     }
                 }
             }
         }
     }
 
-    private void RebuildAdd(NetworkIdentity entity, Vector2Int oldPos, Vector2Int newPos, bool initialize) {
+    private void RebuildAdd(NetworkIdentity changedIdentity, Vector2Int oldPosition, Vector2Int newPosition,
+        bool initialize)
+    {
         // for all tiles the entity now sees at the new position
-        for (int x = -1; x <= 1; x++) {
+        for (int x = -1; x <= 1; x++)
-            for (int y = -1; y <= 1; y++) {
+        {
-                var tilePos = newPos + new Vector2Int(x, y);
+            for (int y = -1; y <= 1; y++)
-                // optimization: don't add on overlapping tiles
+            {
-                if (!initialize && (Mathf.Abs(tilePos.x - oldPos.x) <= 1 &&
+                Vector2Int tilePos = newPosition + new Vector2Int(x, y);
-                                    Mathf.Abs(tilePos.y - oldPos.y) <= 1)) {
+
+                // Skip grid tiles that were already visible before moving
+                if (!initialize && (Mathf.Abs(tilePos.x - oldPosition.x) <= 1 &&
+                                    Mathf.Abs(tilePos.y - oldPosition.y) <= 1))
+                {
                     continue;
                 }
 
-                if (!grid.TryGetValue(tilePos, out var tile)) {
+                if (!grid.TryGetValue(tilePos, out HashSet<NetworkIdentity> tile))
+                {
                     continue;
                 }
 
-                foreach (var identity in tile) {
+                foreach (NetworkIdentity gridIdentity in tile)
-                    // dont touch yourself (hah.)
+                {
-                    if (identity == entity) {
+                    if (gridIdentity == changedIdentity)
+                    {
+                        // Don't do anything with yourself
                         continue;
                     }
 
-                    // if the identity is a player, add the entity to it
+                    // we only modify observers here if the visibility is default, ForceShown/ForceHidden are handled differently
-                    if (identity.connectionToClient != null) {
+                    // if the gridIdentity is a player, it can now see changedIdentity
-                        try {
+                    if (gridIdentity.connectionToClient != null && changedIdentity.visibility == Visibility.Default)
-                            AddObserver(identity.connectionToClient, entity);
+                    {
-                        } catch (ArgumentException e) {
+                        AddObserver(gridIdentity.connectionToClient, changedIdentity);
-                            // sanity check
-                            Debug.LogError(
-                                $"Failed to add {entity} (#{entity.netId}) to the observers of {identity} (#{identity.netId}) (case 1)\n{e}");
-                        }
                     }
 
-                    // if the entity is a player, add the identity to it
+                    // if the changedIdentity is a player, it can now see gridIdentity
-                    if (entity.connectionToClient != null) {
+                    if (changedIdentity.connectionToClient != null && gridIdentity.visibility == Visibility.Default)
-                        try {
+                    {
-                            AddObserver(entity.connectionToClient, identity);
+                        AddObserver(changedIdentity.connectionToClient, gridIdentity);
-                        } catch (ArgumentException e) {
-                            // sanity check
-                            Debug.LogError(
-                                $"Failed to add {identity} (#{identity.netId}) to the observers of {entity} (#{entity.netId}) (case 2)\n{e}");
-                        }
                     }
                 }
             }
         }
 
         // add ourselves to the new grid position
-        if (!grid.TryGetValue(newPos, out HashSet<NetworkIdentity> addTile)) {
+        if (!grid.TryGetValue(newPosition, out HashSet<NetworkIdentity> addTile))
+        {
             addTile = new HashSet<NetworkIdentity>();
-            grid[newPos] = addTile;
+            grid[newPosition] = addTile;
         }
 
-        if (!addTile.Add(entity)) {
+        if (!addTile.Add(changedIdentity))
+        {
             throw new InvalidOperationException("entity was already in the grid");
         }
     }
+
+    /// Adds observers to the NI, but not the other way around. This is used when a NI changes from ForceHidden to Default
+    private void AddObserversHiddenToDefault(NetworkIdentity changed, Vector2Int gridPosition)
+    {
+        // for all tiles the entity now sees at the new position
+        for (int x = -1; x <= 1; x++)
+        {
+            for (int y = -1; y <= 1; y++)
+            {
+                Vector2Int tilePos = gridPosition + new Vector2Int(x, y);
+                if (!grid.TryGetValue(tilePos, out HashSet<NetworkIdentity> tile))
+                {
+                    continue;
+                }
+
+                foreach (NetworkIdentity gridIdentity in tile)
+                {
+                    if (gridIdentity == changed)
+                    {
+                        // Don't do anything with yourself
+                        continue;
+                    }
+
+                    // if the gridIdentity is a player, it can now see changedIdentity
+                    if (gridIdentity.connectionToClient != null)
+                    {
+                        AddObserver(gridIdentity.connectionToClient, changed);
+                    }
+                }
+            }
+        }
+    }
+
+    // Temp hashset to avoid runtime allocation
+    private HashSet<NetworkConnectionToClient> tempShownToDefaultSet = new HashSet<NetworkConnectionToClient>();
+
+    /// Removes observers from the NI, but doesn't change observing. This is used when a NI changes from ForceShown to Default
+    private void RemoveObserversShownToDefault(NetworkIdentity changedIdentity, Vector2Int gridPosition)
+    {
+        tempShownToDefaultSet.Clear();
+        // copy over all current connections that are seeing the NI
+        foreach (NetworkConnectionToClient observer in changedIdentity.observers.Values)
+        {
+            tempShownToDefaultSet.Add(observer);
+        }
+
+        // for all tiles the entity now sees at the current position
+        // remove any connections that can still see the changedIdentity
+        for (int x = -1; x <= 1; x++)
+        {
+            for (int y = -1; y <= 1; y++)
+            {
+                Vector2Int tilePos = gridPosition + new Vector2Int(x, y);
+                if (!grid.TryGetValue(tilePos, out HashSet<NetworkIdentity> tile))
+                {
+                    continue;
+                }
+
+                foreach (NetworkIdentity gridIdentity in tile)
+                {
+                    if (gridIdentity == changedIdentity)
+                    {
+                        // Don't do anything with yourself
+                        continue;
+                    }
+
+                    // if the gridIdentity is a player, it can see changedIdentity
+                    if (gridIdentity.connectionToClient != null)
+                    {
+                        tempShownToDefaultSet.Remove(gridIdentity.connectionToClient);
+                    }
+                }
+            }
+        }
+
+        // any left over connections can't see changedIdentity - thus need removing
+        foreach (NetworkConnectionToClient connection in tempShownToDefaultSet)
+        {
+            RemoveObserver(connection, changedIdentity);
+        }
+
+        // clear when done
+        tempShownToDefaultSet.Clear();
+    }
 }

Assets/Mirror/Core/InterestManagementBase.cs

@@ -1,6 +1,7 @@
 // interest management component for custom solutions like
 // distance based, spatial hashing, raycast based, etc.
 // low level base class allows for low level spatial hashing etc., which is 3-5x faster.
+
 using UnityEngine;
 
 namespace Mirror
@@ -73,5 +74,39 @@ protected void RemoveObserver(NetworkConnectionToClient connection, NetworkIdent
             connection.RemoveFromObserving(identity, false);
             identity.observers.Remove(connection.connectionId);
         }
+
+        /// For ForceShown: Makes sure all ready connections (that aren't already) are added to observers
+        protected void AddObserversAllReady(NetworkIdentity identity)
+        {
+            foreach (NetworkConnectionToClient connection in identity.observers.Values)
+            {
+                if (connection.isReady && !identity.observers.ContainsKey(connection.connectionId))
+                {
+                    connection.AddToObserving(identity);
+                    identity.observers.Add(connection.connectionId, connection);
+                }
+            }
+        }
+
+        /// Removes all observers from this identity
+        protected void ClearObservers(NetworkIdentity identity)
+        {
+            foreach (NetworkConnectionToClient connection in identity.observers.Values)
+            {
+                // Don't remove the client from observing its owned objects
+                if (connection != identity.connectionToClient)
+                {
+                    connection.RemoveFromObserving(identity, false);
+                }
+            }
+
+            // Clear..
+            identity.observers.Clear();
+            // If the object is owned by a client, add it's connection back to the observing set
+            if (identity.connectionToClient != null)
+            {
+                identity.observers.Add(identity.connectionToClient.connectionId, identity.connectionToClient);
+            }
+        }
     }
 }

Assets/Mirror/Tests/Editor/InterestManagement/InterestManagementTests_FastSpatialHashing.cs

@@ -1,135 +1,172 @@
 // default = no component = everyone sees everyone
 
-using System.Collections.Generic;
+using System;
 using NUnit.Framework;
 using UnityEngine;
 
 namespace Mirror.Tests.InterestManagement
 {
-    public class InterestManagementTests_FastSpatialHashing : InterestManagementTests_Common
+    public class InterestManagementTests_FastSpatialHashing : MirrorEditModeTest
     {
         FastSpatialInterestManagement aoi;
 
+        protected NetworkIdentity CreateNI(Action<NetworkIdentity> prespawn = null)
+        {
+            CreateNetworked(out var gameObject, out var identity);
+            prespawn?.Invoke(identity);
+            NetworkServer.Spawn(gameObject);
+            return identity;
+        }
+
+        protected NetworkIdentity CreatePlayerNI(int connectionId, Action<NetworkIdentity> prespawn = null)
+        {
+            CreateNetworked(out var gameObject, out var identity);
+            prespawn?.Invoke(identity);
+            NetworkConnectionToClient connection = new NetworkConnectionToClient(connectionId);
+            connection.isAuthenticated = true;
+            connection.isReady = true;
+            connection.identity = identity;
+            NetworkServer.connections[connection.connectionId] = connection;
+            NetworkServer.Spawn(gameObject, connection);
+            return identity;
+        }
+
         [SetUp]
         public override void SetUp()
         {
-
+            base.SetUp();
-            // TODO: these are just copied from the base Setup methods since the aoi expects "normal" operation
-            // for example: OnSpawned to be called for spawning identities, late adding the aoi does not work currently
-            // the setup also adds each identity to spawned twice so that also causes some issues during teardown
-            instantiated = new List<GameObject>();
-
-            // need a holder GO. with name for easier debugging.
-            holder = new GameObject("MirrorTest.holder");
-
-            // need a transport to send & receive
-            Transport.active = transport = holder.AddComponent<MemoryTransport>();
-
-            // A with connectionId = 0x0A, netId = 0xAA
-            CreateNetworked(out gameObjectA, out identityA);
-            connectionA = new NetworkConnectionToClient(0x0A);
-            connectionA.isAuthenticated = true;
-            connectionA.isReady = true;
-            connectionA.identity = identityA;
-            //NetworkServer.spawned[0xAA] = identityA; // TODO: this causes two the identities to end up in spawned twice
-
-            // B
-            CreateNetworked(out gameObjectB, out identityB);
-            connectionB = new NetworkConnectionToClient(0x0B);
-            connectionB.isAuthenticated = true;
-            connectionB.isReady = true;
-            connectionB.identity = identityB;
-            //NetworkServer.spawned[0xBB] = identityB; // TODO: this causes two the identities to end up in spawned twice
-
             // need to start server so that interest management works
             NetworkServer.Listen(10);
 
-            // add both connections
-            NetworkServer.connections[connectionA.connectionId] = connectionA;
-            NetworkServer.connections[connectionB.connectionId] = connectionB;
-
             aoi = holder.AddComponent<FastSpatialInterestManagement>();
             aoi.visRange = 10;
             // setup server aoi since InterestManagement Awake isn't called
             NetworkServer.aoi = aoi;
-
-            // spawn both so that .observers is created
-            NetworkServer.Spawn(gameObjectA, connectionA);
-            NetworkServer.Spawn(gameObjectB, connectionB);
         }
 
         [TearDown]
         public override void TearDown()
         {
+            foreach (GameObject go in instantiated)
+            {
+                if (go.TryGetComponent(out NetworkIdentity ni))
+                {
+                    // set isServer is false. otherwise Destroy instead of
+                    // DestroyImmediate is called internally, giving an error in Editor
+                    ni.isServer = false;
+                }
+            }
+
+            // clear connections first. calling OnDisconnect wouldn't work since
+            // we have no real clients.
+            NetworkServer.connections.Clear();
+
             base.TearDown();
             // clear server aoi again
             NetworkServer.aoi = null;
         }
 
-        public override void ForceHidden_Initial()
+        private void AssertSelfVisible(NetworkIdentity id)
         {
-            // doesnt support changing visibility at runtime
+            // identities ALWAYS see themselves, if they have a player
+            if (id.connectionToClient != null)
+            {
+                Assert.That(id.observers.ContainsKey(id.connectionToClient.connectionId), Is.True);
+            }
         }
 
-        public override void ForceShown_Initial()
-        {
-            // doesnt support changing visibility at runtime
-        }
-
-        // brute force interest management
-        // => everyone should see everyone if in range
         [Test]
-        public void InRange_Initial()
+        public void ForceHidden()
         {
             // A and B are at (0,0,0) so within range!
+            var a = CreatePlayerNI(1, ni => ni.visibility = Visibility.ForceHidden);
+            var b = CreatePlayerNI(2);
 
+            // no rebuild required here due to initial state :)
+
+            AssertSelfVisible(a);
+            AssertSelfVisible(b);
+            // A should not be seen by B because A is force hidden
+            Assert.That(a.observers.ContainsKey(b.connectionToClient.connectionId), Is.False);
+            // B should be seen by A
+            Assert.That(b.observers.ContainsKey(b.connectionToClient.connectionId), Is.True);
+
+            // If we now set a to default, and rebuild, they should both see each other!
+            a.visibility = Visibility.Default;
             aoi.LateUpdate();
-            // both should see each other because they are in range
+            AssertSelfVisible(a);
-            Assert.That(identityA.observers.ContainsKey(connectionB.connectionId), Is.True);
+            AssertSelfVisible(b);
-            Assert.That(identityB.observers.ContainsKey(connectionA.connectionId), Is.True);
+            Assert.That(a.observers.ContainsKey(b.connectionToClient.connectionId), Is.True);
+            Assert.That(b.observers.ContainsKey(b.connectionToClient.connectionId), Is.True);
+            // If we now set both hidden, and rebuild, they both won't see each other!
+            a.visibility = Visibility.ForceHidden;
+            b.visibility = Visibility.ForceHidden;
+            aoi.LateUpdate();
+            AssertSelfVisible(a);
+            AssertSelfVisible(b);
+            Assert.That(a.observers.ContainsKey(b.connectionToClient.connectionId), Is.False);
+            Assert.That(b.observers.ContainsKey(a.connectionToClient.connectionId), Is.False);
         }
 
-        // brute force interest management
-        // => everyone should see everyone if in range
         [Test]
-        public void InRange_NotInitial()
+        public void ForceShown()
+        {
+        }
+
+        [Test]
+        public void InRangeInitial_To_OutRange()
         {
             // A and B are at (0,0,0) so within range!
-
+            var a = CreatePlayerNI(1);
-            aoi.LateUpdate();
+            var b = CreatePlayerNI(2);
+            AssertSelfVisible(a);
+            AssertSelfVisible(b);
             // both should see each other because they are in range
-            Assert.That(identityA.observers.ContainsKey(connectionB.connectionId), Is.True);
+            Assert.That(a.observers.ContainsKey(b.connectionToClient.connectionId), Is.True);
-            Assert.That(identityB.observers.ContainsKey(connectionA.connectionId), Is.True);
+            Assert.That(b.observers.ContainsKey(a.connectionToClient.connectionId), Is.True);
-        }
+            // update won't change that
-
-        // brute force interest management
-        // => everyone should see everyone if in range
-        [Test]
-        public void OutOfRange_Initial()
-        {
-            // A and B are too far from each other
-            identityB.transform.position = Vector3.right * (aoi.visRange + 1);
-
             aoi.LateUpdate();
-            // both should not see each other
+            AssertSelfVisible(a);
-            Assert.That(identityA.observers.ContainsKey(connectionB.connectionId), Is.False);
+            AssertSelfVisible(b);
-            Assert.That(identityB.observers.ContainsKey(connectionA.connectionId), Is.False);
+            Assert.That(a.observers.ContainsKey(b.connectionToClient.connectionId), Is.True);
-        }
+            Assert.That(b.observers.ContainsKey(a.connectionToClient.connectionId), Is.True);
-
+            // move out of range
-        // brute force interest management
+            a.transform.position = new Vector3(aoi.visRange * 100, 0, 0);
-        // => everyone should see everyone if in range
-        [Test]
-        public void OutOfRange_NotInitial()
-        {
-            // A and B are too far from each other
-            identityB.transform.position = Vector3.right * (aoi.visRange + 1);
-
             aoi.LateUpdate();
-            // both should not see each other
+            AssertSelfVisible(a);
-            Assert.That(identityA.observers.ContainsKey(connectionB.connectionId), Is.False);
+            AssertSelfVisible(b);
-            Assert.That(identityB.observers.ContainsKey(connectionA.connectionId), Is.False);
+            // and they'll see not each other anymore
+            Assert.That(a.observers.ContainsKey(b.connectionToClient.connectionId), Is.False);
+            Assert.That(b.observers.ContainsKey(a.connectionToClient.connectionId), Is.False);
         }
 
-        // TODO add tests to make sure old observers are removed etc.
+        [Test]
+        public void OutRangeInitial_To_InRange()
+        {
+            // A and B are not in range
+            var a = CreatePlayerNI(1,
+                ni => ni.transform.position = new Vector3(aoi.visRange * 100, 0, 0));
+            var b = CreatePlayerNI(2);
+
+            AssertSelfVisible(a);
+            AssertSelfVisible(b);
+            // both should not see each other because they aren't in range
+            Assert.That(a.observers.ContainsKey(b.connectionToClient.connectionId), Is.False);
+            Assert.That(b.observers.ContainsKey(a.connectionToClient.connectionId), Is.False);
+            aoi.LateUpdate();
+            AssertSelfVisible(a);
+            AssertSelfVisible(b);
+            // update won't change that
+            Assert.That(a.observers.ContainsKey(b.connectionToClient.connectionId), Is.False);
+            Assert.That(b.observers.ContainsKey(a.connectionToClient.connectionId), Is.False);
+            // move into range
+            a.transform.position = Vector3.zero;
+            aoi.LateUpdate();
+            AssertSelfVisible(a);
+            AssertSelfVisible(b);
+            // and they'll see each other
+            Assert.That(a.observers.ContainsKey(b.connectionToClient.connectionId), Is.True);
+            Assert.That(b.observers.ContainsKey(a.connectionToClient.connectionId), Is.True);
+        }
     }
 }