com.unity.netcode.gameobjects icon indicating copy to clipboard operation
com.unity.netcode.gameobjects copied to clipboard

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

Open ArjanSioux opened this issue 1 year ago • 14 comments

Description

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

  • Starting a host assigns every NetworkObject an ID and a parent ID.
  • Stopping a host and restarting it seems to assign every NetworkObject a new ID, but does not update parent IDs.
  • This results in the hierarchy being shuffled up at random, with high likelihood of orphans.

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

  • OS: Windows
  • Unity Version: 2022.3.9f1
  • Netcode Version: 1.5.2
  • Netcode Commit: ?

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);
    }
}

ArjanSioux avatar Apr 12 '24 12:04 ArjanSioux

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

NoelStephensUnity avatar Apr 22 '24 14:04 NoelStephensUnity

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

ArjanSioux avatar Apr 24 '24 08:04 ArjanSioux

@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).

NoelStephensUnity avatar Apr 24 '24 14:04 NoelStephensUnity

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.

ArjanSioux avatar May 01 '24 13:05 ArjanSioux

@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

NoelStephensUnity avatar May 02 '24 14:05 NoelStephensUnity

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:

  • When I go "offline" in my project, I start a local host that accepts no incoming connections, with the standard Unity Transport.
  • When I go online in my project, I start a host via the Relay service and corresponding transport.
  • Any specific GameObject's Network Object Id seems to change when switching between the above two, while parent Id is never updated.

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

ArjanSioux avatar May 03 '24 12:05 ArjanSioux

@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:

  • You can migrate your relay code into that AutoObjectSync project to see if you can replicate the issue with just the fundamental components. If so, then zip the Assets folder up and shoot that my way.
    • If you cannot replicate the issue, then some other component/script is possibly the cause.
  • You could try to figure out if there is a specific sequence to this issue relative to you starting an "offline" session versus an "online" one.
    • If you have domain reloading disabled or scene reloading disabled, let me know too as that could be a part of why this is happening.
  • You could place debug log messages anywhere in your scripts where you might be parenting these NetworkObjects.
    • If you are not doing any parenting then ignore this option.
    • If you are doing parenting, then send me the editor and player log of you replicating the issue so I can see when they are parented versus when you are starting and stopping and then restarting a session.
  • You can try to determine if it seems that it is only happening when you are using relay (an "online" session).
    • Depending upon how quickly you are stopping and restarting a session could possibly be related.

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. 👍

NoelStephensUnity avatar May 03 '24 23:05 NoelStephensUnity

No problem, happy to help this get solved.

AutoObjectSyncWithRelay.zip

I took your example and changed the following:

  • Added Relay package
  • Changed NGO version to 1.9.1
  • Added hierarchy of NetworkObjects as in my project
  • Added NetworkConnection.cs, taking care of switching between "offline" host and relay host
  • Adjusted NetworkManagerHelper to show buttons for switching between offline and relay
  • I didn't do/add any parenting myself in these scripts.

To reproduce the problem:

  • Expand (Alt+click) the full hierarchy starting at the "Machine" GameObject
  • Start play mode
  • Switch to relay with the OnGUI button
  • Observe change in hierarchy starting at "Machine" GameObject AutoObjectSyncParentBug

Was running this with Unity 2022.3.9f1

ArjanSioux avatar May 07 '24 13:05 ArjanSioux

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

NoelStephensUnity avatar May 16 '24 14:05 NoelStephensUnity

@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)

  • Machine(NetworkObject)(NetworkTransform)
    • SubAssemblies (NetworkTransform) ---> will this actually move? If not you don't need to add this.
      • System (NetworkTransform) ---> will this actually move? If not you don't need to add this.
        • SubAssemblies (NetworkTransform) ---> will this actually move? If not you don't need to add this.
          • ScraperTray1 (NetworkTransform) --> if all NetworkTransforms above are removed this will still work (if it moves)
            • States
              • ScraperTray

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:

  • If there will be no change in scale, then you don't need to synchronize scale (you can un-check those)
  • If you don't plan on the child actually moving from its current position but do plan on the piece rotating then:
    • If the piece/child rotates on all axis then keep them all checked, but if the child only rotates on a single axis (like a wheel/cog) then you only need to synchronize that axis (x, y, or z).
  • For position it is the same, determine if you actually need to synchronize all axis or just some axis.

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).

NoelStephensUnity avatar May 17 '24 22:05 NoelStephensUnity

@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:

  • Most sub-assemblies will be able to move, some rotate, and only the root machine scale.
  • All sub-assemblies have 1..N states, of which only one is visible. (I plan to enable/disable appropriate MeshRenderers based on the changed event of a NetworkVariable<bool> visible in the sub-assembly.
  • All of the machine parts (except for Products) will be known at build time (can be placed in-scene).
  • Products, the input and output of the machine (and any states in between that), will be spawned at runtime and will be parented to the different ProductSlot parts of the machine.
  • ProductSlots are part of the machine and are often children of moving sub-assemblies, such as a gripper.
  • In our automated tests, we often create a minimal machine for one test case from within the code, so at runtime/not in-scene placed. Having to create a scene for every test case would become unworkable. So far, it felt like quite a pain to need a running host for these tests.

If I understand correctly, I can:

  • 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.)
  • Have a NetworkObject and NetworkTransform on every Product.
  • Leave out NetworkObject and NetworkTransform from all the other parts of the hierarchy.

Some things I'm still unsure of:

  • 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"?
  • 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?
  • 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?

Thanks for all the help!

ArjanSioux avatar May 21 '24 07:05 ArjanSioux

@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:

  • (Root-NetworkObject)

    • (Series of Nested GameObjects some with or without NetworkTransforms)
      • Product Slot (GameObject)
  • Product (NetworkObject Root)

    • Product-PrimaryNode
    • (Series of Nested GameObjects some with or without NetworkTransforms)

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".

  • (Root-NetworkObject)

    • (Series of Nested GameObjects some with or without NetworkTransforms)
      • Product Slot (GameObject)
        • Product-PrimaryNode
          • (Series of Nested GameObjects some with or without NetworkTransforms)
  • Product-PrimaryNode

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?

NoelStephensUnity avatar Jul 29 '24 13:07 NoelStephensUnity