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.15k stars 435 forks source link

AutoObjectParentSync for in-scene GameObjects sets wrong parents after stopping and restarting host #2882

Open ArjanSioux opened 7 months ago

ArjanSioux commented 7 months ago

Description

Given a hierarchy of GameObjects with NetworkObject components with AutoObjectParentSync on:

Reproduce Steps

  1. Create a hierarchy (some parents and children) of GameObjects in a scene.
  2. Add NetworkObjects to all of the GameObjects and make sure AutoObjectParentSync is on.
  3. Add a NetworkManager to the scene.
  4. Enter play mode.
  5. Start a host.
  6. Stop the host.
  7. Start a host again.

Actual Outcome

The parent child relationships in the scene hierarchy got shuffled.

Expected Outcome

The parent child relationships in the scene hierarchy should not be affected.

Screenshots

Before stop and restart of host, ZRailAndLight's parent is SubAssemblies with ID 44, and hierarchy is as expected: image image

After stop and restart of host, ZRailAndLight's parent is States with ID 44, with hierarchy completely shuffled: image image

Environment

Additional Context

My workaround for now is to use this on all NetworkObjects in a NetworkManager.OnClientStopped callback:

public static class NetworkObjectExtensions
{
    private static FieldInfo? _latestParentFieldInfo = typeof(NetworkObject).GetField("m_LatestParent", BindingFlags.NonPublic | BindingFlags.Instance);

    // Netcode for GameObjects' auto parent sync assigns IDs and parent IDs whenever you start a host.
    // When stopping and restarting a host, the object IDs are reassigned, but the parent IDs are kept.
    // This messes up our scene hierarchy (IDs pointing to parents are kept, but those parents have new IDs).
    // For now, this extension method is a workaround, to be called whenever we close a connection (stop a host).
    // Using this, the auto parent sync feature will reevaluate the parent IDs.
    public static void ResetParentNetworkObjectId(this NetworkObject networkObject)
    {
        if (_latestParentFieldInfo == null)
        {
            Debug.LogError("An update broke our workaround for auto parent sync hierarchy shuffle. Check if bug remains or find new variable name.");
            return;
        }
        _latestParentFieldInfo.SetValue(networkObject, null);
    }
}
NoelStephensUnity commented 7 months ago

@ArjanSioux Could you update to NGO to v1.8.1 and see if this issue persists?

ArjanSioux commented 7 months ago

Switched package version to 1.8.1 and commented out my workaround: issue persists.

NoelStephensUnity commented 7 months ago

@ArjanSioux Thanks you for confirming that for me. I marked this for import and will post a PR to resolve this issue. It will land in the update after NGO v1.9.1 (any day this should be available).

ArjanSioux commented 6 months ago

Just retried this with 1.9.1: issue persists. :( Edit: just noticed you mentioned after 1.9.1. I'll try again when 1.9.2 is available.

NoelStephensUnity commented 6 months ago

@ArjanSioux Do you have any scripts that change the parenting of these NetworkObjects?

I put together a scene to test if this happens in v1.9.1. Just added several GameObjects with NetworkObjects (all having Auto Object Parent Sync enabled): image Entered into play mode and started the host: image Stopped the host: image Then started the host again: image

To make sure I started and stopped the host several times and the in-scene placed NetworkObjects kept their parent-child hierarchy.

Using 2022.3.27f1 and NGO v1.9.1. image

I am attaching the project I used for reference purposes. See if it still happens on your end with this project. If not, could you try to determine what is different between the two? (i.e. do you have domain reloading or scene loading disabled and/or are there other components that are possibly changing parenting when the host starts?) AutoObjectSync.zip

ArjanSioux commented 6 months ago

I opened your example project, pasted in the hierarchy from my project and could not reproduce the issue. However, I also went back to 1.8.1 and could not reproduce the issue.

A difference I'm noticing is that in my project Network Object Id does not stay the same between sessions, while it does in your project. Worth noting is:

So even with 1.9.1, the issue can be reproduced in my project.

NoelStephensUnity commented 6 months ago

@ArjanSioux Hmmm... a sneaky bug it would appear.

I know might seem like I am asking you to jump through hoops... but there could be a number of combinations of things that we might be missing in some of those additional details you provided...soo...it might be faster if you try some of the following things to help narrow down where this bug resides:

Sorry to provide so many possible approaches, just pick and choose the ones that appear to be the simplest/least time consuming for you and we can take it from there. I have tried another project that does have relay integrated and added a similar simple in-scene placed parent child hierarchy and still could not replicate on my end... so there is definitely something in your project that is the root cause... we just have to find it. 👍

ArjanSioux commented 6 months ago

No problem, happy to help this get solved.

AutoObjectSyncWithRelay.zip

I took your example and changed the following:

To reproduce the problem:

Was running this with Unity 2022.3.9f1

NoelStephensUnity commented 6 months ago

@ArjanSioux Very awesome... thank you for the details this is going to help a bunch. Will be looking at this shortly. 🥇

NoelStephensUnity commented 6 months ago

@ArjanSioux Just out of curiosity, do you plan on being able to move every single child under the root Machine GameObject and require a NetworkObject on every child?

I ask this because you can nest NetworkBehaviours under a single root NetworkObject and they will work just fine (they would all belong to the root NetworkObject).

When you would want to separate NetworkBehaviours from a NetworkObject is if you need the child to act as an independent instance that could be complete detached from its parent and the parent could be despawned and the detached child would still work (which for an in-scene placed NetworkObject I wouldn't recommend that).

As an example:

You could just have 1 NetworkObject and then place a NetworkTransform on any child at any level and it would still work properly (assuming all of the children you placed NetworkTransforms on will move around/rotate from their original position)

You can also place any other NetworkBehaviour derived class on any child of the Machine and it will work just fine too. The other thing worth noting is what is being synchronized. I am not sure if this is what you have set in your project or if it was just a rapid reconstruction of your project and you didn't make a complete 1:1 match of your settings, but you might think about what you really need to synchronize for the children with NetworkTransforms:

I am going to be looking at the cause for this kind of complex hierarchy, but if you don't plan on detaching anything from the Machine (i.e. everything will remain children as they are arranged in the scene), then I would definitely remove all NetworkObjects except the one on Machine and see if you get better results.

If you do plan on detaching things, then I would recommend making each piece a NetworkPrefab and dynamically spawning them and having them parent under their designated target (which I also have a different approach I could share for something like that).

ArjanSioux commented 6 months ago

@NoelStephensUnity I think your tips would greatly reduce complexity in terms of NetworkObject/Transform for us. I was placing those under the assumption that a full chain of them was required for sync'ing transforms and parents.

So just to verify whether it matches our use cases:

If I understand correctly, I can:

Some things I'm still unsure of:

Thanks for all the help!

NoelStephensUnity commented 4 months ago

@ArjanSioux Apologies for the delayed response. Just finally getting a chance to circle back to your questions:

  • Have a NetworkObject and NetworkTransform on the root of the machine.
  • Have a NetworkTransform on every sub-assembly. (With flags for which transform parts should be sync'ed, different for every specific instance.)

Yes. That is correct.

  • Have a NetworkObject and NetworkTransform on every Product.
  • Leave out NetworkObject and NetworkTransform from all the other parts of the hierarchy.

That would be the recommended way to do this.

  • There will be sub-assemblies that are children of sub-assemblies. So hierarchical movement, which I'm not sure matches your description of "will move around/rotate from their original position"?

That is correct. It will work just like nested transforms work.

  • Do I need a NetworkObject/Transform on every ProductSlot (since Products will at runtime become children of them)? Note: Every ProductSlot will be a descendant of the machine root, which has a NetworkObject.
  • If so, can a ProductSlot, in-scene with a NetworkObject, be a child of something that has no NetworkObject?

What I would do is this:

Once the product is spawned, you can take the Product-PrimaryNode that doesn't have its NetworkObject component and place it under any other GameObject. In this case you would parent it under say the "Product Slot".

You will still have the " Product (NetworkObject Root)" in the root of your scene, and all NetworkBehaviours (including NetworkTransforms) would still have their messages routed correctly. You just have to remember to re-parent the " Product-PrimaryNode" when the " Product-PrimaryNode" despawns.

  • Is there an easy way to disable the network part (running host requirement) for tests in which we instantiate prefabs with NetworkObject, but only test non-network-related behavior?

You could do this, but you would need to just be conscious of how you write your scripts. Pretty much any NetworkBehaviour update method would need to exit early if it wasn't spawned. Another way you could do this is create your own "NetworkBehaviour" base class where you split updates:

public class CustomNetworkBehaviour : NetworkBehaviour
    {

        /// <summary>
        /// Write netcode script here in overridden methods
        /// </summary>
        protected virtual void OnNetcodeUpdate()
        {

        }

        /// <summary>
        /// Write non-netcode script here in overridden methods
        /// </summary>
        protected virtual void OnUpdate()
        {

        }

        private void Update()
        {
            OnUpdate();
            if (IsSpawned)
            {
                OnNetcodeUpdate();
            }
        }
    }

Let me know if this helps?

ArjanSioux commented 1 month ago

Thanks for the thorough response. This really helps!

For the Product - Primary Node being parented to the ProductSlot, ensuring that that parenting is synchronized is something I would have to do manually, right?

The splitting off network specific updates also sounds like a good idea to investigate for us.

NoelStephensUnity commented 1 month ago

For the Product - Primary Node being parented to the ProductSlot, ensuring that that parenting is synchronized is something I would have to do manually, right?

Yes. You could synchronize clients by creating a NetworkVariable that you set upon performing this custom parenting action so non-authority instances would then:

The use of NetworkVariables assures if a late joining client connects they will synchronize to these changes (the caveat is upon say the OnNetworkPostSpawn you would want to check that NetworkVariable and handle the custom parenting accordingly).

You might need some additional information to know which node to parent under, but that is the general idea.