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 433 forks source link

Remove the requirement and error message "NetworkManager cannot be nested" #2984

Open CodeSmile-0000011110110111 opened 2 months ago

CodeSmile-0000011110110111 commented 2 months ago

Is your feature request related to a problem? Please describe. When nesting the NetworkManager object in the scene, an editor script pops up a message box which says (shortened):

NetworkManager cannot be nested.
Click 'Auto-Fix' to automatically remove it from Global Objects or 'Manual-Fix' to fix it yourself in the hierarchy view.

Describe the solution you'd like Make NetworkManager nestable, provided that the reason for doing so is mainly or solely because of DontDestroyOnLoad(..)

DontDestroyOnLoad can be applied to nested game objects simply by first unparenting the object, for example:

void Awake()
{
    transform.parent = null;
    DontDestroyOnLoad(gameObject);
}

Otherwise I would suggest improving the message box so that it mentions WHY it cannot be nested.

Additional context I'm requesting this primarily because it feels overly restrictive to enforce a non-parenting NetworkManager and as a consequence, it feels like an outlier object because no other Unity component I've ever used had this particular restriction.

Other than the DDOL parenting reason above, which has a known and trivial solution, I could not think of any other reason why this is enforced. If there is another reason I'd like to hear about it though since I like to understand the rationale (which is absent from the message, which annoys me like any other "YOU CAN'T DO THIS" message or instruction without telling why).

CodeSmile-0000011110110111 commented 2 months ago

I just want to replicate the related NetworkManager code (w/o the notify/messages):

        /// <summary>
        /// Determines if the NetworkManager's GameObject is parented under another GameObject and
        /// notifies the user that this is not allowed for the NetworkManager.
        /// </summary>
        internal bool NetworkManagerCheckForParent(bool ignoreNetworkManagerCache = false)
        {
#if UNITY_EDITOR
            var isParented = NetworkManagerHelper.NotifyUserOfNestedNetworkManager(this, ignoreNetworkManagerCache);
#else
            var isParented = transform.root != transform;
            if (isParented)
            {
                throw new Exception(GenerateNestedNetworkManagerMessage(transform));
            }
#endif
            return isParented;
        }

Called from OnEnable:

            if (!NetworkManagerCheckForParent())
            {
                DontDestroyOnLoad(gameObject);
            }

To try whether the nested NetworkManager creates any issues I modified it and will continue using it like this until there's a problem (I'd be surprised):

            //if (!NetworkManagerCheckForParent())
            {
                transform.parent = null;
                DontDestroyOnLoad(gameObject);
            }

And NetworkManagerCheckForParent is modified to always return false, to disable the nagging popup message.

NoelStephensUnity commented 2 months ago

Hmmm... I will revisit this area when I get a chance and see if there were any other reasons behind this. It could be a legacy restriction that, as you point out, is no longer required.

Of course, it does seem odd to nest the NetworkManager under something only to have it removed and placed at the root of the DDOL scene...but then again everyone has their own way of organizing things so I will look into this. 👍

CodeSmile-0000011110110111 commented 2 months ago

For rationale: I have parent called "Network Objects" (and several other grouping objects like it) in the scene for honing in on what you intend to modify. Generally I prefer one component per global object, rather than a blob object with plenty of components, as this reduces complexity and is easier to locate components.

So it is currently odd that the NetworkManager cannot be a child of that "Network Objects" group. The other children currently are MPPM settings, Network Simulator, Network Monitor and there'll be more.
 

image

Note: having a parent object is also required for Multiplayer Roles component stripping as that will only work for parented objects. So I thought it would be a good idea to leave no object (with components) at the scene root to begin with.

NoelStephensUnity commented 2 months ago

So it is currently odd that the NetworkManager cannot be a child of that "Network Objects" group. The other children currently are MPPM settings, Network Simulator, Network Monitor and there'll be more.

Well, if you think about it... NetworkManager derives from MonoBehaviour and doesn't require being "spawned" (i.e. like a NetowrkObject) but in order to spawn something it does need to be started. But... I could see how wanting to group all "netcode" things together would make sense...but I might rename "Network Objects" to something like "Netcode Objects" (again personal preference always wins here).

CodeSmile-0000011110110111 commented 2 months ago

I might rename "Network Objects" to something like "Netcode Objects"

I will do so the moment it's called NetcodeManager. :)