Skip to content

Commit b64117e

Browse files
fix: Player removed from observers when player object despawns (#3110)
* fix Remove legacy observer removal code. * update adding change log entry. * fix Assure owner is assigned as an observer to the player. Make sure observers are not removed from despawned players. * fix Make sure to take spawn with observers into account when assigning the OwnerClientId as an observer. * test adding validation test for the fix. * update Adding change log PR numbers to the two entries. (left one out in PR-3108) * test-fix Fixing weird issue with the PlayerSpawnDespawn and ghost NetworkManagers.
1 parent c696b58 commit b64117e

File tree

4 files changed

+179
-33
lines changed

4 files changed

+179
-33
lines changed

com.unity.netcode.gameobjects/CHANGELOG.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
1212

1313
### Fixed
1414

15-
- Fixed issue where `NetworkAnimator` would statically allocate write buffer space for `Animator` parameters that could cause a write error if the number of parameters exceeded the space allocated.
15+
- Fixed issue where client is removed as an observer from spawned objects when their player instance is despawned. (#3110)
16+
- Fixed issue where `NetworkAnimator` would statically allocate write buffer space for `Animator` parameters that could cause a write error if the number of parameters exceeded the space allocated. (#3108)
1617

1718
### Changed
1819

com.unity.netcode.gameobjects/Runtime/Components/NetworkAnimator.cs

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -306,10 +306,6 @@ private void ParseStateMachineStates(int layerIndex, ref AnimatorController anim
306306
private void BuildTransitionStateInfoList()
307307
{
308308
#if UNITY_EDITOR
309-
if (UnityEditor.EditorApplication.isUpdating || UnityEditor.EditorApplication.isPlayingOrWillChangePlaymode)
310-
{
311-
return;
312-
}
313309
if (m_Animator == null)
314310
{
315311
return;
@@ -1268,10 +1264,11 @@ internal void UpdateAnimationState(AnimationState animationState)
12681264
NetworkLog.LogError($"[DestinationState To Transition Info] Layer ({animationState.Layer}) sub-table does not contain destination state ({animationState.DestinationStateHash})!");
12691265
}
12701266
}
1271-
else if (NetworkManager.LogLevel == LogLevel.Developer)
1272-
{
1273-
NetworkLog.LogError($"[DestinationState To Transition Info] Layer ({animationState.Layer}) does not exist!");
1274-
}
1267+
// For reference, it is valid to have no transition information
1268+
//else if (NetworkManager.LogLevel == LogLevel.Developer)
1269+
//{
1270+
// NetworkLog.LogError($"[DestinationState To Transition Info] Layer ({animationState.Layer}) does not exist!");
1271+
//}
12751272
}
12761273
else if (animationState.Transition && animationState.CrossFade)
12771274
{

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

Lines changed: 23 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,13 @@ private void AddPlayerObject(NetworkObject playerObject)
8888
}
8989
}
9090

91+
// Only if spawn with observers is set or we are using a distributed authority network topology and this is the client's player should we add
92+
// the owner as an observer.
93+
if (playerObject.SpawnWithObservers || (NetworkManager.DistributedAuthorityMode && NetworkManager.LocalClientId == playerObject.OwnerClientId))
94+
{
95+
playerObject.Observers.Add(playerObject.OwnerClientId);
96+
}
97+
9198
m_PlayerObjects.Add(playerObject);
9299
if (!m_PlayerObjectsTable.ContainsKey(playerObject.OwnerClientId))
93100
{
@@ -110,8 +117,9 @@ internal void UpdateNetworkClientPlayer(NetworkObject playerObject)
110117
if (playerNetworkClient.PlayerObject != null && m_PlayerObjects.Contains(playerNetworkClient.PlayerObject))
111118
{
112119
// Just remove the previous player object but keep the assigned observers of the NetworkObject
113-
RemovePlayerObject(playerNetworkClient.PlayerObject, true);
120+
RemovePlayerObject(playerNetworkClient.PlayerObject);
114121
}
122+
115123
// Now update the associated NetworkClient's player object
116124
NetworkManager.ConnectionManager.ConnectedClients[playerObject.OwnerClientId].AssignPlayerObject(ref playerObject);
117125
AddPlayerObject(playerObject);
@@ -120,7 +128,7 @@ internal void UpdateNetworkClientPlayer(NetworkObject playerObject)
120128
/// <summary>
121129
/// Removes a player object and updates all other players' observers list
122130
/// </summary>
123-
private void RemovePlayerObject(NetworkObject playerObject, bool keepObservers = false)
131+
private void RemovePlayerObject(NetworkObject playerObject, bool destroyingObject = false)
124132
{
125133
if (!playerObject.IsPlayerObject)
126134
{
@@ -141,16 +149,21 @@ private void RemovePlayerObject(NetworkObject playerObject, bool keepObservers =
141149
}
142150
}
143151

144-
// If we want to keep the observers, then exit early
145-
if (keepObservers)
152+
if (NetworkManager.ConnectionManager.ConnectedClients.ContainsKey(playerObject.OwnerClientId) && destroyingObject)
146153
{
147-
return;
154+
NetworkManager.ConnectionManager.ConnectedClients[playerObject.OwnerClientId].PlayerObject = null;
148155
}
149156

150-
foreach (var player in m_PlayerObjects)
151-
{
152-
player.Observers.Remove(playerObject.OwnerClientId);
153-
}
157+
// If we want to keep the observers, then exit early
158+
//if (keepObservers)
159+
//{
160+
// return;
161+
//}
162+
163+
//foreach (var player in m_PlayerObjects)
164+
//{
165+
// player.Observers.Remove(playerObject.OwnerClientId);
166+
//}
154167
}
155168

156169
internal void MarkObjectForShowingTo(NetworkObject networkObject, ulong clientId)
@@ -1550,23 +1563,9 @@ internal void OnDespawnObject(NetworkObject networkObject, bool destroyGameObjec
15501563
SpawnedObjectsList.Remove(networkObject);
15511564
}
15521565

1553-
// DANGO-TODO: When we fix the issue with observers not being applied to NetworkObjects,
1554-
// (client connect/disconnect) we can remove this hacky way of doing this.
1555-
// Basically, when a player disconnects and/or is destroyed they are removed as an observer from all other client
1556-
// NetworkOject instances.
1557-
if (networkObject.IsPlayerObject && !networkObject.IsOwner && networkObject.OwnerClientId != NetworkManager.LocalClientId)
1558-
{
1559-
foreach (var netObject in SpawnedObjects)
1560-
{
1561-
if (netObject.Value.Observers.Contains(networkObject.OwnerClientId))
1562-
{
1563-
netObject.Value.Observers.Remove(networkObject.OwnerClientId);
1564-
}
1565-
}
1566-
}
15671566
if (networkObject.IsPlayerObject)
15681567
{
1569-
RemovePlayerObject(networkObject);
1568+
RemovePlayerObject(networkObject, destroyGameObject);
15701569
}
15711570

15721571
// Always clear out the observers list when despawned

com.unity.netcode.gameobjects/Tests/Runtime/PlayerObjectTests.cs

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
using System.Collections;
2+
using System.Collections.Generic;
3+
using System.Linq;
4+
using System.Text;
25
using NUnit.Framework;
36
using Unity.Netcode.TestHelpers.Runtime;
47
using UnityEngine;
@@ -184,4 +187,150 @@ public IEnumerator PlayerSpawnPosition()
184187
}
185188
}
186189
}
190+
191+
/// <summary>
192+
/// This test validates that when a player is spawned it has all observers
193+
/// properly set and the spawned and player object lists are properly populated.
194+
/// It also validates that when a player is despawned and the client is still connected
195+
/// that the client maintains its observers for other players.
196+
/// </summary>
197+
[TestFixture(HostOrServer.DAHost)]
198+
[TestFixture(HostOrServer.Host)]
199+
[TestFixture(HostOrServer.Server)]
200+
internal class PlayerSpawnAndDespawnTests : NetcodeIntegrationTest
201+
{
202+
protected override int NumberOfClients => 4;
203+
204+
private StringBuilder m_ErrorLog = new StringBuilder();
205+
206+
public PlayerSpawnAndDespawnTests(HostOrServer hostOrServer) : base(hostOrServer) { }
207+
208+
private bool ValidateObservers(ulong playerClientId, NetworkObject player, ref List<NetworkManager> networkManagers)
209+
{
210+
foreach (var networkManager in networkManagers)
211+
{
212+
if (player != null && player.IsSpawned)
213+
{
214+
if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(player.NetworkObjectId))
215+
{
216+
m_ErrorLog.AppendLine($"Client-{networkManager.LocalClientId} does not contain a spawned object entry {nameof(NetworkObject)}-{player.NetworkObjectId} for Client-{playerClientId}!");
217+
return false;
218+
}
219+
220+
var playerClone = networkManager.SpawnManager.SpawnedObjects[player.NetworkObjectId];
221+
foreach (var clientId in networkManager.ConnectedClientsIds)
222+
{
223+
if (!playerClone.IsNetworkVisibleTo(clientId))
224+
{
225+
m_ErrorLog.AppendLine($"Client-{networkManager.LocalClientId} failed visibility check for Client-{clientId} on {nameof(NetworkObject)}-{player.NetworkObjectId} for Client-{playerClientId}!");
226+
return false;
227+
}
228+
}
229+
230+
var foundPlayerClone = (NetworkObject)null;
231+
foreach (var playerObject in networkManager.SpawnManager.PlayerObjects)
232+
{
233+
if (playerObject.OwnerClientId == playerClientId && playerObject.NetworkObjectId == player.NetworkObjectId)
234+
{
235+
foundPlayerClone = playerObject;
236+
break;
237+
}
238+
}
239+
if (!foundPlayerClone)
240+
{
241+
m_ErrorLog.AppendLine($"Client-{networkManager.LocalClientId} does not contain a player entry for {nameof(NetworkObject)}-{player.NetworkObjectId} for Client-{playerClientId}!");
242+
return false;
243+
}
244+
245+
}
246+
else
247+
{
248+
// If the player client in question is despawned, then no NetworkManager instance
249+
// should contain a clone of that (or the client's NetworkManager instance as well)
250+
foreach (var playerClone in networkManager.SpawnManager.PlayerObjects)
251+
{
252+
if (playerClone.OwnerClientId == playerClientId)
253+
{
254+
m_ErrorLog.AppendLine($"Client-{networkManager.LocalClientId} contains a player {nameof(NetworkObject)}-{playerClone.NetworkObjectId} for Client-{playerClientId} when it should be despawned!");
255+
return false;
256+
}
257+
}
258+
}
259+
}
260+
return true;
261+
}
262+
263+
private bool ValidateAllClientPlayerObservers()
264+
{
265+
var networkManagers = new List<NetworkManager>();
266+
267+
if (m_ServerNetworkManager.IsHost)
268+
{
269+
networkManagers.Add(m_ServerNetworkManager);
270+
}
271+
foreach (var networkManager in m_ClientNetworkManagers)
272+
{
273+
networkManagers.Add(networkManager);
274+
}
275+
276+
m_ErrorLog.Clear();
277+
var success = true;
278+
foreach (var networkManager in networkManagers)
279+
{
280+
var spawnedOrNot = networkManager.LocalClient.PlayerObject == null ? "despawned" : "spawned";
281+
m_ErrorLog.AppendLine($"Validating Client-{networkManager.LocalClientId} {spawnedOrNot} player.");
282+
if (networkManager.LocalClient == null)
283+
{
284+
m_ErrorLog.AppendLine($"No {nameof(NetworkClient)} found for Client-{networkManager.LocalClientId}!");
285+
success = false;
286+
break;
287+
}
288+
if (!ValidateObservers(networkManager.LocalClientId, networkManager.LocalClient.PlayerObject, ref networkManagers))
289+
{
290+
m_ErrorLog.AppendLine($"Client-{networkManager.LocalClientId} validation pass failed.");
291+
success = false;
292+
break;
293+
}
294+
}
295+
networkManagers.Clear();
296+
return success;
297+
}
298+
299+
300+
[UnityTest]
301+
public IEnumerator PlayerSpawnDespawn()
302+
{
303+
// Validate all observers are properly set with all players spawned
304+
yield return WaitForConditionOrTimeOut(ValidateAllClientPlayerObservers);
305+
AssertOnTimeout($"First Validation Failed:\n {m_ErrorLog}");
306+
var selectedClient = m_ClientNetworkManagers[Random.Range(0, m_ClientNetworkManagers.Count() - 1)];
307+
var playerSelected = selectedClient.LocalClient.PlayerObject;
308+
309+
if (m_DistributedAuthority)
310+
{
311+
playerSelected.Despawn(false);
312+
}
313+
else
314+
{
315+
m_ServerNetworkManager.SpawnManager.SpawnedObjects[playerSelected.NetworkObjectId].Despawn(true);
316+
}
317+
318+
// Validate all observers are properly set with one of the players despawned
319+
yield return WaitForConditionOrTimeOut(ValidateAllClientPlayerObservers);
320+
AssertOnTimeout($"Second Validation Failed:\n {m_ErrorLog}");
321+
322+
if (m_DistributedAuthority)
323+
{
324+
playerSelected.SpawnAsPlayerObject(selectedClient.LocalClientId, false);
325+
}
326+
else
327+
{
328+
SpawnPlayerObject(m_ServerNetworkManager.NetworkConfig.PlayerPrefab, selectedClient);
329+
}
330+
331+
// Validate all observers are properly set when the client's player is respawned.
332+
yield return WaitForConditionOrTimeOut(ValidateAllClientPlayerObservers);
333+
AssertOnTimeout($"Third Validation Failed:\n {m_ErrorLog}");
334+
}
335+
}
187336
}

0 commit comments

Comments
 (0)