Unity-Technologies / com.unity.netcode.gameobjects

Netcode for GameObjects is a high-level netcode SDK that provides networking capabilities to GameObject/MonoBehaviour workflows within Unity and sits on top of underlying transport layer.
MIT License
2.13k stars 430 forks source link

AddSceneObject() --> CreateLocalNetworkObject() doesn't parent nested NetworkObjects correctly for non-host clients. #2038

Closed gxp-mattias closed 1 year ago

gxp-mattias commented 2 years ago

Description

I've got a scene where the server spawns in x separate objects (all with NetworkObject components) as children of a single "Spawner" object, which also has a NetworkObject component. When a client connects, everything works as intended and the client Instantiates the objects that were spawned by the server and then attempts to spawn the NetworkObject components on those objects. This is happening in the SynchronizeSceneNetworkObjects() method of SceneEventData.cs.

SynchronizeSceneNetworkObjects() --> AddSceneObject() --> CreateLocalNetworkObject() --> ...

From here, since these objects which were spawned in at runtime by the server are not scene objects, we eventually Instantiate the prefabs, then call transform.SetParent() --> NetworkObject.OnTransformParentChanged(), which throws our error when it sees that we're a client. The change to the parent is reverted, and the objects remain in the root of the hierarchy.

Actual Outcome

The x objects with NetworkObject components are spawned at the root of the hierarchy, as opposed to being moved to children of the "Spawner" object, which also has a NetworkObject component.

Expected Outcome

The hierarchy of the Server's game is preserved for the clients' games as well. All objects with NetworkObject components that are supposed to be parents or children of other objects with NetworkObject components are.

Environment

NoelStephensUnity commented 2 years ago

Hello @gxp-mattias! Sorry that you are having issues with NetworkObject parenting. Netcode for GameObjects uses a server authoritative model, which means all parenting actions have to be done on the server side. If you must have a client driving when a NetworkObject is parented, then I would suggest using an RPC to send a message to the server, otherwise when the NetworkObject is spawned you could do it on the server side in OnNetworkSpawn.

gxp-mattias commented 2 years ago

Hi @NoelStephensUnity Thanks for getting back to me. I understand the idea behind parenting being controlled by the server, but in my specific case I don't understand why what I'm doing breaks server authority. The client isn't trying to drive or set any parenting operations. The client is only trying to get an identical object hierarchy as the server when they join the game after the server has already spawned some things in.

NoelStephensUnity commented 2 years ago

@gxp-mattias it would definitely help if I could see an example of your project or a sub-set that highlights the issue(optional). If that is not possible then perhaps the following information might help:

NetworkObjects spawned on the server should automatically be synchronized with newly joining clients. During the client synchronization period, if there are any children of a NetworkObject then upon the children spawning they should be "automatically" parented without having to do anything on the client-side.

As an example: If the server spawns 2 NetworkObjects (A & B) and makes B a child of A, then when a client joins, as long as you have scene management enabled within the NetworkManager, on the client-side (during synchronization) it should spawn both NetworkObjects (A & B) and make B a child of A automatically for you.

With the above in mind, do you have any "client-side" specific code that is trying to parent the NetworkObjects while the client is synchronizing (this would indeed cause that exception)?

If so and your client and server are executing the same block of code, wrap the code that parents the NetworkObjects in an "If" statement that only allows the server to execute it:

if (IsServer)
{
    ChildNetworkObject.transform = ParentNetworkObject.transform.parent;
}

As a side note: When we refer to "server authority" that typically means "only the server can perform these actions". So, if you try to perform a server authoritative action (i.e. spawning, parenting, etc.) on a client you will most likely get an exception complaining that a client is trying to perform an action it is not allowed to do.

If I understand you correctly and there is client-side code trying to match the same hierarchy by parent NetworkObjects locally on the client-side (i.e. not allowing the synchronization process to handle it), then that will break the "server authority" contract by performing a server-only action on a client.

Also, I would highly recommend updating to 1.0.0-pre.10 as it contains some fixes that "might" resolve this issue. Otherwise, let me know if this helps at all or if I am completely misunderstanding the issue you are having?

gxp-mattias commented 2 years ago

@NoelStephensUnity I apologize for my lack of clarity. There is no client side code that is trying to do something that only the server has the authority to do.

The server spawns n objects (I'll refer to them as [Obj-1, Obj-2, ..., Obj-n]). When each of these objects is spawned, their parent transform is set to the object which contains the script which spawns them. So all n objects share the same parent, P-Obj. P-Obj and [Obj-1 ... Obj-n] are all network objects.

This is all happening before the client joins the game. The client doesn't try to do anything when they join the game. There is no custom code at work. I am getting these errors (and no parenting) precisely in the default synchronization process.

I can't share with you the project, but I will create a dummy project to showcase the problem. Thanks so much for your patience and help so far!

NoelStephensUnity commented 2 years ago

@gxp-mattias Ahhh...ok...then yes I need to investigate this issue further.

Before I can do that, I have a few more questions for you before I can try to replicate the issue on my side:

Once I can replicate/confirm the issue I will then open an internal ticket and add the project I used to replicate it to the ticket.

Cheers,

Noel

NoelStephensUnity commented 2 years ago

@gxp-mattias As a follow up I believe I replicated the bug on my side (I assumed the parent is an in-scene placed NetworkObject) and have a way to work around the issue.

The Script Used

The below script I used to both verify that it was indeed a bug (check) and it was used to come up with an alternate approach that will avoid the issue you are experiencing.

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

public class GenericSpawner : NetworkBehaviour
{
    public GameObject AssetToSpawn;  // Must be a Network Prefab registered with NetworkManager
    public int SpawnCount = 5;
    public bool ParentSpawnedObjects;

    private List<NetworkObject> m_SpawnedInstances = new List<NetworkObject>();

    public override void OnNetworkSpawn()
    {
        if (IsServer)
        {
            var offset = transform.position;
            for (int i = 0; i < SpawnCount; i++)
            {
                offset.y += 0.5f;
                var instance = Instantiate(AssetToSpawn);

                var instanceNetworkObject = instance.GetComponent<NetworkObject>();
                m_SpawnedInstances.Add(instanceNetworkObject);
                instanceNetworkObject.Spawn();
                instance.transform.position = offset;
                if (ParentSpawnedObjects)
                {
                    instance.transform.parent = transform;
                }
            }
        }
    }

    public override void OnNetworkDespawn()
    {
        if (IsServer)
        {
            foreach (var networkObject in m_SpawnedInstances)
            {
                networkObject.Despawn(true);
            }

            m_SpawnedInstances.Clear();
        }
    }
}

My first attempt (where I confirmed the bug):

The first pass I assumed you were using an in-scene placed Networkobject so I place this script on a newly created in-scene placed NetworkObject, created a new Network Prefab to use for spawning the children (I added a NetworkTransform to this so they would synchronize their position for visualization purposes), registered the newly created Network Prefab with the NetworkManager, assigned the Network Prefab to the "AssetToSpawn" property of the in-scene placed NetworkObject GenericSpawner component, and finally checked the "ParentSpawnedObjects" property so it would parent the spawned instances under itself. This resulted in a soft synchronization error on the client side as the children of the parent spawner were (the bug) somehow marked as in-scene placed NetworkObjects too (when they should not be...still needs further investigation)

My second attempt (The Work Around):

I ended up with two registered Network Prefabs:

The Work Around Once I turned my spawner into a NetworkPrefab, I then created a new in-scene placed NetworkObject and called it: SpawnWrapper. The properties looked like this: image Where I am only spawning 1 instance of the ParentSpawner that gets positioned where the in-scene placed SpawnWrapper is positioned and the "Parent Spawnd Objects" property was left un-checked (i.e. false). Then the ParentSpawner instance, upon being spawned by the wrapper, would spawn its children.

The idea for this is: The "SpawnWrapper" basically converted the original (first attempt) in-scene placed parent spawner into a dynamically spawned instance of the ParentSpawner Network Prefab. This avoids the parenting issue with the in-scene placed NetworkObject since the SpawnWrapper is not parenting the ParentSpawner.

Currently I am working on writing better documentation for in-scene placed NetworkObjects, and already have a section which calls out the recommended usage patterns for in-scene placed NetworkObjects which also discusses how to determine when you should or should not use in-scene placed NetworkObjects.

Until we release an update, I would highly recommend using this "wrapper" approach if you can answer yes to any the following questions (part of the up-and-coming documentation updates):

So, if you can say "yes" to any of the above questions then the path of "least resistance" is to just use the in-scene placed NetworkObject wrapper approach to spawn the Network Prefab....and in your case it would then spawn more NetworkObjects and parent them under itself.

Let me know if using the "wrapper" approach solves your current issue?

gxp-mattias commented 2 years ago

@NoelStephensUnity Thank you so much for testing this out and demonstrating a workaround. I also got the soft-sync issue with spawned network prefabs being recognized as in-scene placed network objects. I was unable to trace where exactly IsSceneObject is set to true for these objects, so I assume it might be happening in code that's generated by the NGO backend.

I will test out your workaround with the SpawnWrapper and see if it solves the problem. I'm fairly confident that it will solve it, since your original test setup is almost identical to how I'm doing it in our project.

Thanks again!

gxp-mattias commented 2 years ago

@NoelStephensUnity I tried implementing your solution and it didn't work for me right out of the box. However I realized that this was maybe because of the way I was parenting the child objects to the parent. I was doing something like this:

GameObject spawnedPrefab = Instantiate(myPrefab, transform); spawnedPrefab.GetComponenet<NetworkObject>().Spawn();

With the parenting happening before the network object spawning, I still got the same error as before, even with the wrapper implementation. However once I changed the order of these two operations such that the network object is spawned first, and then the object is parented, the wrapper solution worked without a problem!

Thanks again, Mattias

NoelStephensUnity commented 2 years ago

Ahh! Yes, v1.0 does not support "pre-spawn parenting". Apologies for not making that portion clearer. However, I am glad to here that you are no longer blocked on this issue!
(leaving this issue open for triage purposes)

ashwinimurt commented 2 years ago

Setting priority to low because of existing workaround.

ashwinimurt commented 2 years ago

Backlog: MTT-4262