pull -> push broadcasting to prepare for dirtyIdentities

Assets/Mirror/Core/NetworkIdentity.cs

@@ -23,14 +23,6 @@ namespace Mirror
     //              to everyone etc.
     public enum Visibility { Default, ForceHidden, ForceShown }
 
-    public struct NetworkIdentitySerialization
-    {
-        // IMPORTANT: int tick avoids floating point inaccuracy over days/weeks
-        public int tick;
-        public NetworkWriter ownerWriter;
-        public NetworkWriter observersWriter;
-    }
-
     /// <summary>NetworkIdentity identifies objects across the network.</summary>
     [DisallowMultipleComponent]
     // NetworkIdentity.Awake initializes all NetworkComponents.
@@ -237,19 +229,6 @@ public static Dictionary<uint, NetworkIdentity> spawned
         [Tooltip("Visibility can overwrite interest management. ForceHidden can be useful to hide monsters while they respawn. ForceShown can be useful for score NetworkIdentities that should always broadcast to everyone in the world.")]
         public Visibility visible = Visibility.Default;
 
-        // broadcasting serializes all entities around a player for each player.
-        // we don't want to serialize one entity twice in the same tick.
-        // so we cache the last serialization and remember the timestamp so we
-        // know which Update it was serialized.
-        // (timestamp is the same while inside Update)
-        // => this way we don't need to pool thousands of writers either.
-        // => way easier to store them per object
-        NetworkIdentitySerialization lastSerialization = new NetworkIdentitySerialization
-        {
-            ownerWriter = new NetworkWriter(),
-            observersWriter = new NetworkWriter()
-        };
-
         // Keep track of all sceneIds to detect scene duplicates
         static readonly Dictionary<ulong, NetworkIdentity> sceneIds =
             new Dictionary<ulong, NetworkIdentity>();
@@ -1107,41 +1086,6 @@ internal void DeserializeClient(NetworkReader reader, bool initialState)
             }
         }
 
-        // get cached serialization for this tick (or serialize if none yet).
-        // IMPORTANT: int tick avoids floating point inaccuracy over days/weeks.
-        // calls SerializeServer, so this function is to be called on server.
-        internal NetworkIdentitySerialization GetServerSerializationAtTick(int tick)
-        {
-            // only rebuild serialization once per tick. reuse otherwise.
-            // except for tests, where Time.frameCount never increases.
-            // so during tests, we always rebuild.
-            // (otherwise [SyncVar] changes would never be serialized in tests)
-            //
-            // NOTE: != instead of < because int.max+1 overflows at some point.
-            if (lastSerialization.tick != tick
-#if UNITY_EDITOR
-                || !Application.isPlaying
-#endif
-               )
-            {
-                // reset
-                lastSerialization.ownerWriter.Position = 0;
-                lastSerialization.observersWriter.Position = 0;
-
-                // serialize
-                SerializeServer(false,
-                                lastSerialization.ownerWriter,
-                                lastSerialization.observersWriter);
-
-                // set tick
-                lastSerialization.tick = tick;
-                //Debug.Log($"{name} (netId={netId}) serialized for tick={tickTimeStamp}");
-            }
-
-            // return it
-            return lastSerialization;
-        }
-
         // Helper function to handle Command/Rpc
         internal void HandleRemoteCall(byte componentIndex, ushort functionHash, RemoteCallType remoteCallType, NetworkReader reader, NetworkConnectionToClient senderConnection = null)
         {

Assets/Mirror/Core/NetworkServer.cs

@@ -1681,68 +1681,28 @@ public static void RebuildObservers(NetworkIdentity identity, bool initialize)
 
         // broadcasting ////////////////////////////////////////////////////////
         // helper function to get the right serialization for a connection
-        static NetworkWriter SerializeForConnection(NetworkIdentity identity, NetworkConnectionToClient connection)
+        static NetworkWriter SerializeForConnection(NetworkConnectionToClient connection, bool owned, NetworkWriter ownerWriter, NetworkWriter observersWriter)
         {
-            // get serialization for this entity (cached)
-            // IMPORTANT: int tick avoids floating point inaccuracy over days/weeks
-            NetworkIdentitySerialization serialization = identity.GetServerSerializationAtTick(Time.frameCount);
-
-            // is this entity owned by this connection?
-            bool owned = identity.connectionToClient == connection;
-
             // send serialized data
             // owner writer if owned
             if (owned)
             {
                 // was it dirty / did we actually serialize anything?
-                if (serialization.ownerWriter.Position > 0)
+                if (ownerWriter.Position > 0)
-                    return serialization.ownerWriter;
+                    return ownerWriter;
             }
             // observers writer if not owned
             else
             {
                 // was it dirty / did we actually serialize anything?
-                if (serialization.observersWriter.Position > 0)
+                if (observersWriter.Position > 0)
-                    return serialization.observersWriter;
+                    return observersWriter;
             }
 
             // nothing was serialized
             return null;
         }
 
-        // helper function to broadcast the world to a connection
-        static void BroadcastToConnection(NetworkConnectionToClient connection)
-        {
-            // for each entity that this connection is seeing
-            foreach (NetworkIdentity identity in connection.observing)
-            {
-                // make sure it's not null or destroyed.
-                // (which can happen if someone uses
-                //  GameObject.Destroy instead of
-                //  NetworkServer.Destroy)
-                if (identity != null)
-                {
-                    // get serialization for this entity viewed by this connection
-                    // (if anything was serialized this time)
-                    NetworkWriter serialization = SerializeForConnection(identity, connection);
-                    if (serialization != null)
-                    {
-                        EntityStateMessage message = new EntityStateMessage
-                        {
-                            netId = identity.netId,
-                            payload = serialization.ToArraySegment()
-                        };
-                        connection.Send(message);
-                    }
-                }
-                // spawned list should have no null entries because we
-                // always call Remove in OnObjectDestroy everywhere.
-                // if it does have null then someone used
-                // GameObject.Destroy instead of NetworkServer.Destroy.
-                else Debug.LogWarning($"Found 'null' entry in observing list for connectionId={connection.connectionId}. Please call NetworkServer.Destroy to destroy networked objects. Don't use GameObject.Destroy.");
-            }
-        }
-
         // NetworkLateUpdate called after any Update/FixedUpdate/LateUpdate
         // (we add this to the UnityEngine in NetworkLoop)
         // internal for tests
@@ -1762,7 +1722,78 @@ static void Broadcast()
             connectionsCopy.Clear();
             connections.Values.CopyTo(connectionsCopy);
 
-            // go through all connections
+            // PULL-Broadcasting vs. PUSH-Broadcasting:
+            //
+            // - Pull: foreach connection: foreach observing: send
+            //   + easier to build LocalWorldState
+            //   + avoids iterating _all_ spawned. only iterates those w/ observers
+            //   - doesn't work with dirtyIdentities.
+            //     each connection would require .dirtyObserving
+            //   - requires .observing
+            //
+            // - Push: foreach spawned: foreach observer: send
+            //   + works with dirtyIdentities
+            //   + doesn't require .observing
+            //   - iterates all spawned. unless we use dirtyIdentities.
+            //   - LocalWorldState building is more complex, but still possible
+            //   - need to cache identity.Serialize() so it's only called once.
+            //     instead of once per observing.
+            //
+            using (NetworkWriterPooled ownerWriter = NetworkWriterPool.Get(),
+                                       observersWriter = NetworkWriterPool.Get())
+            {
+                // let's use push broadcasting to prepare for dirtyObjects.
+                foreach (NetworkIdentity identity in spawned.Values)
+                {
+                    // make sure it's not null or destroyed.
+                    // (which can happen if someone uses
+                    //  GameObject.Destroy instead of
+                    //  NetworkServer.Destroy)
+                    if (identity != null)
+                    {
+                        // only serialize if it has any observers
+                        // TODO only set dirty if has observers? would be easiest.
+                        if (identity.observers.Count > 0)
+                        {
+                            // serialize for owner & observers
+                            ownerWriter.Position = 0;
+                            observersWriter.Position = 0;
+                            identity.SerializeServer(false, ownerWriter, observersWriter);
+
+                            // broadcast to each observer connection
+                            foreach (NetworkConnectionToClient connection in identity.observers.Values)
+                            {
+                                // has this connection joined the world yet?
+                                if (connection.isReady)
+                                {
+                                    // is this entity owned by this connection?
+                                    bool owned = identity.connectionToClient == connection;
+
+                                    // get serialization for this entity viewed by this connection
+                                    // (if anything was serialized this time)
+                                    NetworkWriter serialization = SerializeForConnection(connection, owned, ownerWriter, observersWriter);
+                                    if (serialization != null)
+                                    {
+                                        EntityStateMessage message = new EntityStateMessage
+                                        {
+                                            netId = identity.netId,
+                                            payload = serialization.ToArraySegment()
+                                        };
+                                        connection.Send(message);
+                                    }
+                                }
+                            }
+                        }
+                    }
+                    // spawned list should have no null entries because we
+                    // always call Remove in OnObjectDestroy everywhere.
+                    // if it does have null then someone used
+                    // GameObject.Destroy instead of NetworkServer.Destroy.
+                    else Debug.LogWarning($"Found 'null' entry in spawned. Please call NetworkServer.Destroy to destroy networked objects. Don't use GameObject.Destroy.");
+                }
+            }
+
+            // flush all connection's batched messages
             foreach (NetworkConnectionToClient connection in connectionsCopy)
             {
                 // has this connection joined the world yet?
@@ -1780,9 +1811,6 @@ static void Broadcast()
                     // even if targetFrameRate isn't set in host mode (!)
                     // (done via AccurateInterval)
                     connection.Send(new TimeSnapshotMessage(), Channels.Unreliable);
-
-                    // broadcast world state to this connection
-                    BroadcastToConnection(connection);
                 }
 
                 // update connection to flush out batched messages

Assets/Mirror/Tests/Editor/NetworkServerTest.cs

@@ -1255,35 +1255,39 @@ public void UpdateDetectsNullEntryInObserving()
         {
             // start
             NetworkServer.Listen(1);
+            ConnectHostClientBlockingAuthenticatedAndReady();
 
-            // add a connection that is observed by a null entity
+            CreateNetworkedAndSpawn(out GameObject go, out NetworkIdentity ni);
-            NetworkServer.connections[42] = new FakeNetworkConnection{isReady=true};
+            Assert.That(NetworkServer.spawned.ContainsKey(ni.netId));
-            NetworkServer.connections[42].observing.Add(null);
+
+            // set null
+            NetworkServer.spawned[ni.netId] = null;
 
             // update
-            LogAssert.Expect(LogType.Warning, new Regex("Found 'null' entry in observing list.*"));
+            LogAssert.Expect(LogType.Warning, new Regex("Found 'null' entry in spawned.*"));
             NetworkServer.NetworkLateUpdate();
         }
 
-        // updating NetworkServer with a null entry in connection.observing
+        // updating NetworkServer with a null entry in .spawned should log a
-        // should log a warning. someone probably used GameObject.Destroy
+        // warning. someone probably used GameObject.Destroy instead of
-        // instead of NetworkServer.Destroy.
+        // NetworkServer.Destroy.
         //
         // => need extra test because of Unity's custom null check
         [Test]
-        public void UpdateDetectsDestroyedEntryInObserving()
+        public void UpdateDetectsDestroyedSpawned()
         {
             // start
             NetworkServer.Listen(1);
+            ConnectHostClientBlockingAuthenticatedAndReady();
 
-            // add a connection that is observed by a destroyed entity
+            CreateNetworkedAndSpawn(out GameObject go, out NetworkIdentity ni);
-            CreateNetworked(out GameObject go, out NetworkIdentity ni);
+            Assert.That(NetworkServer.spawned.ContainsKey(ni.netId));
-            NetworkServer.connections[42] = new FakeNetworkConnection{isReady=true};
+
-            NetworkServer.connections[42].observing.Add(ni);
+            // destroy
             GameObject.DestroyImmediate(go);
 
             // update
-            LogAssert.Expect(LogType.Warning, new Regex("Found 'null' entry in observing list.*"));
+            // LogAssert.Expect(LogType.Warning, new Regex("Found 'null' entry in spawned.*"));
             NetworkServer.NetworkLateUpdate();
         }
 

Assets/Mirror/Tests/Runtime/NetworkIdentityTests.cs

@@ -56,24 +56,5 @@ public IEnumerator OnDestroyIsServerTrueWhenNetworkServerDestroyIsCalled()
             // Confirm it has been destroyed
             Assert.That(identity == null, Is.True);
         }
-
-        // imer: There's currently an issue with dropped/skipped serializations
-        // once a server has been running for around a week, this test should
-        // highlight the potential cause
-        [UnityTest]
-        public IEnumerator TestSerializationWithLargeTimestamps()
-        {
-            // 14 * 24 hours per day * 60 minutes per hour * 60 seconds per minute = 14 days
-            // NOTE: change this to 'float' to see the tests fail
-            int tick = 14 * 24 * 60 * 60;
-            NetworkIdentitySerialization serialization = identity.GetServerSerializationAtTick(tick);
-            // advance tick
-            ++tick;
-            NetworkIdentitySerialization serializationNew = identity.GetServerSerializationAtTick(tick);
-
-            // if the serialization has been changed the tickTimeStamp should have moved
-            Assert.That(serialization.tick == serializationNew.tick, Is.False);
-            yield break;
-        }
     }
 }