fix: nested gameobject networkmanager scenename exception #1432

NoelStephensUnity · 2021-11-18T19:58:11Z

Networkmanage 's gameObject can't be a child of another gameObject. otherwise will show this issue:
Exception: Failed to find any loaded scene named "XXXX"
and everything related to the network will not working.

This fixes Github-Issue #1417 (described above)

MTT-1732

PR Checklist

Have you added a backport label (if needed)? For example, the type:backport-release-* label. After you backport the PR, the label changes to stat:backported-release-*.
Have you updated the changelog? Each package has a CHANGELOG.md file.

Changelog

com.unity.netcode.gameobjects

Fixed issue when NetworkManager's GameObject is nested under one or more GameObject(s) it wouldn't properly migrate into the DDOL and would cause various errors including client side scene synchronization upon first connecting

Testing and Documentation

Includes integration tests: Modified an existing test as well as added some additional functionality to MultiInstanceHelpers.

When moving a GameObject into DDOL, we must move the root GameObject or it will fail.

added comment above change in NetworkManager.OnEnable to clarify why we must get the root GameObject of the NetworkManager before attempting to migrate it into the DDOL.

added fix into the unreleased section of the changelog.

Changed NetworkManager.GetParent to be renamed to NetworkManager.GetRootParent to better reflect what the method does. Updated NetworkManager.GetRootParent to automatically return the root parent without having to pass in the GameObject that the NetworkManager component is attached to. Added the ability to override the server and client creation process for MultiInstanceHelpers. We could expand upon what portions of MultiInstanceHelpers can be overridden, but this at least puts a framework in place and should be useful to handle construction of customized NetworkManager instances in place of the default MultiInstanceHelpers way.

0xFA11 · 2021-11-19T12:33:52Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

+                // We must always move the root GameObject into the DDOL
+                DontDestroyOnLoad(GetRootParent());


I have mixed feelings about this.
We might just throw an exception here (similar to DontDestroyOnLoad API throwing "only root objects can be marked DDOL") and expect user to do the right thing so that they could be much more intentional.
One thing I don't want to encourage here is to fix issues "automagically" for the user — they should face invalid use cases and align things accordingly.

so to be clear, maybe the fix is just throwing a descriptive exception here.
having said that, it looks like issue #1417 never got an exception but instead got a "not found" error.
perhaps we should properly throw an exception to better inform user what's going on so that they could understand and react?

maybe the fix is just throwing a descriptive exception here.

Object.DontDestroyOnLoad is basically a "one off" method from SceneManager.MoveGameObjectToScene which has the following requirement:

You can only move root GameObjects from one Scene to another. This means the GameObject to move must not be a child of any other GameObject in its Scene.

This makes it a requirement to provide the root GameObject to Object.DontDestroyOnLoad.

never got an exception but instead got a "not found" error.

Don't forget the second part of what the user reports in the issue:

and everything related to the network will not working.

It can get many different types of errors when this happens for different reasons.

So, keeping in alignment with the default UnityEngine requirements solves the issue. For NetworkManager (only), it should be perfectly valid to place the NetworkManager component's GameObject as a child under another GameObject (or several depending upon what the user need might be).

Throwing an exception I am going to try to stay away from for this one.

I disagree. DDOL on a child object is not OK in Unity therefore I believe we should not magically fix user errors here but instead just shout them out loud.
We could chat about this but I'm also happy to defer this one to @andrews-unity or @mattwalsh-unity

Well, I do agree about the requirement to send the root parent into the DDOL. I also think that we could throw an exception or warn the user in the editor. The only thing I wonder about is whether it is the right thing to throw an exception under this condition since everything NGO is really "a child" (component) of GameObject. If you have GameObject-A as a root parent with no NGO components and GameObject-B with a NetworkObject component as the child of GameObject-A but you want to make sure the NetworkObject (GameObject-B) itself is moved into the DDOL then the big question is do we assert/except on this condition or do we just hunt for the root parent and move that into DDOL?

I am open to both lines of thinking and will see if I can stir up a discussion on this topic to discuss further.

Ok, per our slack exchange, I'd like to go ahead and do the following:

(having conferred with @TwoTenPvP) Make NetworkManager always "DDOL" - remove the setting. That is we never ever want to have the act of changing a scene implicitly tear down a NM

require NetworkManager to be sitting on a GameObject which itself is a root-level component, else complain as loudly and proudly and early as possible (ideally in the editor, ideally to block running at all)

(not in this PR) investigate how we can make "NM is a component of a GO" not a thing

0xFA11 · 2021-11-19T12:34:22Z

com.unity.netcode.gameobjects/Tests/Runtime/MultiInstanceHelpers.cs

+    public interface IMultiInstanceHelperOverrides
+    {
+        bool OnCreateNewClients(int clientCount, out NetworkManager[] clients);
+
+        bool OnCreateHostServer(out NetworkManager server);
+    }


huh? what's going on here?

This provides developers with the ability to override the client and host-server creation process for the MultiInstanceHelpers class by implementing IMultiInstanceHelperOverrides and assigning the implementation to MultiInstanceHelpers.MultiInstanceHelperOverrides. There have been many instances where this capability would be useful in the past, but in order to be able to test it does require nesting the NetworkManager which needs to be done prior to initialization.
Maybe I should put some better comments above it? :)

why it's in this PR?

It is for testing. It provides the ability to override how we construct the server and client NetworkManagers.
This was the solution I came up with to test a nested NetworkManager GameObject. It also provides developers with this added functionality.

This might change as we might just throw an exception under this condition and as such would be an editor specific test as opposed to runtime (i.e. no need for using any kind of MultiInstance test).

0xFA11

^ see my comments

deferring to other tech leads

mattwalsh-unity · 2021-11-29T20:52:38Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

+            currentGameObject = currentGameObject == null ? gameObject : currentGameObject;
+            if (currentGameObject.transform.parent != null)
+            {
+                return GetRootParent(currentGameObject.transform.parent.gameObject);


Can transform.root work here?

Yes! Awesome catch! I am about to check in that adjustment.

mattwalsh-unity · 2021-11-29T21:54:14Z

com.unity.netcode.gameobjects/Tests/Runtime/MultiInstanceHelpers.cs

@@ -117,7 +159,8 @@ public static void Destroy()
            // Destroy the network manager instances
            foreach (var networkManager in NetworkManagerInstances)
            {
-                Object.DestroyImmediate(networkManager.gameObject);
+                // Always destroy the root parent


What if we don't want to destroy the parent? This is making parents of NM's special.

For MultiInstance specific testing scenarios, yes that would make this uniquely handled.
I am not aware of a condition where we want to persist the parent GameObject of a nest NetworkManager GameObject, but if we keep this approach then I could add some additional functionality. If we go the path of just throwing an exception then the majority of the unit/integration test specific code would be removed and this would no longer be here.

…ename-exception

Using the transform.root.gameObject as opposed to the recursive approach.

mattwalsh-unity

Ok, see my one comment

mattwalsh-unity · 2021-11-30T01:17:53Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

+                // We must always move the root GameObject into the DDOL
+                DontDestroyOnLoad(GetRootParent());


Ok, per our slack exchange, I'd like to go ahead and do the following:

(having conferred with @TwoTenPvP) Make NetworkManager always "DDOL" - remove the setting. That is we never ever want to have the act of changing a scene implicitly tear down a NM

require NetworkManager to be sitting on a GameObject which itself is a root-level component, else complain as loudly and proudly and early as possible (ideally in the editor, ideally to block running at all)

(not in this PR) investigate how we can make "NM is a component of a GO" not a thing

NoelStephensUnity · 2021-12-01T23:25:20Z

The fix for this has been migrated to PR-1484

NoelStephensUnity added 2 commits November 18, 2021 13:37

fix

46562dc

When moving a GameObject into DDOL, we must move the root GameObject or it will fail.

style

72eea67

added comment above change in NetworkManager.OnEnable to clarify why we must get the root GameObject of the NetworkManager before attempting to migrate it into the DDOL.

NoelStephensUnity added the type:backport-release-1.1.0 label Nov 18, 2021

updated changelog

c451f34

added fix into the unreleased section of the changelog.

NoelStephensUnity changed the title ~~Fix/nested gameobject networkmanager scenename exception~~ fix: nested gameobject networkmanager scenename exception Nov 18, 2021

NoelStephensUnity requested a review from 0xFA11 November 18, 2021 21:13

0xFA11 reviewed Nov 19, 2021

View reviewed changes

0xFA11 previously requested changes Nov 19, 2021

View reviewed changes

NoelStephensUnity requested a review from 0xFA11 November 19, 2021 14:36

NoelStephensUnity requested review from andrews-unity and mattwalsh-unity November 19, 2021 19:50

mattwalsh-unity reviewed Nov 29, 2021

View reviewed changes

NoelStephensUnity added 2 commits November 29, 2021 17:07

Merge branch 'develop' into fix/nested-gameobject-networkmanager-scen…

2ee8018

…ename-exception

refactor

bdfb81f

Using the transform.root.gameObject as opposed to the recursive approach.

mattwalsh-unity suggested changes Nov 30, 2021

View reviewed changes

NoelStephensUnity closed this Dec 1, 2021

		// We must always move the root GameObject into the DDOL
		DontDestroyOnLoad(GetRootParent());

fix: nested gameobject networkmanager scenename exception #1432

fix: nested gameobject networkmanager scenename exception #1432

Uh oh!

Conversation

NoelStephensUnity commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Changelog

com.unity.netcode.gameobjects

Testing and Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xFA11 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattwalsh-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity commented Dec 1, 2021

Uh oh!

Uh oh!

NoelStephensUnity commented Nov 18, 2021 •

edited

Loading

NoelStephensUnity Nov 19, 2021 •

edited

Loading

NoelStephensUnity Nov 19, 2021 •

edited

Loading