Scene prefab instance invokes custom unspawn instead of deactivating in NetworkClient.DestroyObject

[cmortonNFLX]

Describe the bug
Prefabs placed in a scene, but also registered via RegisterPrefab will invoke the custom unspawn handler instead of being deactivated and added back to spawnableObjects and Reset.

[IMPORTANT] How can we reproduce the issue, step by step:

• Using Tanks as an example.
• Create a new prefab (Cube with a NetworkIdentity component is fine)
• Place it in the scene in the center of the Tanks sample
• Add code to the tanks example to call RegisterPrefab with this prefab and give it a custom unspawn handler (just destroy the object like you would in the simplest possible unspawn handler)
• Add DistanceInterestManagement to Tank example, dist=3 (this allows us to test unspawning on the client)
• Run the sample as host in a stand alone app
• Run the client in editor.
• Move the client tank towards the center and the object will appear (correctly)
• Move the client tank away from center and the object will be destroyed (incorrect)
• Move the tank back towards the center and an error will be reported in the console

Spawn scene object not found for 7A7BE7168BCD866C. Make sure that client and server use exactly the same project.

Expected behavior
Would not expect scene objects to invoke custom unspawn handlers since they don't invoke custom spawn handlers.

Desktop (please complete the following information):

• OS: MacOS
• Build target: standalone
• Unity version: 2021.3]
• Mirror branch: 73.0

Additional Notes
It's also invalid to pass null to RegisterPrefab for the unspawn handler (so this can't be worked around easily)

The code causing this behavior also exists in DestroyAllClientObjects but I don't have a repro for an issue caused by it.

[MrGadget1024]

You don't need to call RegisterPrefab on networked scene objects unless you want the custom unspawn to be invoked. Mirror picks up all networked scene objects automatically.

Does that answer the question, or do you still think there's a bug?

[cmortonNFLX]

@MrGadget1024 The issue here is for mixed use prefabs. I have a prefab that is scene place sometimes, but other times I do dynamically spawn them. Say I have a Vehicle prefab players can drive. Sometimes it's placed by a level designer, other times I spawn it via code.

If my project uses custom unspawning for dynamic objects (i.e. for pooling or some other purpose) then that custom unspawning is also called for scene placed objects (which breaks things). We've been running with a modification to just change the order in NetworkClient.DestroyObject such that sceneId is checked first before unspawnHandlers. This works fine for us, but I understand might not be right for thousands of other mirror projects.

If you didn't want to make that change then this is might just be a restriction that could use a better error message. I.e. on scene load any prefabs that have custom unspawning registered could produce an error that explicit states that's not allowed ?

We'd prefer the reordering change to NetworkClient.DestroyObject if possible!

[MrGadget1024]

We'd prefer the reordering change to NetworkClient.DestroyObject if possible!

Show me your change, please. Here's the DestroyObject method as it exists today:

static void DestroyObject(uint netId)
{
    // Debug.Log($"NetworkClient.OnObjDestroy netId: {netId}");
    if (spawned.TryGetValue(netId, out NetworkIdentity identity) && identity != null)
    {
        if (identity.isLocalPlayer)
            identity.OnStopLocalPlayer();

        identity.OnStopClient();

        // custom unspawn handler for this prefab? (for prefab pools etc.)
        if (InvokeUnSpawnHandler(identity.assetId, identity.gameObject))
        {
            // reset object after user's handler
            identity.Reset();
        }
        // otherwise fall back to default Destroy
        else if (identity.sceneId == 0)
        {
            // don't call reset before destroy so that values are still set in OnDestroy
            GameObject.Destroy(identity.gameObject);
        }
        // scene object.. disable it in scene instead of destroying
        else
        {
            identity.gameObject.SetActive(false);
            spawnableObjects[identity.sceneId] = identity;
            // reset for scene objects
            identity.Reset();
        }

        // remove from dictionary no matter how it is unspawned
        connection.owned.Remove(identity); // if any
        spawned.Remove(netId);
    }
    //else Debug.LogWarning($"Did not find target for destroy message for {netId}");
}

[cmortonNFLX]

// !!! Changed ordering here to ensure scene objects are always cleaned up properly, even if we could spawn the same object at runtime via RegisterPrefab

// scene object.. disable it in scene instead of destroying
if (identity.sceneId != 0)
{
  identity.gameObject.SetActive(false);
  spawnableObjects[identity.sceneId] = identity;
  // reset for scene objects
  identity.Reset();
}
// custom unspawn handler for this prefab? (for prefab pools etc.)
else if (InvokeUnSpawnHandler(identity.assetId, identity.gameObject))
{
    // reset object after user's handler
    identity.Reset();
}
// otherwise fall back to default Destroy
else
{
    // don't call reset before destroy so that values are still set in OnDestroy
    GameObject.Destroy(identity.gameObject);
}

[MrGadget1024]

With your change, are you getting any warnings like this?
Replacing existing spawnHandlers for prefab '{prefab.name}' with assetId '{assetId}'

[cmortonNFLX]

No we aren't seeing that. What's the stack? I didn't make it clear in my test instructions perhaps but i'm only calling RegisterPrefab once on the test cube prefab and I'm not including it in the NetworkManager spawnPrefabs list (that would be what I'd expect to generate that warning)

Also fyi we are on 73.0 still so not sure if anything else has changed.