Skip to content

Commit 0466b85

Browse files
NoelStephensUnityNoelStephensUnity
authored
fix: NetworkShow client synchronization duplicate players and NetworkShow with ChangeOwnership fixes [Backport] (#3493)
The PR resolves the issue discovered when investigating the [Forum-1651980 issue](https://discussions.unity.com/t/netcode-for-gameobjects-2-4-0-released/1651980/2). The initial client synchronization pre-serialization preparation was not excluding any spawned `NetworkObject` instances that had a pending visibility update for the client being synchronized. This fix adds this check to that process. [MTTB-1372](https://jira.unity3d.com/browse/MTTB-1372) This addresses the [Forum-Support-1646996](https://discussions.unity.com/t/does-networkshow-assign-ownership-internally-in-unity-netcode/1646996/10) issue where it was determined if you invoke `NetworkObject.NetworkShow` and then immediately invoke `NetworkObject.ChangeOwnership` (or somewhere within the same callstack for that frame) the target client will get an error regarding an unnecessary `ChangeOwnershipMessage`. [MTTB-1337](https://jira.unity3d.com/browse/MTTB-1337) ## Changelog - Fixed: Issue where the initial client synchronization pre-serialization process was not excluding spawned `NetworkObjects` that already had pending visibility for the client being synchronized. - Fixed: Issue where invoking `NetworkObject.NetworkShow` and `NetworkObject.ChangeOwnership` consecutively within the same call stack location could result in an unnecessary change in ownership error message generated on the target client side. ## Testing and Documentation - Includes integration tests. - No Documentation changes were required. <!-- Uncomment and mark items off with a * if this PR deprecates any API: ### Deprecated API - [ ] An `[Obsolete]` attribute was added along with a `(RemovedAfter yyyy-mm-dd)` entry. - [ ] An [api updater] was added. - [ ] Deprecation of the API is explained in the CHANGELOG. - [ ] The users can understand why this API was removed and what they should use instead. --> ## Backport This is a back port of #3488. This is a partial back port of #3468. <!-- If this is a backport: - Add the following to the PR title: "\[Backport\] ..." . - Link to the original PR. If this needs a backport - state this here If a backport is not needed please provide the reason why. If the "Backports" section is not present it will lead to a CI test failure. -->
1 parent 7ed7a52 commit 0466b85

File tree

7 files changed

+224
-0
lines changed

7 files changed

+224
-0
lines changed

‎com.unity.netcode.gameobjects/CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
1515

1616
### Fixed
1717

18+
- Fixed issue where the initial client synchronization pre-serialization process was not excluding spawned `NetworkObjects` that already had pending visibility for the client being synchronized. (#3493)
19+
- Fixed issue where invoking `NetworkObject.NetworkShow` and `NetworkObject.ChangeOwnership` consecutively within the same call stack location could result in an unnecessary change in ownership error message generated on the target client side. (#3493)
1820
- Fixed issue where `NetworkVariable`s on a `NetworkBehaviour` could fail to synchronize changes if one has `NetworkVariableUpdateTraits` set and is dirty but is not ready to send. (#3465)
1921
- Fixed issue where when a client changes ownership via RPC the `NetworkBehaviour.OnOwnershipChanged` can result in identical previous and current owner identifiers. (#3434)
2022

‎com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,15 @@ public void Handle(ref NetworkContext context)
3535
{
3636
var networkManager = (NetworkManager)context.SystemOwner;
3737
var networkObject = networkManager.SpawnManager.SpawnedObjects[NetworkObjectId];
38+
39+
// Sanity check that we are not sending duplicated or unnecessary change ownership messages
40+
if (networkObject.OwnerClientId == OwnerClientId)
41+
{
42+
// Log error and then ignore the message
43+
NetworkLog.LogError($"[Receiver: Client-{networkManager.LocalClientId}][Sender: Client-{context.SenderId}] Detected unnecessary ownership changed message for {networkObject.name} (NID:{NetworkObjectId}).");
44+
return;
45+
}
46+
3847
var originalOwner = networkObject.OwnerClientId;
3948

4049
networkObject.OwnerClientId = OwnerClientId;

‎com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventData.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,12 @@ internal void AddSpawnedNetworkObjects()
317317
m_NetworkObjectsSync.Clear();
318318
foreach (var sobj in m_NetworkManager.SpawnManager.SpawnedObjectsList)
319319
{
320+
var spawnedObject = sobj;
321+
// Don't synchronize objects that have pending visibility as that will be sent as a CreateObjectMessage towards the end of the current frame
322+
if (TargetClientId != NetworkManager.ServerClientId && m_NetworkManager.SpawnManager.IsObjectVisibilityPending(TargetClientId, ref spawnedObject))
323+
{
324+
continue;
325+
}
320326
if (sobj.Observers.Contains(TargetClientId))
321327
{
322328
m_NetworkObjectsSync.Add(sobj);

‎com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,10 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId)
321321

322322
foreach (var client in NetworkManager.ConnectedClients)
323323
{
324+
if (IsObjectVisibilityPending(client.Key, ref networkObject))
325+
{
326+
continue;
327+
}
324328
if (networkObject.IsNetworkVisibleTo(client.Value.ClientId))
325329
{
326330
var size = NetworkManager.ConnectionManager.SendMessage(ref message, NetworkDelivery.ReliableSequenced, client.Value.ClientId);
@@ -343,6 +347,23 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId)
343347
m_LastChangeInOwnership[networkObject.NetworkObjectId] = Time.realtimeSinceStartup + (tickFrequency * k_MaximumTickOwnershipChangeMultiplier);
344348
}
345349

350+
/// <summary>
351+
/// Will determine if a client has been granted visibility for a NetworkObject but
352+
/// the <see cref="CreateObjectMessage"/> has yet to be generated for it. Under this case,
353+
/// the client might not need to be sent a message (i.e. <see cref="ChangeOwnershipMessage")
354+
/// </summary>
355+
/// <param name="clientId">the client to check</param>
356+
/// <param name="networkObject">the <see cref="NetworkObject"/> to check if it is pending show</param>
357+
[System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
358+
internal bool IsObjectVisibilityPending(ulong clientId, ref NetworkObject networkObject)
359+
{
360+
if (ObjectsToShowToClient.ContainsKey(clientId))
361+
{
362+
return ObjectsToShowToClient[clientId].Contains(networkObject);
363+
}
364+
return false;
365+
}
366+
346367
internal bool HasPrefab(NetworkObject.SceneObject sceneObject)
347368
{
348369
if (!NetworkManager.NetworkConfig.EnableSceneManagement || !sceneObject.IsSceneObject)

‎com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/PlayerSpawnObjectVisibilityTests.cs

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
using System.Collections;
2+
using System.Text.RegularExpressions;
3+
using NUnit.Framework;
4+
using Unity.Netcode.TestHelpers.Runtime;
5+
using UnityEngine;
6+
using UnityEngine.TestTools;
7+
8+
9+
namespace Unity.Netcode.RuntimeTests
10+
{
11+
[TestFixture(HostOrServer.Server)]
12+
[TestFixture(HostOrServer.Host)]
13+
internal class PlayerSpawnObjectVisibilityTests : NetcodeIntegrationTest
14+
{
15+
protected override int NumberOfClients => 0;
16+
17+
public enum PlayerSpawnStages
18+
{
19+
OnNetworkSpawn,
20+
OnNetworkPostSpawn,
21+
}
22+
23+
public PlayerSpawnObjectVisibilityTests(HostOrServer hostOrServer) : base(hostOrServer) { }
24+
25+
public class PlayerVisibilityTestComponent : NetworkBehaviour
26+
{
27+
public PlayerSpawnStages Stage;
28+
29+
private void Awake()
30+
{
31+
var networkObject = GetComponent<NetworkObject>();
32+
// Assure the player prefab will not spawn with observers.
33+
// This assures that when the server/host spawns the connecting client's
34+
// player prefab, the spawn object will initially not be spawnd on the client side.
35+
networkObject.SpawnWithObservers = false;
36+
}
37+
38+
public override void OnNetworkSpawn()
39+
{
40+
ShowToClient(PlayerSpawnStages.OnNetworkSpawn);
41+
base.OnNetworkSpawn();
42+
}
43+
44+
protected override void OnNetworkPostSpawn()
45+
{
46+
ShowToClient(PlayerSpawnStages.OnNetworkPostSpawn);
47+
base.OnNetworkPostSpawn();
48+
}
49+
50+
private void ShowToClient(PlayerSpawnStages currentStage)
51+
{
52+
if (!IsServer || Stage != currentStage)
53+
{
54+
return;
55+
}
56+
NetworkObject.NetworkShow(OwnerClientId);
57+
}
58+
}
59+
60+
protected override void OnCreatePlayerPrefab()
61+
{
62+
m_PlayerPrefab.AddComponent<PlayerVisibilityTestComponent>();
63+
base.OnCreatePlayerPrefab();
64+
}
65+
66+
/// <summary>
67+
/// Tests the scenario where under a client-server network topology if a player prefab
68+
/// is spawned by the server with no observers but the player prefab itself has server
69+
/// side script that will network show the spawned object to the owning client.
70+
///
71+
/// Because NetworkShow will defer the CreateObjectMessage until the late update, the
72+
/// server/host needs to filter out including anything within the synchronization
73+
/// message that already has pending visibility.
74+
/// </summary>
75+
/// <param name="spawnStage">Spawn stages to test</param>
76+
/// <returns>IEnumerator</returns>
77+
[UnityTest]
78+
public IEnumerator NetworkShowOnSpawnTest([Values] PlayerSpawnStages spawnStage)
79+
{
80+
m_PlayerPrefab.GetComponent<PlayerVisibilityTestComponent>().Stage = spawnStage;
81+
82+
yield return CreateAndStartNewClient();
83+
84+
yield return new WaitForSeconds(0.25f);
85+
86+
NetcodeLogAssert.LogWasNotReceived(LogType.Warning, new Regex("but it is already in the spawned list!"));
87+
var client = m_ClientNetworkManagers[0];
88+
Assert.True(client.LocalClient.PlayerObject != null, $"Client-{client.LocalClientId} does not have a player object!");
89+
}
90+
}
91+
}

‎com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/PlayerSpawnObjectVisibilityTests.cs.meta

Lines changed: 11 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎com.unity.netcode.gameobjects/Tests/Runtime/NetworkShowHideTests.cs

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -601,5 +601,89 @@ public IEnumerator NetworkShowHideAroundListModify()
601601
Compare(ShowHideObject.ObjectsPerClientId[0].MyList, ShowHideObject.ObjectsPerClientId[2].MyList);
602602
}
603603
}
604+
605+
private GameObject m_OwnershipObject;
606+
private NetworkObject m_OwnershipNetworkObject;
607+
private NetworkManager m_NewOwner;
608+
private ulong m_ObjectId;
609+
610+
611+
private bool AllObjectsSpawnedOnClients()
612+
{
613+
foreach (var networkManager in m_ClientNetworkManagers)
614+
{
615+
if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(m_OwnershipNetworkObject.NetworkObjectId))
616+
{
617+
return false;
618+
}
619+
}
620+
return true;
621+
}
622+
623+
private bool ObjectHiddenOnNonAuthorityClients()
624+
{
625+
foreach (var networkManager in m_ClientNetworkManagers)
626+
{
627+
if (networkManager.LocalClientId == m_OwnershipNetworkObject.OwnerClientId)
628+
{
629+
continue;
630+
}
631+
if (networkManager.SpawnManager.SpawnedObjects.ContainsKey(m_OwnershipNetworkObject.NetworkObjectId))
632+
{
633+
return false;
634+
}
635+
}
636+
return true;
637+
}
638+
639+
private bool OwnershipHasChanged()
640+
{
641+
if (!m_NewOwner.SpawnManager.SpawnedObjects.ContainsKey(m_ObjectId))
642+
{
643+
return false;
644+
}
645+
return m_NewOwner.SpawnManager.SpawnedObjects[m_ObjectId].OwnerClientId == m_NewOwner.LocalClientId;
646+
}
647+
648+
/// <summary>
649+
/// Validates when invoking NetworkObject.NetworkShow and NetworkObject.ChangeOwnership
650+
/// back-to-back it will not attempt to send a change ownership message since the visibility
651+
/// message (CreateObjectMessage) is deferred until the end of the frame.
652+
/// </summary>
653+
/// <returns>IEnumerator</returns>
654+
[UnityTest]
655+
public IEnumerator NetworkShowAndChangeOwnership()
656+
{
657+
var authority = m_ServerNetworkManager;
658+
659+
m_OwnershipObject = SpawnObject(m_PrefabToSpawn, authority);
660+
m_OwnershipNetworkObject = m_OwnershipObject.GetComponent<NetworkObject>();
661+
662+
yield return WaitForConditionOrTimeOut(AllObjectsSpawnedOnClients);
663+
AssertOnTimeout("Timed out waiting for all clients to spawn the ownership object!");
664+
665+
VerboseDebug($"Hiding object {m_OwnershipNetworkObject.NetworkObjectId} on all clients");
666+
foreach (var client in m_ClientNetworkManagers)
667+
{
668+
m_OwnershipNetworkObject.NetworkHide(client.LocalClientId);
669+
}
670+
671+
yield return WaitForConditionOrTimeOut(ObjectHiddenOnNonAuthorityClients);
672+
AssertOnTimeout("Timed out waiting for all clients to hide the ownership object!");
673+
674+
m_NewOwner = m_ClientNetworkManagers[0];
675+
Assert.AreNotEqual(m_OwnershipNetworkObject.OwnerClientId, m_NewOwner.LocalClientId, $"Client-{m_NewOwner.LocalClientId} should not have ownership of object {m_OwnershipNetworkObject.NetworkObjectId}!");
676+
Assert.False(m_NewOwner.SpawnManager.SpawnedObjects.ContainsKey(m_OwnershipNetworkObject.NetworkObjectId), $"Client-{m_NewOwner.LocalClientId} should not have object {m_OwnershipNetworkObject.NetworkObjectId} spawned!");
677+
678+
// Run NetworkShow and ChangeOwnership directly after one-another
679+
VerboseDebug($"Calling {nameof(NetworkObject.NetworkShow)} on object {m_OwnershipNetworkObject.NetworkObjectId} for client {m_NewOwner.LocalClientId}");
680+
m_OwnershipNetworkObject.NetworkShow(m_NewOwner.LocalClientId);
681+
VerboseDebug($"Calling {nameof(NetworkObject.ChangeOwnership)} on object {m_OwnershipNetworkObject.NetworkObjectId} for client {m_NewOwner.LocalClientId}");
682+
m_OwnershipNetworkObject.ChangeOwnership(m_NewOwner.LocalClientId);
683+
m_ObjectId = m_OwnershipNetworkObject.NetworkObjectId;
684+
yield return WaitForConditionOrTimeOut(OwnershipHasChanged);
685+
AssertOnTimeout($"Timed out waiting for clients-{m_NewOwner.LocalClientId} to gain ownership of object {m_OwnershipNetworkObject.NetworkObjectId}!");
686+
VerboseDebug($"Client {m_NewOwner.LocalClientId} now owns object {m_OwnershipNetworkObject.NetworkObjectId}!");
687+
}
604688
}
605689
}

0 commit comments

Comments
 (0)