Skip to content

Commit 7077860

Browse files
EmandMEmandM
committed
Fix up SpawnManager tests
1 parent ae22678 commit 7077860

3 files changed

Lines changed: 136 additions & 150 deletions

File tree

‎com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1343,7 +1343,14 @@ internal void OnClientDisconnectFromServer(ulong clientId)
13431343
}
13441344
else if (!NetworkManager.ShutdownInProgress)
13451345
{
1346-
playerObject.RemoveOwnership();
1346+
if (NetworkManager.DistributedAuthorityMode)
1347+
{
1348+
NetworkManager.SpawnManager.ChangeOwnership(playerObject, NetworkManager.LocalClientId, true);
1349+
}
1350+
else
1351+
{
1352+
playerObject.RemoveOwnership();
1353+
}
13471354
}
13481355
}
13491356

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

Lines changed: 34 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System.Linq;
55
using System.Runtime.CompilerServices;
66
using System.Text;
7+
using Unity.Netcode.Logging;
78
using UnityEngine;
89
using Object = UnityEngine.Object;
910

@@ -157,18 +158,34 @@ private void RemovePlayerObject(NetworkObject playerObject, bool destroyingObjec
157158
}
158159
playerObject.IsPlayerObject = false;
159160
m_PlayerObjects.Remove(playerObject);
160-
if (m_PlayerObjectsTable.ContainsKey(playerObject.OwnerClientId))
161+
162+
var originalOwner = playerObject.OwnerClientId;
163+
164+
// Try on the current owner's table
165+
if (m_PlayerObjectsTable.TryGetValue(playerObject.OwnerClientId, out var ownerTable) && ownerTable.Remove(playerObject))
161166
{
162-
m_PlayerObjectsTable[playerObject.OwnerClientId].Remove(playerObject);
163-
if (m_PlayerObjectsTable[playerObject.OwnerClientId].Count == 0)
167+
if (ownerTable.Count == 0)
164168
{
165169
m_PlayerObjectsTable.Remove(playerObject.OwnerClientId);
166170
}
167171
}
168-
// If the client exists locally and we are destroying...
169-
if (NetworkManager.ConnectionManager.ConnectedClients.ContainsKey(playerObject.OwnerClientId) && destroyingObject)
172+
// If the object wasn't removed from the owner's list, we need to check on all lists
173+
// The ownership could have changed since it was created
174+
else
175+
{
176+
foreach (var (owner, playerObjects) in m_PlayerObjectsTable)
177+
{
178+
if (playerObjects.Remove(playerObject))
179+
{
180+
originalOwner = owner;
181+
break;
182+
}
183+
}
184+
}
185+
186+
// If the client exists locally, and we are destroying...
187+
if (destroyingObject && NetworkManager.ConnectionManager.ConnectedClients.TryGetValue(originalOwner, out var client))
170188
{
171-
var client = NetworkManager.ConnectionManager.ConnectedClients[playerObject.OwnerClientId];
172189
// and the client's currently assigned player object is what is being destroyed...
173190
if (client != null && client.PlayerObject == playerObject)
174191
{
@@ -378,33 +395,25 @@ public List<NetworkObject> GetPlayerNetworkObjects(ulong clientId)
378395
}
379396

380397
/// <summary>
381-
/// Returns the player object with a given clientId or null if one does not exist. This is only valid server side.
398+
/// Returns the player object with a given clientId or null if one does not exist.
382399
/// </summary>
400+
/// <remarks>
401+
/// In client-server only the server can get other player's player objects.
402+
/// </remarks>
383403
/// <param name="clientId">the client identifier of the player</param>
384404
/// <returns>The player object with a given clientId or null if one does not exist</returns>
385405
public NetworkObject GetPlayerNetworkObject(ulong clientId)
386406
{
387-
if (!NetworkManager.DistributedAuthorityMode)
407+
// Only the server can get other player's player objects in client-server mode
408+
if (!NetworkManager.DistributedAuthorityMode && !NetworkManager.IsServer && NetworkManager.LocalClientId != clientId)
388409
{
389-
if (!NetworkManager.IsServer && NetworkManager.LocalClientId != clientId)
390-
{
391-
if (NetworkManager.LogLevel <= LogLevel.Error)
392-
{
393-
NetworkLog.LogErrorServer($"{clientId} Only the server can find player objects from other clients.");
394-
}
395-
return null;
396-
}
397-
if (TryGetNetworkClient(clientId, out NetworkClient networkClient))
398-
{
399-
return networkClient.PlayerObject;
400-
}
410+
NetworkManager.Log.Error(new Context(LogLevel.Error, "Only the server can find player objects from other clients."));
411+
return null;
401412
}
402-
else
413+
414+
if (m_PlayerObjectsTable.TryGetValue(clientId, out var playerObjects))
403415
{
404-
if (m_PlayerObjectsTable.ContainsKey(clientId))
405-
{
406-
return m_PlayerObjectsTable[clientId].First();
407-
}
416+
return playerObjects.Count > 0 ? playerObjects[0] : null;
408417
}
409418
return null;
410419
}

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

Lines changed: 94 additions & 124 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System.Collections;
2+
using System.Text.RegularExpressions;
23
using NUnit.Framework;
34
using Unity.Netcode.TestHelpers.Runtime;
45
using UnityEngine.TestTools;
@@ -9,164 +10,133 @@ namespace Unity.Netcode.RuntimeTests
910
[TestFixture(HostOrServer.Host)]
1011
internal class NetworkSpawnManagerTests : NetcodeIntegrationTest
1112
{
12-
private ulong serverSideClientId => NetworkManager.ServerClientId;
13-
private ulong clientSideClientId => m_ClientNetworkManagers[0].LocalClientId;
14-
private ulong otherClientSideClientId => m_ClientNetworkManagers[1].LocalClientId;
15-
1613
protected override int NumberOfClients => 2;
1714

18-
// TODO: [CmbServiceTests] Adapt to run with the service. Combine server and client tests
19-
protected override bool UseCMBService()
20-
{
21-
return false;
22-
}
23-
2415
public NetworkSpawnManagerTests(HostOrServer hostOrServer) : base(hostOrServer) { }
2516

2617
[Test]
27-
public void TestServerCanAccessItsOwnPlayer()
28-
{
29-
// server can access its own player
30-
var serverSideServerPlayerObject = m_ServerNetworkManager.SpawnManager.GetPlayerNetworkObject(serverSideClientId);
31-
Assert.NotNull(serverSideServerPlayerObject);
32-
Assert.AreEqual(serverSideClientId, serverSideServerPlayerObject.OwnerClientId);
33-
}
34-
35-
36-
/// <summary>
37-
/// Test was converted from a Test to UnityTest so distributed authority mode will pass this test.
38-
/// In distributed authority mode, client-side player spawning is enabled by default which requires
39-
/// all client (including DAHost) instances to wait for all players to be spawned.
40-
/// </summary>
41-
[UnityTest]
42-
public IEnumerator TestServerCanAccessOtherPlayers()
43-
{
44-
yield return null;
45-
// server can access other players
46-
var serverSideClientPlayerObject = m_ServerNetworkManager.SpawnManager.GetPlayerNetworkObject(clientSideClientId);
47-
Assert.NotNull(serverSideClientPlayerObject);
48-
Assert.AreEqual(clientSideClientId, serverSideClientPlayerObject.OwnerClientId);
49-
50-
var serverSideOtherClientPlayerObject = m_ServerNetworkManager.SpawnManager.GetPlayerNetworkObject(otherClientSideClientId);
51-
Assert.NotNull(serverSideOtherClientPlayerObject);
52-
Assert.AreEqual(otherClientSideClientId, serverSideOtherClientPlayerObject.OwnerClientId);
53-
}
54-
55-
[Test]
56-
public void TestClientCantAccessServerPlayer()
18+
public void TestGetPlayerNetworkObject()
5719
{
58-
if (m_DistributedAuthority)
20+
foreach (var toTest in m_NetworkManagers)
5921
{
60-
VerboseDebug($"Ignoring test: Clients have access to other player objects in {m_NetworkTopologyType} mode.");
61-
return;
22+
foreach (var toGet in m_NetworkManagers)
23+
{
24+
// GetPlayerNetworkObject should only be able to get the object
25+
// - When using DA
26+
// - When testing the Server NetworkManager
27+
// - And when the client is getting their own PlayerObject.
28+
var canFetch = m_DistributedAuthority || toTest.IsServer || toTest == toGet;
29+
30+
if (!canFetch)
31+
{
32+
LogAssert.Expect(UnityEngine.LogType.Error, new Regex("Only the server can find player objects from other clients."));
33+
}
34+
35+
var playerObject = toTest.SpawnManager.GetPlayerNetworkObject(toGet.LocalClientId);
36+
37+
if (canFetch)
38+
{
39+
Assert.That(playerObject, Is.Not.Null);
40+
Assert.That(toGet.LocalClientId, Is.EqualTo(playerObject.OwnerClientId));
41+
}
42+
else
43+
{
44+
Assert.That(playerObject, Is.Null);
45+
}
46+
}
6247
}
63-
// client can't access server player
64-
string expectedLog = $"[Netcode-Server Sender=0] {serverSideClientId} Only the server can find player objects from other clients.";
65-
LogAssert.Expect(UnityEngine.LogType.Error, expectedLog);
66-
m_ClientNetworkManagers[0].SpawnManager.GetPlayerNetworkObject(serverSideClientId);
67-
}
6848

69-
[Test]
70-
public void TestClientCanAccessOwnPlayer()
71-
{
72-
// client can access own player
73-
var clientSideClientPlayerObject = m_ClientNetworkManagers[0].SpawnManager.GetPlayerNetworkObject(clientSideClientId);
74-
Assert.NotNull(clientSideClientPlayerObject);
75-
Assert.AreEqual(clientSideClientId, clientSideClientPlayerObject.OwnerClientId);
49+
// finally, test that an invalid clientId returns a null object
50+
var invalid = GetAuthorityNetworkManager().SpawnManager.GetPlayerNetworkObject(9999);
51+
Assert.That(invalid, Is.Null);
7652
}
7753

7854
[Test]
79-
public void TestClientCanAccessOtherPlayer()
55+
public void TestGetLocalPlayerObject()
8056
{
81-
82-
if (!m_DistributedAuthority)
57+
foreach (var manager in m_NetworkManagers)
8358
{
84-
VerboseDebug($"Ignoring test: Clients do not have access to other player objects in {m_NetworkTopologyType} mode.");
85-
return;
59+
var playerObject = manager.SpawnManager.GetLocalPlayerObject();
60+
Assert.That(playerObject, Is.Not.Null);
61+
Assert.That(manager.LocalClientId, Is.EqualTo(playerObject.OwnerClientId));
62+
Assert.That(manager.LocalClient.PlayerObject, Is.EqualTo(playerObject));
8663
}
87-
88-
var otherClientPlayer = m_ClientNetworkManagers[0].SpawnManager.GetPlayerNetworkObject(otherClientSideClientId);
89-
Assert.NotNull(otherClientPlayer, $"Failed to obtain Client{otherClientSideClientId}'s player object!");
9064
}
9165

92-
[Test]
93-
public void TestClientCantAccessOtherPlayer()
66+
public enum DestroyWithOwner
9467
{
95-
if (m_DistributedAuthority)
96-
{
97-
VerboseDebug($"Ignoring test: Clients have access to other player objects in {m_NetworkTopologyType} mode.");
98-
return;
99-
}
100-
101-
// client can't access other player
102-
string expectedLog = $"[Netcode-Server Sender=0] {otherClientSideClientId} Only the server can find player objects from other clients.";
103-
LogAssert.Expect(UnityEngine.LogType.Error, expectedLog);
104-
m_ClientNetworkManagers[0].SpawnManager.GetPlayerNetworkObject(otherClientSideClientId);
68+
DestroyWithOwner,
69+
DontDestroyWithOwner
10570
}
10671

107-
[Test]
108-
public void TestServerGetsNullValueIfInvalidId()
109-
{
110-
// server gets null value if invalid id
111-
var nullPlayer = m_ServerNetworkManager.SpawnManager.GetPlayerNetworkObject(9999);
112-
Assert.Null(nullPlayer);
113-
}
114-
115-
[Test]
116-
public void TestServerCanUseGetLocalPlayerObject()
117-
{
118-
// test server can use GetLocalPlayerObject
119-
var serverSideServerPlayerObject = m_ServerNetworkManager.SpawnManager.GetLocalPlayerObject();
120-
Assert.NotNull(serverSideServerPlayerObject);
121-
Assert.AreEqual(serverSideClientId, serverSideServerPlayerObject.OwnerClientId);
122-
}
123-
124-
[Test]
125-
public void TestClientCanUseGetLocalPlayerObject()
72+
[UnityTest]
73+
public IEnumerator TestPlayerPrefabConnectAndDisconnect([Values] DestroyWithOwner destroySetting)
12674
{
127-
// test client can use GetLocalPlayerObject
128-
var clientSideClientPlayerObject = m_ClientNetworkManagers[0].SpawnManager.GetLocalPlayerObject();
129-
Assert.NotNull(clientSideClientPlayerObject);
130-
Assert.AreEqual(clientSideClientId, clientSideClientPlayerObject.OwnerClientId);
131-
}
75+
var destroyWithOwner = destroySetting == DestroyWithOwner.DestroyWithOwner;
76+
var authority = GetAuthorityNetworkManager();
77+
// Regression test: Ensure the authority's player object is set
78+
Assert.That(authority.LocalClient.PlayerObject != null, Is.True, "The server should have a player object!");
13279

133-
private bool m_ClientDisconnected;
80+
// Mark PlayerPrefab as DontDestroyWithOwner
81+
m_PlayerPrefab.GetComponent<NetworkObject>().DontDestroyWithOwner = !destroyWithOwner;
13482

135-
[UnityTest]
136-
public IEnumerator TestConnectAndDisconnect()
137-
{
13883
// test when client connects, player object is now available
139-
yield return CreateAndStartNewClient();
140-
var newClientNetworkManager = m_ClientNetworkManagers[NumberOfClients];
141-
var newClientLocalClientId = newClientNetworkManager.LocalClientId;
84+
var newClient = CreateNewClient();
85+
yield return StartClient(newClient);
86+
var newClientId = newClient.LocalClientId;
14287

14388
// test new client can get that itself locally
144-
var newPlayerObject = newClientNetworkManager.SpawnManager.GetLocalPlayerObject();
89+
var newPlayerObject = newClient.SpawnManager.GetLocalPlayerObject();
14590
Assert.NotNull(newPlayerObject);
146-
Assert.AreEqual(newClientLocalClientId, newPlayerObject.OwnerClientId);
91+
Assert.AreEqual(newClientId, newPlayerObject.OwnerClientId);
92+
14793
// test server can get that new client locally
148-
var serverSideNewClientPlayer = m_ServerNetworkManager.SpawnManager.GetPlayerNetworkObject(newClientLocalClientId);
94+
var serverSideNewClientPlayer = authority.SpawnManager.GetPlayerNetworkObject(newClientId);
14995
Assert.NotNull(serverSideNewClientPlayer);
150-
Assert.AreEqual(newClientLocalClientId, serverSideNewClientPlayer.OwnerClientId);
96+
Assert.AreEqual(newClientId, serverSideNewClientPlayer.OwnerClientId);
15197

15298
// test when client disconnects, player object no longer available.
153-
var nbConnectedClients = m_ServerNetworkManager.ConnectedClients.Count;
154-
m_ClientDisconnected = false;
155-
newClientNetworkManager.OnClientDisconnectCallback += ClientNetworkManager_OnClientDisconnectCallback;
156-
m_ServerNetworkManager.DisconnectClient(newClientLocalClientId);
157-
yield return WaitForConditionOrTimeOut(() => m_ClientDisconnected);
158-
Assert.IsFalse(s_GlobalTimeoutHelper.TimedOut, "Timed out waiting for client to disconnect");
99+
var nbConnectedClients = authority.ConnectedClients.Count;
100+
authority.DisconnectClient(newClientId);
101+
102+
yield return WaitForConditionOrTimeOut(() => !newClient.IsConnectedClient);
103+
AssertOnTimeout("Timed out waiting for client to disconnect");
104+
159105
// Call this to clean up NetcodeIntegrationTestHelpers
160-
NetcodeIntegrationTestHelpers.StopOneClient(newClientNetworkManager);
106+
yield return StopOneClient(newClient);
161107

162-
Assert.AreEqual(m_ServerNetworkManager.ConnectedClients.Count, nbConnectedClients - 1);
163-
serverSideNewClientPlayer = m_ServerNetworkManager.SpawnManager.GetPlayerNetworkObject(newClientLocalClientId);
164-
Assert.Null(serverSideNewClientPlayer);
165-
}
108+
Assert.AreEqual(authority.ConnectedClients.Count, nbConnectedClients - 1);
109+
Assert.True(newPlayerObject == null, "The client's player object should have been destroyed!");
166110

167-
private void ClientNetworkManager_OnClientDisconnectCallback(ulong obj)
168-
{
169-
m_ClientDisconnected = true;
111+
if (destroyWithOwner)
112+
{
113+
Assert.That(serverSideNewClientPlayer == null, Is.True, "The server's version of the client's player object should have been destroyed");
114+
}
115+
else
116+
{
117+
Assert.That(serverSideNewClientPlayer == null, Is.False, "The server's version of the client's player object shouldn't have been destroyed!");
118+
Assert.That(serverSideNewClientPlayer.OwnerClientId, Is.EqualTo(authority.LocalClientId), "Ownership should have transferred to the authority!");
119+
Assert.That(serverSideNewClientPlayer.IsPlayerObject, Is.True, $"{nameof(NetworkObject.IsPlayerObject)} should still be set!");
120+
121+
// Requesting the player's object after they've left should still while the object hasn't been destroyed
122+
var playerObject = authority.SpawnManager.GetPlayerNetworkObject(newClientId);
123+
Assert.That(playerObject != null, Is.True, "The authority should still be able to get the player object after the client has disconnected");
124+
125+
// Despawn and destroy the player object
126+
playerObject.Despawn();
127+
128+
// Check that now the player object is null
129+
yield return WaitForConditionOrTimeOut(() => playerObject == null);
130+
AssertOnTimeout("Timed out waiting for the object to be destroyed.");
131+
132+
// Regression test:
133+
// check that the authority's player object isn't destroyed
134+
Assert.That(authority.LocalClient.PlayerObject != null, Is.True, "The server's player object should not have been destroyed!");
135+
}
136+
137+
// sanity check that requesting the object from the client who left after the object was destroyed is now null
138+
var sanity = authority.SpawnManager.GetPlayerNetworkObject(newClientId);
139+
Assert.Null(sanity, $"{nameof(NetworkSpawnManager.GetPlayerNetworkObject)} shouldn't be able to get the player object after the client has disconnected!");
170140
}
171141
}
172142
}

0 commit comments

Comments
 (0)