Skip to content
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

NGO v1.12.0 & v1.11.0 does not trigger OnNetworkDespawn function on NetworkObjects associated with Disconnecting Clients on the Server side when INetworkPrefabInstanceHandler is used. #3190

Open
Ermelious opened this issue Jan 4, 2025 · 3 comments
Labels
Investigating Issue is currently being investigated stat:awaiting response Status - Awaiting response from author. stat:awaiting triage Status - Awaiting triage from the Netcode team. type:bug Bug Report

Comments

@Ermelious
Copy link

Description

NGO v1.12.0 & v1.11.0 does not trigger OnNetworkDespawn function on NetworkObjects associated with Disconnecting Clients on the Server side when INetworkPrefabInstanceHandler is used.

Reproduce Steps

  1. Use SpawnAsPlayerObject with INetworkPrefabInstanceHandler
  2. Disconnect a Client from the Hosting Server
  3. Monitor the OnNetworkDespawn function on the Player script. It does not execute on the Server when the Client disconnects.

Upgrade to NGO v2.1.1

  1. Repeat the above steps.
  2. Monitor the OnNetworkDespawn function which triggers on the Server now for the player objects associated with the disconnecting client.

It doesn't seem like I'm the only one experiencing this issue (Additionally the Destroy function in INetworkPrefabInstanceHandler is being called twice per NetworkObject, refer to the thread for more details):
https://discussions.unity.com/t/onnetworkdespawn-ondestroy-from-networkbehaviour-does-not-get-executed-on-the-hosting-client-when-using-inetworkprefabinstancehandler/1578473/6

Expected Outcome

OnNetworkDespawn should execute on the Server for NetworkObjects associated with the Disconnecting Clients.

My code:

using System.Collections;
using System.Collections.Generic;
using Unity.Netcode;
using UnityEngine;

public class NetcodeSpawnableHandler : INetworkPrefabInstanceHandler
{
    private readonly SpawnableData spawnableData;

    public NetcodeSpawnableHandler(SpawnableData spawnableData)
    {
        this.spawnableData = spawnableData;
    }

    public void Destroy(NetworkObject networkObject)
    {
        var spawnable = networkObject.GetComponent<Spawnable>();
        if (spawnable != null)
        {
            networkObject.gameObject.SetActive(false);
        }
        else
        {
            Debug.LogWarning("Attempted to despawn an object not managed by SpawnableData.");
        }
    }

    public NetworkObject Instantiate(ulong ownerClientId, Vector3 position, Quaternion rotation)
    {
        Debug.Log("NetcodeSpawnableHandler: " + spawnableData.name);
        var spawnable = spawnableData.Spawn(null, position, rotation);
        return spawnable.GetComponent<NetworkObject>();
    }
}
    public static async Task RegisterSpawnableToNetworkManager(SpawnableData spawnableData)
    {
        await spawnableData.PreloadAssetReference();
        // Add the custom handler for the specified prefab
        NetworkManager.Singleton.PrefabHandler.AddHandler(
            spawnableData.assetReferenceHandle.Result,
            new NetcodeSpawnableHandler(spawnableData)
        );
    }

Screenshots

Environment

  • OS: Windows 11
  • Unity Version: 2023
  • Netcode Version: 1.11.0 and 1.12.0
@Ermelious Ermelious added stat:awaiting triage Status - Awaiting triage from the Netcode team. type:bug Bug Report labels Jan 4, 2025
@NoelStephensUnity NoelStephensUnity added the Investigating Issue is currently being investigated label Jan 6, 2025
@NoelStephensUnity
Copy link
Collaborator

Hi, @Ermelious,

Hmmm... it looks like you are not checking the returned value for the NetworkManager.Singleton.PrefabHandler.AddHandle to make sure the handler registration succeeded. Not knowing what the rest of the code looks like that starts the task could you update your RegisterSpawnableToNetworkManager method to this and let me know what you see in your console output?

    public static async Task RegisterSpawnableToNetworkManager(SpawnableData spawnableData)
    {
        await spawnableData.PreloadAssetReference();
        // Add the custom handler for the specified prefab
        if (!NetworkManager.Singleton.PrefabHandler.AddHandler(
            spawnableData.assetReferenceHandle.Result,
            new NetcodeSpawnableHandler(spawnableData)
        ))
        {
            // Not sure what type spawnableData.assetReferenceHandle.Result is. Assuming it is a GameObject.
            var spawnableNetworkObject = spawnableData.assetReferenceHandle.Result.GetComponent<NetworkObject>();
            Debug.Log($"Failed to register handler for {spawnableData.assetReferenceHandle.Result} with a GlobalObjectId value of {spawnableNetworkObject.PrefabIdHash}!");
        }
    }

@NoelStephensUnity NoelStephensUnity added the stat:awaiting response Status - Awaiting response from author. label Jan 7, 2025
@ezoray
Copy link

ezoray commented Jan 9, 2025

Hi Noel, I was the one who replied to the above thread. I've had a look at this again and I think the reason it looks like OnNetworkDespawn is not called is due to the prefab handler's Destroy method being called before the object is despawned for the disconnecting client (and when the client owns the object). For host owned objects the despawning happens before the call to Destroy. So in the Destroy method if you're returning the object to a pool and make it inactive it doesn't trigger the OnNetworkDespawn call.

This is in 1.12.0, in 2.x the call order is correct but the Destroy method is called twice. I've attached a couple of test scenes which I hope will makes things clearer. In one I use Boss Room's object pool which throws an exception in 2.x as it tries to return the object to the pool twice.

prefabhandlers.zip

@NoelStephensUnity
Copy link
Collaborator

NoelStephensUnity commented Jan 9, 2025

@ezoray
So, I would recommend looking at this example:
ObjectPooling_2022-3.zip
It is configured for 2022.3 using v1.12.0, but you can also upgrade to a recent version of Unity 6 (I used 6000.0.32f1) and NGO v2.2.0 and it works the same there too.

Looking at the code-base in v1.12.0, I am not seeing that same order of operations that you are describing. Within the NetworkSpawnManager OnNetworkDespawn does get invoked before invoking NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy.

Then I started to think that it was when the client disconnected itself but in both the NetworkSpawnManager for v1.x
and in the NetworkSpawnManager for v2.x they check for destroyGameObject and if that is set to false (which is what is passed in when when destroying spawned objects during the shutdown) then this entire block of code is not invoked:

            if (destroyGameObject && gobj != null)
            {
                if (NetworkManager.PrefabHandler.ContainsHandler(networkObject))
                {
                    NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(networkObject);
                }
                else
                {
                    UnityEngine.Object.Destroy(gobj);
                }
            }

For example purposes, while running Unity 6 with NGO v2.2.0 I would connect a client to a running host, hit the space bar to spawn a bunch of NetworkObjects from the pool, then I would disconnect the client (hitting the X button in the top right) and this is what the console log looks like:
Image
You can see that I spawned several things and then when you see it despawning that is when I disconnected the client (from the client side) which that ran through the NetworkObject's OnNetworkDespawn first (i.e. "[2] OnNetworkDespawn on Client-1.") and then the prefabhandler's destroy method was invoked (i.e. Destroy Sphere_29-2).
Where the OnNetworkDespawn used the NetworkObjectId first (contained within [#]) and the prefab handler's destroy method added the NetworkObjectId at the end of the log message.

The example I provided has some additional settings to help you debug various scenarios. The two of interest are "Use Pool For Spawn" (when enabled it pulls from the pool as opposed to instantiating when disabled) and "Return To Pool" (when enabled it returns the objects to the pool as to destroying when disabled).

For example:
Image
This configuration will pull NetworkObjects from the pool but will destroy them when they are despawned. It was the last combination I tried in order to try and replicate the issue using the ObjectPoolSystem. If both "Use Pool For Spawn" and "Return To Pool" are set then it acts like a normal object pool (this is what I would recommend using and originally was the default behavior in the script...I adjusted it to be able to try and replicate the issue under various conditions).

The ObjectPoolSystem also provides you with the ability to easily obtain a reference to it like this:

public class SpawnManager : NetworkBehaviour
{
    [Tooltip("Links the network prefab assigned to an ObjectPoolSystem in order to get the ObjectPoolSystem instance to use.")]
    public GameObject PrefabToSpawn;
    private ObjectPoolSystem m_ObjectPoolSystem;

    private List<ObjectLifeTimeHandler> m_ObjectLifeTimeHandlers = new List<ObjectLifeTimeHandler>();

    protected override void OnNetworkPostSpawn()
    {
        if (IsServer)
        {
            m_ObjectPoolSystem = ObjectPoolSystem.GetPoolSystem(PrefabToSpawn);
        }
        base.OnNetworkPostSpawn();
    }

Where you just have to set the network prefab on another component to use as a way to finding the ObjectPoolSystem that is pooling that network prefab (avoids having to do any weird references and such). It also organizes the object instances under the ObjectPoolSystem's GameObject while they are despawned so you don't get a bunch of disabled objects in your active scene (i.e. for organization purposes and makes it easier to see the objects getting pulled from and put back into the pool).

Finally, it provides a way to make sure all ObjectPoolSystems have instatiated their pools before proceeding.
I provided an example of how this could be used within the NetworkManagerHelper for like a progress bar (didn't add the actual UI portion but did add a component to use and hook up to a progress bar) to show the progress of the pool(s) being instantiated/pre-warmed.

The only thing I can think of is that the BossRoom component uses an ObjectPool and registers an ActionOnDestroy:

            void ActionOnDestroy(NetworkObject networkObject)
            {
                Destroy(networkObject.gameObject);
            }

            m_Prefabs.Add(prefab);

            // Create the pool
            m_PooledObjects[prefab] = new ObjectPool<NetworkObject>(CreateFunc, ActionOnGet, ActionOnRelease, ActionOnDestroy, defaultCapacity: prewarmCount);

Which could be the culprit behind the duplicate destroys. The ObjectPoolSystem just uses a Stack which doesn't have the rest of the fancier ObjectPool functionality... but it is a matter of preference as to how one wants to implement/manage their pooled objects.

Either case, try out the example I provided and see if you can replicate the issue?
Also, you might put some log information within the ActionOnDestroy to see if that is the root cause behind the duplicate destroys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Investigating Issue is currently being investigated stat:awaiting response Status - Awaiting response from author. stat:awaiting triage Status - Awaiting triage from the Netcode team. type:bug Bug Report
Projects
None yet
Development

No branches or pull requests

3 participants