NetworkServer.dirtyObjects; NetworkBehaviour.SetDirty -> NetworkIdentity.OnBecameDirty

Assets/Mirror/Core/NetworkBehaviour.cs

@@ -155,10 +155,18 @@ protected void SetSyncVarHookGuard(ulong dirtyBit, bool value)
                 syncVarHookGuard &= ~dirtyBit;
         }
 
+        // callback for both SyncObject and SyncVar dirty bit setters.
+        // called once it becomes dirty, not called again while already dirty.
+        // we only want to follow the .netIdentity memory indirection once.
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        void OnBecameDirty() => netIdentity.OnBecameDirty();
+
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         void SetSyncObjectDirtyBit(ulong dirtyBit)
         {
+            bool clean = syncObjectDirtyBits == 0;
             syncObjectDirtyBits |= dirtyBit;
+            if (clean) OnBecameDirty();
         }
 
         /// <summary>Set as dirty so that it's synced to clients again.</summary>
@@ -166,7 +174,9 @@ void SetSyncObjectDirtyBit(ulong dirtyBit)
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public void SetSyncVarDirtyBit(ulong dirtyBit)
         {
+            bool clean = syncObjectDirtyBits == 0;
             syncVarDirtyBits |= dirtyBit;
+            if (clean) OnBecameDirty();
         }
 
         /// <summary>Set as dirty to trigger OnSerialize & send. Dirty bits are cleared after the send.</summary>
@@ -190,6 +200,12 @@ public bool IsDirty() =>
             // only check time if bits were dirty. this is more expensive.
             NetworkTime.localTime - lastSyncTime >= syncInterval;
 
+        // check only dirty bits, ignoring sync interval.
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public bool IsDirtyPending() =>
+            // check bits first. this is basically free.
+            (syncVarDirtyBits | syncObjectDirtyBits) != 0UL;
+
         /// <summary>Clears all the dirty bits that were set by SetSyncVarDirtyBit() (formally SetDirtyBits)</summary>
         // automatically invoked when an update is sent for this object, but can
         // be called manually as well.

Assets/Mirror/Core/NetworkIdentity.cs

@@ -806,19 +806,57 @@ internal void OnStopLocalPlayer()
             }
         }
 
+        // NetworkBehaviour OnBecameDirty calls NetworkIdentity callback with index
+        bool addedToDirtySpawned = false;
+        internal void OnBecameDirty()
+        {
+            // ensure either isServer or isClient are set.
+            // ensures tests are obvious. without proper setup, it should throw.
+            if (!isClient && !isServer)
+                Debug.LogWarning("NetworkIdentity.OnBecameDirty(): neither isClient nor isServer are true. Improper setup?");
+
+
+            if (isServer)
+            {
+                // only add to dirty spawned once.
+                // don't run the insertion twice.
+                if (!addedToDirtySpawned)
+                {
+                    // insert into server dirty objects if not inserted yet
+                    // TODO keep a bool so we don't insert all the time?
+
+                    // only add if observed.
+                    // otherwise no point in adding + iterating from broadcast.
+                    if (observers.Count > 0)
+                    {
+                        NetworkServer.dirtySpawned.Add(this);
+                        addedToDirtySpawned = true;
+                    }
+                }
+            }
+        }
+
         // build dirty mask for server owner & observers (= all dirty components).
         // faster to do it in one iteration instead of iterating separately.
-        (ulong, ulong) ServerDirtyMasks(bool initialState)
+        (ulong, ulong, ulong) ServerDirtyMasks(bool initialState)
         {
             ulong ownerMask = 0;
             ulong observerMask = 0;
 
+            // are any dirty but not ready to be sent yet?
+            // we need to know this because then we don't remove
+            // the NetworkIdentity from dirtyObjects just yet.
+            // otherwise if we remove before it was synced, we would miss a sync.
+            ulong dirtyPending = 0;
+
             NetworkBehaviour[] components = NetworkBehaviours;
             for (int i = 0; i < components.Length; ++i)
             {
                 NetworkBehaviour component = components[i];
 
                 bool dirty = component.IsDirty();
+                bool pending = !dirty && component.IsDirtyPending();
+
                 ulong nthBit = (1u << i);
 
                 // owner needs to be considered for both SyncModes, because
@@ -828,7 +866,10 @@ internal void OnStopLocalPlayer()
                 // for delta, only for ServerToClient and only if dirty.
                 //     ClientToServer comes from the owner client.
                 if (initialState || (component.syncDirection == SyncDirection.ServerToClient && dirty))
+                {
                     ownerMask |= nthBit;
+                    if (pending) dirtyPending |= nthBit;
+                }
 
                 // observers need to be considered only in Observers mode
                 //
@@ -837,10 +878,13 @@ internal void OnStopLocalPlayer()
                 // SyncDirection is irrelevant, as both are broadcast to
                 // observers which aren't the owner.
                 if (component.syncMode == SyncMode.Observers && (initialState || dirty))
+                {
                     observerMask |= nthBit;
+                    if (pending) dirtyPending |= nthBit;
+                }
             }
 
-            return (ownerMask, observerMask);
+            return (ownerMask, observerMask, dirtyPending);
         }
 
         // build dirty mask for client.
@@ -885,7 +929,7 @@ internal static bool IsDirty(ulong mask, int index)
         // check ownerWritten/observersWritten to know if anything was written
         // We pass dirtyComponentsMask into this function so that we can check
         // if any Components are dirty before creating writers
-        internal void SerializeServer(bool initialState, NetworkWriter ownerWriter, NetworkWriter observersWriter)
+        internal void SerializeServer(bool initialState, NetworkWriter ownerWriter, NetworkWriter observersWriter, out bool pendingDirty)
         {
             // ensure NetworkBehaviours are valid before usage
             ValidateComponents();
@@ -898,7 +942,7 @@ internal void SerializeServer(bool initialState, NetworkWriter ownerWriter, Netw
             // instead of writing a 1 byte index per component,
             // we limit components to 64 bits and write one ulong instead.
             // the ulong is also varint compressed for minimum bandwidth.
-            (ulong ownerMask, ulong observerMask) = ServerDirtyMasks(initialState);
+            (ulong ownerMask, ulong observerMask, ulong pendingMask) = ServerDirtyMasks(initialState);
 
             // if nothing dirty, then don't even write the mask.
             // otherwise, every unchanged object would send a 1 byte dirty mask!
@@ -955,6 +999,16 @@ internal void SerializeServer(bool initialState, NetworkWriter ownerWriter, Netw
                     }
                 }
             }
+
+            // are any dirty but not ready to be sent yet?
+            // we need to know this because then we don't remove
+            // the NetworkIdentity from dirtyObjects just yet.
+            // otherwise if we remove before it was synced, we would miss a sync.
+            pendingDirty = pendingMask != 0;
+
+            // if none are still pending, this will be removed from dirtyObjects.
+            // in that case, clear our flag (the flag is only for performance).
+            if (!pendingDirty) addedToDirtySpawned = false;
         }
 
         // serialize components into writer on the client.

Assets/Mirror/Core/NetworkServer.cs

@@ -48,6 +48,20 @@ public static partial class NetworkServer
         public static readonly Dictionary<uint, NetworkIdentity> spawned =
             new Dictionary<uint, NetworkIdentity>();
 
+        // all spawned which are dirty (= need broadcasting).
+        //
+        // note that some dirty objects may not be ready for broadcasting,
+        // because we add them independent of syncInterval.
+        //
+        // otherwise each NetworkBehaviour would need an Update() to wait until
+        // syncInterval is elapsed, which is more expansive then simply adding
+        // a few false positives here.
+        //
+        // NetworkIdentity adds itself to dirtySpawned exactly once.
+        // we can safely use a List<T> here, faster than a Dictionary with enumerators.
+        internal static readonly List<NetworkIdentity> dirtySpawned =
+            new List<NetworkIdentity>();
+
         /// <summary>Single player mode can use dontListen to not accept incoming connections</summary>
         // see also: https://github.com/vis2k/Mirror/pull/2595
         public static bool dontListen;
@@ -1213,7 +1227,7 @@ static ArraySegment<byte> CreateSpawnMessagePayload(bool isOwner, NetworkIdentit
 
             // serialize all components with initialState = true
             // (can be null if has none)
-            identity.SerializeServer(true, ownerWriter, observersWriter);
+            identity.SerializeServer(true, ownerWriter, observersWriter, out _);
 
             // convert to ArraySegment to avoid reader allocations
             // if nothing was written, .ToArraySegment returns an empty segment.
@@ -1627,7 +1641,6 @@ public static void RebuildObservers(NetworkIdentity identity, bool initialize)
         // broadcasting ////////////////////////////////////////////////////////
         // helper function to get the right serialization for a connection
         static NetworkWriter SerializeForConnection(
-            NetworkConnectionToClient connection,
             bool owned,
             NetworkWriter ownerWriter,
             NetworkWriter observersWriter)
@@ -1652,47 +1665,7 @@ static NetworkWriter SerializeForConnection(
             return null;
         }
 
-        static void BroadcastIdentity(
+        static void BroadcastDirtySpawned()
-            NetworkIdentity identity,
-            NetworkWriterPooled ownerWriter,
-            NetworkWriterPooled observersWriter)
-        {
-            // 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);
-                        }
-                    }
-                }
-            }
-        }
-
-        static void BroadcastSpawned()
         {
             // PULL-Broadcasting vs. PUSH-Broadcasting:
             //
@@ -1715,21 +1688,73 @@ static void BroadcastSpawned()
                                        observersWriter = NetworkWriterPool.Get())
             {
                 // let's use push broadcasting to prepare for dirtyObjects.
-                foreach (NetworkIdentity identity in spawned.Values)
+                // only iterate NetworkIdentities which we know to be dirty.
+                // for example, in an MMO we don't need to iterate NPCs,
+                // item drops, idle monsters etc. every Broadcast.
+                for (int i = 0; i < dirtySpawned.Count; ++i)
                 {
+                    NetworkIdentity identity = dirtySpawned[i];
+
                     // make sure it's not null or destroyed.
                     // (which can happen if someone uses
                     //  GameObject.Destroy instead of
                     //  NetworkServer.Destroy)
                     if (identity != null)
                     {
-                        BroadcastIdentity(identity, ownerWriter, observersWriter);
+                        // 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, out bool pendingDirty);
+
+                            // 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(owned, ownerWriter, observersWriter);
+                                    if (serialization != null)
+                                    {
+                                        EntityStateMessage message = new EntityStateMessage
+                                        {
+                                            netId = identity.netId,
+                                            payload = serialization.ToArraySegment()
+                                        };
+                                        connection.Send(message);
+                                    }
+                                }
+                            }
+
+                            // if there are no more dirty components pending,
+                            // then remove this in place
+                            if (!pendingDirty)
+                            {
+                                // List.RemoveAt(i) is O(N).
+                                // instead, use O(1) swap-remove from Rust.
+                                // dirtySpawned.RemoveAt(i);
+
+                                dirtySpawned.SwapRemove(i);
+
+                                // the last element was moved to 'i'.
+                                // count was reduced by 1.
+                                // our for-int loop checks .Count, nothing more to do here.
+                            }
+                        }
                     }
                     // 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.");
+                    else Debug.LogWarning($"Found 'null' entry in dirtySpawned. Please call NetworkServer.Destroy to destroy networked objects. Don't use GameObject.Destroy.");
                 }
             }
         }
@@ -1800,7 +1825,7 @@ static void Broadcast()
             connections.Values.CopyTo(connectionsCopy);
 
             // broadcast spawned entities
-            BroadcastSpawned();
+            BroadcastDirtySpawned();
 
             // flush all connection's batched messages
             FlushConnections();

Assets/Mirror/Core/Tools/Extensions.cs

@@ -60,5 +60,20 @@ public static bool TryDequeue<T>(this Queue<T> source, out T element)
             return false;
         }
 #endif
+
+        // List.RemoveAt is O(N).
+        // implement Rust's swap-remove O(1) removal technique.
+        public static void SwapRemove<T>(this List<T> list, int index)
+        {
+            // we can only swap if we have at least two elements
+            if (list.Count >= 2)
+            {
+                // copy last element to index
+                list[index] = list[list.Count - 1];
+            }
+
+            // remove last element
+            list.RemoveAt(list.Count - 1);
+        }
     }
 }

Assets/Mirror/Tests/Editor/NetworkIdentity/NetworkIdentitySerializationTests.cs

@@ -50,7 +50,7 @@ public void SerializeAndDeserializeAll()
             serverObserversComp.value = 42;
 
             // serialize server object
-            serverIdentity.SerializeServer(true, ownerWriter, observersWriter);
+            serverIdentity.SerializeServer(true, ownerWriter, observersWriter, out _);
 
             // deserialize client object with OWNER payload
             NetworkReader reader = new NetworkReader(ownerWriter.ToArray());
@@ -96,7 +96,7 @@ public void SerializationException()
             // serialize server object
             // should work even if compExc throws an exception.
             // error log because of the exception is expected.
-            serverIdentity.SerializeServer(true, ownerWriter, observersWriter);
+            serverIdentity.SerializeServer(true, ownerWriter, observersWriter, out _);
 
             // deserialize client object with OWNER payload
             // should work even if compExc throws an exception
@@ -187,7 +187,7 @@ public void SerializationMismatch()
             serverComp.value = "42";
 
             // serialize server object
-            serverIdentity.SerializeServer(true, ownerWriter, observersWriter);
+            serverIdentity.SerializeServer(true, ownerWriter, observersWriter, out _);
 
             // deserialize on client
             // ignore warning log because of serialization mismatch
@@ -219,7 +219,7 @@ public void SerializeServer_NotInitial_NotDirty_WritesNothing()
             // serialize server object.
             // 'initial' would write everything.
             // instead, try 'not initial' with 0 dirty bits
-            serverIdentity.SerializeServer(false, ownerWriter, observersWriter);
+            serverIdentity.SerializeServer(false, ownerWriter, observersWriter, out _);
             Assert.That(ownerWriter.Position, Is.EqualTo(0));
             Assert.That(observersWriter.Position, Is.EqualTo(0));
         }
@@ -285,7 +285,7 @@ public void SerializeServer_OwnerMode_ClientToServer()
             comp.SetValue(11); // modify with helper function to avoid #3525
 
             // initial: should still write for owner
-            identity.SerializeServer(true, ownerWriter, observersWriter);
+            identity.SerializeServer(true, ownerWriter, observersWriter, out _);
             Debug.Log("initial ownerWriter: " + ownerWriter);
             Debug.Log("initial observerWriter: " + observersWriter);
             Assert.That(ownerWriter.Position, Is.GreaterThan(0));
@@ -295,7 +295,7 @@ public void SerializeServer_OwnerMode_ClientToServer()
             comp.SetValue(22); // modify with helper function to avoid #3525
             ownerWriter.Position = 0;
             observersWriter.Position = 0;
-            identity.SerializeServer(false, ownerWriter, observersWriter);
+            identity.SerializeServer(false, ownerWriter, observersWriter, out _);
             Debug.Log("delta ownerWriter: " + ownerWriter);
             Debug.Log("delta observersWriter: " + observersWriter);
             Assert.That(ownerWriter.Position, Is.EqualTo(0));
@@ -323,7 +323,7 @@ public void SerializeServer_ObserversMode_ClientToServer()
             comp.SetValue(11); // modify with helper function to avoid #3525
 
             // initial: should write something for owner and observers
-            identity.SerializeServer(true, ownerWriter, observersWriter);
+            identity.SerializeServer(true, ownerWriter, observersWriter, out _);
             Debug.Log("initial ownerWriter: " + ownerWriter);
             Debug.Log("initial observerWriter: " + observersWriter);
             Assert.That(ownerWriter.Position, Is.GreaterThan(0));
@@ -333,7 +333,7 @@ public void SerializeServer_ObserversMode_ClientToServer()
             comp.SetValue(22); // modify with helper function to avoid #3525
             ownerWriter.Position = 0;
             observersWriter.Position = 0;
-            identity.SerializeServer(false, ownerWriter, observersWriter);
+            identity.SerializeServer(false, ownerWriter, observersWriter, out _);
             Debug.Log("delta ownerWriter: " + ownerWriter);
             Debug.Log("delta observersWriter: " + observersWriter);
             Assert.That(ownerWriter.Position, Is.EqualTo(0));

Assets/Mirror/Tests/Editor/SyncVars/SyncVarAttributeTest.cs

@@ -404,7 +404,7 @@ public void TestSyncingAbstractNetworkBehaviour()
             NetworkWriter ownerWriter = new NetworkWriter();
             // not really used in this Test
             NetworkWriter observersWriter = new NetworkWriter();
-            serverIdentity.SerializeServer(true, ownerWriter, observersWriter);
+            serverIdentity.SerializeServer(true, ownerWriter, observersWriter, out _);
 
             // set up a "client" object
             CreateNetworked(out _, out NetworkIdentity clientIdentity, out SyncVarAbstractNetworkBehaviour clientBehaviour);