Skip to content

fix: client destroy exception #981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions com.unity.multiplayer.mlapi/Runtime/Core/NetworkObject.cs
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,11 @@ public static void NetworkHide(List<NetworkObject> networkObjects, ulong clientI

private void OnDestroy()
{
if (NetworkManager != null && NetworkManager.IsListening && NetworkManager.IsServer == false && IsSpawned)
{
throw new NotServerException($"Destroy a spawned {nameof(NetworkObject)} on a non-host client is not valid. Call {nameof(Destroy)} or {nameof(Despawn)} on the server/host instead.");
}
Comment on lines +420 to +423
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks safe to me but I'll defer to Noel for the final call.


if (NetworkManager != null && NetworkManager.SpawnManager != null && NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(NetworkObjectId, out var networkObject))
{
NetworkManager.SpawnManager.OnDespawnObject(networkObject, false);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
using System;
using System.Collections;
using MLAPI.Exceptions;
using NUnit.Framework;
using UnityEngine;
using UnityEngine.TestTools;
using Object = UnityEngine.Object;

namespace MLAPI.RuntimeTests
{
/// <summary>
/// Tests calling destroy on spawned / unspawned <see cref="NetworkObject"/>s. Expected behavior:
/// - Server or client destroy on unspawned => Object gets destroyed, no exceptions
/// - Server destroy spawned => Object gets destroyed and despawned/destroyed on all clients. Server does not run <see cref="NetworkPrefaInstanceHandler.HandleNetworkPrefabDestroy"/>. Client runs it.
/// - Client destroy spawned => throw exception.
/// </summary>
public class NetworkObjectDestroyTests : BaseMultiInstanceTest
{
protected override int NbClients => 1;

[UnitySetUp]
public override IEnumerator Setup()
{
yield return StartSomeClientsAndServerWithPlayers(true, NbClients, playerPrefab =>
{
// playerPrefab.AddComponent<TestDestroy>();
});
}

/// <summary>
/// Tests that a server can destroy a NetworkObject and that it gets despawned correctly.
/// </summary>
/// <returns></returns>
[UnityTest]
public IEnumerator TestNetworkObjectServerDestroy()
{
// This is the *SERVER VERSION* of the *CLIENT PLAYER*
var serverClientPlayerResult = new MultiInstanceHelpers.CoroutineResultWrapper<NetworkObject>();
yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.GetNetworkObjectByRepresentation((x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId), m_ServerNetworkManager, serverClientPlayerResult));

// This is the *CLIENT VERSION* of the *CLIENT PLAYER*
var clientClientPlayerResult = new MultiInstanceHelpers.CoroutineResultWrapper<NetworkObject>();
yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.GetNetworkObjectByRepresentation((x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId), m_ClientNetworkManagers[0], clientClientPlayerResult));

Assert.IsNotNull(serverClientPlayerResult.Result.gameObject);
Assert.IsNotNull(clientClientPlayerResult.Result.gameObject);

// destroy the server player
Object.Destroy(serverClientPlayerResult.Result.gameObject);

yield return null;

Assert.IsTrue(serverClientPlayerResult.Result == null); // Assert.IsNull doesn't work here

yield return null; // wait one frame more until we receive on client

Assert.IsTrue(clientClientPlayerResult.Result == null);

// create an unspawned networkobject and destroy it
var go = new GameObject();
go.AddComponent<NetworkObject>();
Object.Destroy(go);

yield return null;
Assert.IsTrue(go == null);
}

/// <summary>
/// Tests that a client cannot destroy a spawned networkobject.
/// </summary>
/// <returns></returns>
[UnityTest]
public IEnumerator TestNetworkObjectClientDestroy()
{
// This is the *SERVER VERSION* of the *CLIENT PLAYER*
var serverClientPlayerResult = new MultiInstanceHelpers.CoroutineResultWrapper<NetworkObject>();
yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.GetNetworkObjectByRepresentation((x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId), m_ServerNetworkManager, serverClientPlayerResult));

// This is the *CLIENT VERSION* of the *CLIENT PLAYER*
var clientClientPlayerResult = new MultiInstanceHelpers.CoroutineResultWrapper<NetworkObject>();
yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.GetNetworkObjectByRepresentation((x => x.IsPlayerObject && x.OwnerClientId == m_ClientNetworkManagers[0].LocalClientId), m_ClientNetworkManagers[0], clientClientPlayerResult));

// destroy the client player, this is not allowed
LogAssert.Expect(LogType.Exception, "NotServerException: Destroy a spawned NetworkObject on a non-host client is not valid. Call Destroy or Despawn on the server/host instead.");
Object.DestroyImmediate(clientClientPlayerResult.Result.gameObject);
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.