From 9d291a9d894ee96766e73f265a3b5c4a091aa347 Mon Sep 17 00:00:00 2001 From: mischa Date: Thu, 3 Aug 2023 12:42:01 +0800 Subject: [PATCH 1/5] push->pull broadcasting --- Assets/Mirror/Core/NetworkIdentity.cs | 56 ------ Assets/Mirror/Core/NetworkServer.cs | 166 ++++++++++++------ .../Editor/NetworkServer/NetworkServerTest.cs | 30 ++-- .../Tests/Runtime/NetworkIdentityTests.cs | 19 -- 4 files changed, 126 insertions(+), 145 deletions(-) diff --git a/Assets/Mirror/Core/NetworkIdentity.cs b/Assets/Mirror/Core/NetworkIdentity.cs index a1d73978a5..18428ee9e2 100644 --- a/Assets/Mirror/Core/NetworkIdentity.cs +++ b/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; - } - /// NetworkIdentity identifies objects across the network. [DisallowMultipleComponent] // NetworkIdentity.Awake initializes all NetworkComponents. @@ -203,19 +195,6 @@ internal set [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 sceneIds = new Dictionary(); @@ -1102,41 +1081,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; - } - internal void AddObserver(NetworkConnectionToClient conn) { if (observers.ContainsKey(conn.connectionId)) diff --git a/Assets/Mirror/Core/NetworkServer.cs b/Assets/Mirror/Core/NetworkServer.cs index b3acb465bb..b46f2d572b 100644 --- a/Assets/Mirror/Core/NetworkServer.cs +++ b/Assets/Mirror/Core/NetworkServer.cs @@ -1626,65 +1626,111 @@ 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) - return serialization.ownerWriter; + if (ownerWriter.Position > 0) + return ownerWriter; } // observers writer if not owned else { // was it dirty / did we actually serialize anything? - if (serialization.observersWriter.Position > 0) - return serialization.observersWriter; + if (observersWriter.Position > 0) + return observersWriter; } // nothing was serialized return null; } - // helper function to broadcast the world to a connection - static void BroadcastToConnection(NetworkConnectionToClient connection) + static void BroadcastIdentity( + NetworkIdentity identity, + NetworkWriterPooled ownerWriter, + NetworkWriterPooled observersWriter) { - // for each entity that this connection is seeing - foreach (NetworkIdentity identity in connection.observing) + // only serialize if it has any observers + // TODO only set dirty if has observers? would be easiest. + if (identity.observers.Count > 0) { - // make sure it's not null or destroyed. - // (which can happen if someone uses - // GameObject.Destroy instead of - // NetworkServer.Destroy) - if (identity != null) + // 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) { - // get serialization for this entity viewed by this connection - // (if anything was serialized this time) - NetworkWriter serialization = SerializeForConnection(identity, connection); - if (serialization != null) + // has this connection joined the world yet? + if (connection.isReady) { - EntityStateMessage message = new EntityStateMessage + // 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) { - netId = identity.netId, - payload = serialization.ToArraySegment() - }; - connection.Send(message); + EntityStateMessage message = new EntityStateMessage + { + netId = identity.netId, + payload = serialization.ToArraySegment() + }; + connection.Send(message); + } + } + } + } + } + + static void BroadcastSpawned() + { + // 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) + { + BroadcastIdentity(identity, ownerWriter, observersWriter); } + // 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."); } - // 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."); } } @@ -1703,26 +1749,9 @@ static bool DisconnectIfInactive(NetworkConnectionToClient connection) return false; } - // NetworkLateUpdate called after any Update/FixedUpdate/LateUpdate - // (we add this to the UnityEngine in NetworkLoop) - // internal for tests - internal static readonly List connectionsCopy = - new List(); - - static void Broadcast() + // flush all connection's batched messages + static void FlushConnections() { - // copy all connections into a helper collection so that - // OnTransportDisconnected can be called while iterating. - // -> OnTransportDisconnected removes from the collection - // -> which would throw 'can't modify while iterating' errors - // => see also: https://github.com/vis2k/Mirror/issues/2739 - // (copy nonalloc) - // TODO remove this when we move to 'lite' transports with only - // socket send/recv later. - connectionsCopy.Clear(); - connections.Values.CopyTo(connectionsCopy); - - // go through all connections foreach (NetworkConnectionToClient connection in connectionsCopy) { // check for inactivity. disconnects if necessary. @@ -1744,14 +1773,37 @@ 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 connection.Update(); } + } + + // NetworkLateUpdate called after any Update/FixedUpdate/LateUpdate + // (we add this to the UnityEngine in NetworkLoop) + // internal for tests + internal static readonly List connectionsCopy = + new List(); + + static void Broadcast() + { + // copy all connections into a helper collection so that + // OnTransportDisconnected can be called while iterating. + // -> OnTransportDisconnected removes from the collection + // -> which would throw 'can't modify while iterating' errors + // => see also: https://github.com/vis2k/Mirror/issues/2739 + // (copy nonalloc) + // TODO remove this when we move to 'lite' transports with only + // socket send/recv later. + connectionsCopy.Clear(); + connections.Values.CopyTo(connectionsCopy); + + // broadcast spawned entities + BroadcastSpawned(); + + // flush all connection's batched messages + FlushConnections(); // TODO this is way too slow because we iterate ALL spawned :/ // TODO this is way too complicated :/ diff --git a/Assets/Mirror/Tests/Editor/NetworkServer/NetworkServerTest.cs b/Assets/Mirror/Tests/Editor/NetworkServer/NetworkServerTest.cs index 0bca731b5f..7d7c5af1e1 100644 --- a/Assets/Mirror/Tests/Editor/NetworkServer/NetworkServerTest.cs +++ b/Assets/Mirror/Tests/Editor/NetworkServer/NetworkServerTest.cs @@ -1221,35 +1221,39 @@ public void UpdateDetectsNullEntryInObserving() { // start NetworkServer.Listen(1); + ConnectHostClientBlockingAuthenticatedAndReady(); + + CreateNetworkedAndSpawn(out GameObject go, out NetworkIdentity ni); + Assert.That(NetworkServer.spawned.ContainsKey(ni.netId)); - // add a connection that is observed by a null entity - NetworkServer.connections[42] = new FakeNetworkConnectionToClient{isReady=true}; - 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 - // should log a warning. someone probably used GameObject.Destroy - // instead of NetworkServer.Destroy. + // updating NetworkServer with a null entry in .spawned should log a + // warning. someone probably used GameObject.Destroy instead of + // NetworkServer.Destroy. // // => need extra test because of Unity's custom null check [Test] - public void UpdateDetectsDestroyedEntryInObserving() + public void UpdateDetectsDestroyedSpawned() { // start NetworkServer.Listen(1); + ConnectHostClientBlockingAuthenticatedAndReady(); + + CreateNetworkedAndSpawn(out GameObject go, out NetworkIdentity ni); + Assert.That(NetworkServer.spawned.ContainsKey(ni.netId)); - // add a connection that is observed by a destroyed entity - CreateNetworked(out GameObject go, out NetworkIdentity ni); - NetworkServer.connections[42] = new FakeNetworkConnectionToClient{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(); } diff --git a/Assets/Mirror/Tests/Runtime/NetworkIdentityTests.cs b/Assets/Mirror/Tests/Runtime/NetworkIdentityTests.cs index f478704270..495e75c8f2 100644 --- a/Assets/Mirror/Tests/Runtime/NetworkIdentityTests.cs +++ b/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; - } } } From 0b484d08309dff3df4e9aeec1d08547c39ba08fc Mon Sep 17 00:00:00 2001 From: mischa Date: Thu, 3 Aug 2023 12:46:45 +0800 Subject: [PATCH 2/5] NetworkServer.dirtyObjects; NetworkBehaviour.SetDirty -> NetworkIdentity.OnBecameDirty --- Assets/Mirror/Core/NetworkBehaviour.cs | 16 +++ Assets/Mirror/Core/NetworkIdentity.cs | 62 ++++++++- Assets/Mirror/Core/NetworkServer.cs | 119 +++++++++++------- Assets/Mirror/Core/Tools/Extensions.cs | 15 +++ .../NetworkIdentitySerializationTests.cs | 16 +-- .../Editor/SyncVars/SyncVarAttributeTest.cs | 2 +- 6 files changed, 170 insertions(+), 60 deletions(-) diff --git a/Assets/Mirror/Core/NetworkBehaviour.cs b/Assets/Mirror/Core/NetworkBehaviour.cs index affca80707..e189d98abc 100644 --- a/Assets/Mirror/Core/NetworkBehaviour.cs +++ b/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(); } /// Set as dirty so that it's synced to clients again. @@ -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(); } /// Set as dirty to trigger OnSerialize & send. Dirty bits are cleared after the send. @@ -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; + /// Clears all the dirty bits that were set by SetSyncVarDirtyBit() (formally SetDirtyBits) // automatically invoked when an update is sent for this object, but can // be called manually as well. diff --git a/Assets/Mirror/Core/NetworkIdentity.cs b/Assets/Mirror/Core/NetworkIdentity.cs index 18428ee9e2..39d9f815aa 100644 --- a/Assets/Mirror/Core/NetworkIdentity.cs +++ b/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. diff --git a/Assets/Mirror/Core/NetworkServer.cs b/Assets/Mirror/Core/NetworkServer.cs index b46f2d572b..f1693a41df 100644 --- a/Assets/Mirror/Core/NetworkServer.cs +++ b/Assets/Mirror/Core/NetworkServer.cs @@ -48,6 +48,20 @@ public static partial class NetworkServer public static readonly Dictionary spawned = new Dictionary(); + // 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 here, faster than a Dictionary with enumerators. + internal static readonly List dirtySpawned = + new List(); + /// Single player mode can use dontListen to not accept incoming connections // see also: https://github.com/vis2k/Mirror/pull/2595 public static bool dontListen; @@ -1213,7 +1227,7 @@ static ArraySegment 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( - 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() + static void BroadcastDirtySpawned() { // 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(); diff --git a/Assets/Mirror/Core/Tools/Extensions.cs b/Assets/Mirror/Core/Tools/Extensions.cs index 02407e09fc..3093f5892f 100644 --- a/Assets/Mirror/Core/Tools/Extensions.cs +++ b/Assets/Mirror/Core/Tools/Extensions.cs @@ -60,5 +60,20 @@ public static bool TryDequeue(this Queue 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(this List 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); + } } } diff --git a/Assets/Mirror/Tests/Editor/NetworkIdentity/NetworkIdentitySerializationTests.cs b/Assets/Mirror/Tests/Editor/NetworkIdentity/NetworkIdentitySerializationTests.cs index 9d466f0260..8596c1daf3 100644 --- a/Assets/Mirror/Tests/Editor/NetworkIdentity/NetworkIdentitySerializationTests.cs +++ b/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)); diff --git a/Assets/Mirror/Tests/Editor/SyncVars/SyncVarAttributeTest.cs b/Assets/Mirror/Tests/Editor/SyncVars/SyncVarAttributeTest.cs index b40d7360f5..faf8373b77 100644 --- a/Assets/Mirror/Tests/Editor/SyncVars/SyncVarAttributeTest.cs +++ b/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); From a7c88f0713dbaa919cca211dce51e453c2215a19 Mon Sep 17 00:00:00 2001 From: mischa Date: Fri, 4 Aug 2023 13:21:37 +0800 Subject: [PATCH 3/5] SwapRemove test coverage --- .../Tests/Editor/Tools/ExtensionsTest.cs | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/Assets/Mirror/Tests/Editor/Tools/ExtensionsTest.cs b/Assets/Mirror/Tests/Editor/Tools/ExtensionsTest.cs index 07c71afa5b..e1f40de3e9 100644 --- a/Assets/Mirror/Tests/Editor/Tools/ExtensionsTest.cs +++ b/Assets/Mirror/Tests/Editor/Tools/ExtensionsTest.cs @@ -30,5 +30,47 @@ public void ArraySegment_ToHexString() ArraySegment segment = new ArraySegment(new byte[] {0xAA, 0xBB, 0xCC}); Assert.That(segment.ToHexString(), Is.EqualTo("AA-BB-CC")); } + + [Test] + public void SwapRemove() + { + List list = new List(); + + // one entry + list.Clear(); + list.Add(42); + list.SwapRemove(0); + Assert.That(list.Count, Is.EqualTo(0)); + + // multiple entries - remove first + list.Clear(); + list.Add(42); + list.Add(43); + list.Add(44); + list.SwapRemove(0); + Assert.That(list.Count, Is.EqualTo(2)); + Assert.That(list[0], Is.EqualTo(44)); + Assert.That(list[1], Is.EqualTo(43)); // order changed due to swap + + // multiple entries - remove middle + list.Clear(); + list.Add(42); + list.Add(43); + list.Add(44); + list.SwapRemove(1); + Assert.That(list.Count, Is.EqualTo(2)); + Assert.That(list[0], Is.EqualTo(42)); + Assert.That(list[1], Is.EqualTo(44)); + + // multiple entries - remove last + list.Clear(); + list.Add(42); + list.Add(43); + list.Add(44); + list.SwapRemove(2); + Assert.That(list.Count, Is.EqualTo(2)); + Assert.That(list[0], Is.EqualTo(42)); + Assert.That(list[1], Is.EqualTo(43)); + } } } From 580d5eb8a4881057eaf6d7e627e8fefbd321c16b Mon Sep 17 00:00:00 2001 From: mischa Date: Sat, 5 Aug 2023 00:58:59 +0800 Subject: [PATCH 4/5] fix index [credit: James] --- Assets/Mirror/Core/NetworkServer.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Assets/Mirror/Core/NetworkServer.cs b/Assets/Mirror/Core/NetworkServer.cs index f1693a41df..6973c9eeeb 100644 --- a/Assets/Mirror/Core/NetworkServer.cs +++ b/Assets/Mirror/Core/NetworkServer.cs @@ -1741,12 +1741,11 @@ static void BroadcastDirtySpawned() // 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. + // the last entry was swapped in here. + // we need to visit [i] (now last) again next time. + --i; } } } From 09ff2b540e35d75bd5c142737c0f683c69315939 Mon Sep 17 00:00:00 2001 From: mischa Date: Sat, 5 Aug 2023 00:59:07 +0800 Subject: [PATCH 5/5] STOPWATCH --- Assets/Mirror/Core/NetworkServer.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Assets/Mirror/Core/NetworkServer.cs b/Assets/Mirror/Core/NetworkServer.cs index 6973c9eeeb..fd1a496ef0 100644 --- a/Assets/Mirror/Core/NetworkServer.cs +++ b/Assets/Mirror/Core/NetworkServer.cs @@ -1,8 +1,10 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using Mirror.RemoteCalls; using UnityEngine; +using Debug = UnityEngine.Debug; namespace Mirror { @@ -1810,6 +1812,7 @@ static void FlushConnections() internal static readonly List connectionsCopy = new List(); + static Stopwatch watch = new Stopwatch(); static void Broadcast() { // copy all connections into a helper collection so that @@ -1823,12 +1826,16 @@ static void Broadcast() connectionsCopy.Clear(); connections.Values.CopyTo(connectionsCopy); + watch.Restart(); + // broadcast spawned entities BroadcastDirtySpawned(); // flush all connection's batched messages FlushConnections(); + Debug.Log($"Broadcast took {watch.Elapsed.TotalMilliseconds:F1} ms"); + // TODO this is way too slow because we iterate ALL spawned :/ // TODO this is way too complicated :/ // to understand what this tries to prevent, consider this example: