Skip to content

Commit a7ffde6

Browse files
authored
fix: change OwnerClientId before firing OnGainedOwnership() callback (#1092)
1 parent 611678a commit a7ffde6

File tree

3 files changed

+199
-25
lines changed

3 files changed

+199
-25
lines changed

com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs

Lines changed: 35 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -120,19 +120,23 @@ public void HandleAddObject(ulong clientId, Stream stream)
120120

121121
public void HandleDestroyObject(ulong clientId, Stream stream)
122122
{
123-
using (var reader = PooledNetworkReader.Get(stream))
123+
using var reader = PooledNetworkReader.Get(stream);
124+
ulong networkObjectId = reader.ReadUInt64Packed();
125+
126+
if (!NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(networkObjectId, out var networkObject))
124127
{
125-
ulong networkId = reader.ReadUInt64Packed();
126-
if (!NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(networkId, out NetworkObject networkObject))
128+
// This is the same check and log message that happens inside OnDespawnObject, but we have to do it here
129+
// while we still have access to the network ID, otherwise the log message will be less useful.
130+
if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
127131
{
128-
// This is the same check and log message that happens inside OnDespawnObject, but we have to do it here
129-
// while we still have access to the network ID, otherwise the log message will be less useful.
130-
Debug.LogWarning($"Trying to destroy object {networkId} but it doesn't seem to exist anymore!");
131-
return;
132+
NetworkLog.LogWarning($"Trying to destroy {nameof(NetworkObject)} #{networkObjectId} but it does not exist in {nameof(NetworkSpawnManager.SpawnedObjects)} anymore!");
132133
}
133-
m_NetworkManager.NetworkMetrics.TrackObjectDestroyReceived(clientId, networkId, networkObject.name, stream.Length);
134-
NetworkManager.SpawnManager.OnDespawnObject(networkObject, true);
134+
135+
return;
135136
}
137+
138+
m_NetworkManager.NetworkMetrics.TrackObjectDestroyReceived(clientId, networkObjectId, networkObject.name, stream.Length);
139+
NetworkManager.SpawnManager.OnDespawnObject(networkObject, true);
136140
}
137141

138142
/// <summary>
@@ -145,31 +149,37 @@ public void HandleSceneEvent(ulong clientId, Stream stream)
145149
NetworkManager.SceneManager.HandleSceneEvent(clientId, stream);
146150
}
147151

148-
149152
public void HandleChangeOwner(ulong clientId, Stream stream)
150153
{
151-
using (var reader = PooledNetworkReader.Get(stream))
152-
{
153-
ulong networkId = reader.ReadUInt64Packed();
154-
ulong ownerClientId = reader.ReadUInt64Packed();
154+
using var reader = PooledNetworkReader.Get(stream);
155+
ulong networkObjectId = reader.ReadUInt64Packed();
156+
ulong ownerClientId = reader.ReadUInt64Packed();
155157

156-
var networkObject = NetworkManager.SpawnManager.SpawnedObjects[networkId];
157-
if (networkObject.OwnerClientId == NetworkManager.LocalClientId)
158+
if (!NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(networkObjectId, out var networkObject))
159+
{
160+
if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
158161
{
159-
//We are current owner.
160-
networkObject.InvokeBehaviourOnLostOwnership();
162+
NetworkLog.LogWarning($"Trying to handle owner change but {nameof(NetworkObject)} #{networkObjectId} does not exist in {nameof(NetworkSpawnManager.SpawnedObjects)} anymore!");
161163
}
162164

163-
if (ownerClientId == NetworkManager.LocalClientId)
164-
{
165-
//We are new owner.
166-
networkObject.InvokeBehaviourOnGainedOwnership();
167-
}
165+
return;
166+
}
168167

169-
networkObject.OwnerClientId = ownerClientId;
168+
if (networkObject.OwnerClientId == NetworkManager.LocalClientId)
169+
{
170+
//We are current owner.
171+
networkObject.InvokeBehaviourOnLostOwnership();
172+
}
173+
174+
networkObject.OwnerClientId = ownerClientId;
170175

171-
NetworkManager.NetworkMetrics.TrackOwnershipChangeReceived(clientId, networkObject.NetworkObjectId, networkObject.name, stream.Length);
176+
if (ownerClientId == NetworkManager.LocalClientId)
177+
{
178+
//We are new owner.
179+
networkObject.InvokeBehaviourOnGainedOwnership();
172180
}
181+
182+
NetworkManager.NetworkMetrics.TrackOwnershipChangeReceived(clientId, networkObject.NetworkObjectId, networkObject.name, stream.Length);
173183
}
174184

175185
public void HandleTimeSync(ulong clientId, Stream stream)
Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
using System.Collections;
2+
using NUnit.Framework;
3+
using UnityEngine;
4+
using UnityEngine.TestTools;
5+
6+
namespace Unity.Netcode.RuntimeTests
7+
{
8+
public class NetworkObjectOwnershipComponent : NetworkBehaviour
9+
{
10+
public bool OnLostOwnershipFired = false;
11+
public ulong CachedOwnerIdOnLostOwnership = 0;
12+
13+
public override void OnLostOwnership()
14+
{
15+
OnLostOwnershipFired = true;
16+
CachedOwnerIdOnLostOwnership = OwnerClientId;
17+
}
18+
19+
public bool OnGainedOwnershipFired = false;
20+
public ulong CachedOwnerIdOnGainedOwnership = 0;
21+
22+
public override void OnGainedOwnership()
23+
{
24+
OnGainedOwnershipFired = true;
25+
CachedOwnerIdOnGainedOwnership = OwnerClientId;
26+
}
27+
}
28+
29+
[TestFixture(true)]
30+
[TestFixture(false)]
31+
public class NetworkObjectOwnershipTests
32+
{
33+
private const int k_ClientInstanceCount = 1;
34+
35+
private NetworkManager m_ServerNetworkManager;
36+
private NetworkManager[] m_ClientNetworkManagers;
37+
38+
private GameObject m_DummyPrefab;
39+
private GameObject m_DummyGameObject;
40+
41+
private readonly bool m_IsHost;
42+
43+
public NetworkObjectOwnershipTests(bool isHost)
44+
{
45+
m_IsHost = isHost;
46+
}
47+
48+
[UnitySetUp]
49+
public IEnumerator Setup()
50+
{
51+
// we need at least 1 client for tests
52+
Assert.That(k_ClientInstanceCount, Is.GreaterThan(0));
53+
54+
// create NetworkManager instances
55+
Assert.That(MultiInstanceHelpers.Create(k_ClientInstanceCount, out m_ServerNetworkManager, out m_ClientNetworkManagers));
56+
Assert.That(m_ServerNetworkManager, Is.Not.Null);
57+
Assert.That(m_ClientNetworkManagers, Is.Not.Null);
58+
Assert.That(m_ClientNetworkManagers.Length, Is.EqualTo(k_ClientInstanceCount));
59+
60+
// create and register our ad-hoc DummyPrefab (we'll spawn it later during tests)
61+
m_DummyPrefab = new GameObject("DummyPrefabPrototype");
62+
m_DummyPrefab.AddComponent<NetworkObject>();
63+
m_DummyPrefab.AddComponent<NetworkObjectOwnershipComponent>();
64+
MultiInstanceHelpers.MakeNetworkedObjectTestPrefab(m_DummyPrefab.GetComponent<NetworkObject>());
65+
m_ServerNetworkManager.NetworkConfig.NetworkPrefabs.Add(new NetworkPrefab { Prefab = m_DummyPrefab });
66+
foreach (var clientNetworkManager in m_ClientNetworkManagers)
67+
{
68+
clientNetworkManager.NetworkConfig.NetworkPrefabs.Add(new NetworkPrefab { Prefab = m_DummyPrefab });
69+
}
70+
71+
// start server and client NetworkManager instances
72+
Assert.That(MultiInstanceHelpers.Start(m_IsHost, m_ServerNetworkManager, m_ClientNetworkManagers));
73+
74+
// wait for connection on client side
75+
yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientsConnected(m_ClientNetworkManagers));
76+
77+
// wait for connection on server side
78+
yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientConnectedToServer(m_ServerNetworkManager));
79+
}
80+
81+
[TearDown]
82+
public void Teardown()
83+
{
84+
MultiInstanceHelpers.Destroy();
85+
86+
if (m_DummyGameObject != null)
87+
{
88+
Object.DestroyImmediate(m_DummyGameObject);
89+
}
90+
91+
if (m_DummyPrefab != null)
92+
{
93+
Object.DestroyImmediate(m_DummyPrefab);
94+
}
95+
}
96+
97+
[UnityTest]
98+
public IEnumerator TestOwnershipCallbacks()
99+
{
100+
m_DummyGameObject = Object.Instantiate(m_DummyPrefab);
101+
var dummyNetworkObject = m_DummyGameObject.GetComponent<NetworkObject>();
102+
Assert.That(dummyNetworkObject, Is.Not.Null);
103+
104+
dummyNetworkObject.NetworkManagerOwner = m_ServerNetworkManager;
105+
dummyNetworkObject.Spawn();
106+
var dummyNetworkObjectId = dummyNetworkObject.NetworkObjectId;
107+
Assert.That(dummyNetworkObjectId, Is.GreaterThan(0));
108+
109+
int nextFrameNumber = Time.frameCount + 2;
110+
yield return new WaitUntil(() => Time.frameCount >= nextFrameNumber);
111+
112+
Assert.That(m_ServerNetworkManager.SpawnManager.SpawnedObjects.ContainsKey(dummyNetworkObjectId));
113+
foreach (var clientNetworkManager in m_ClientNetworkManagers)
114+
{
115+
Assert.That(clientNetworkManager.SpawnManager.SpawnedObjects.ContainsKey(dummyNetworkObjectId));
116+
}
117+
118+
119+
var serverObject = m_ServerNetworkManager.SpawnManager.SpawnedObjects[dummyNetworkObjectId];
120+
var clientObject = m_ClientNetworkManagers[0].SpawnManager.SpawnedObjects[dummyNetworkObjectId];
121+
Assert.That(serverObject, Is.Not.Null);
122+
Assert.That(clientObject, Is.Not.Null);
123+
124+
var serverComponent = serverObject.GetComponent<NetworkObjectOwnershipComponent>();
125+
var clientComponent = clientObject.GetComponent<NetworkObjectOwnershipComponent>();
126+
Assert.That(serverComponent, Is.Not.Null);
127+
Assert.That(clientComponent, Is.Not.Null);
128+
129+
130+
Assert.That(serverObject.OwnerClientId, Is.EqualTo(m_ServerNetworkManager.ServerClientId));
131+
Assert.That(clientObject.OwnerClientId, Is.EqualTo(m_ClientNetworkManagers[0].ServerClientId));
132+
133+
Assert.That(m_ServerNetworkManager.ConnectedClients.ContainsKey(m_ClientNetworkManagers[0].LocalClientId));
134+
serverObject.ChangeOwnership(m_ClientNetworkManagers[0].LocalClientId);
135+
136+
nextFrameNumber = Time.frameCount + 2;
137+
yield return new WaitUntil(() => Time.frameCount >= nextFrameNumber);
138+
139+
140+
Assert.That(clientComponent.OnGainedOwnershipFired);
141+
Assert.That(clientComponent.CachedOwnerIdOnGainedOwnership, Is.EqualTo(m_ClientNetworkManagers[0].LocalClientId));
142+
serverObject.ChangeOwnership(m_ServerNetworkManager.ServerClientId);
143+
144+
nextFrameNumber = Time.frameCount + 2;
145+
yield return new WaitUntil(() => Time.frameCount >= nextFrameNumber);
146+
147+
148+
Assert.That(serverObject.OwnerClientId, Is.EqualTo(m_ServerNetworkManager.LocalClientId));
149+
Assert.That(clientComponent.OnLostOwnershipFired);
150+
Assert.That(clientComponent.CachedOwnerIdOnLostOwnership, Is.EqualTo(m_ClientNetworkManagers[0].LocalClientId));
151+
}
152+
}
153+
}

com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.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.

0 commit comments

Comments
 (0)