fix: OnNetworkDespawn not called during shutdown

[NoelStephensUnity]

When shutting down directly, NetworkObjects were not running through the OnNetworkDespawn process prior to being destroyed. This fixes the issue by assuring NetworkObjects are despawned prior to being destroyed (if applicable).

MTT-1542

Also Fixes:
MTT-1254

This was discovered by the samples group and relates to:
MTT-1472

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 NetworkObjects not being despawned before they are destroyed during shutdown for client, host, and server instances.

Testing and Documentation

• Includes integration test that confirms this fix applies to client, host, and server instances.

[NoelStephensUnity]

Since this area has a high probability to generate a lot of spam, I reduced it to require the NetworkLog level be Developer as there can be several frames (during Update and FixedUpdate) where NetworkBehaviours will make checks that require a reference to the NetworkObject but it doesn't exist.

The question here is do we want to generate a warning here since NetworkObject can be null and should be valid to return a null value for m_NetworkObject.

[NoelStephensUnity]

@MFatihMAR
This was my solution to handle the issue where changing the parent would cause NetworkObject.OnTransformParentChanged to log an exception because NetworkManager is null during shutdown when the NetworkObjects are being despawned. Adding an additional series of checks in NetworkObject.OnTransformParentChanged seemed less-than-optimal since this is specific to only despawning.

[0xFA11]

if (NetworkManager == null || !NetworkManager.IsListening)
{
    transform.parent = m_CachedParent;
    Debug.LogException(new NotListeningException($"{nameof(NetworkManager)} is not listening, start a server or host before reparenting"));
    return;
}

is this the check you're referring to?

if so, we could split that logic up into 2 pieces: NetworkManager == null and NetworkManager.IsListening.
we'd return early if NetworkManager == null
we'd log exception if !NetworkManager.IsListening

also, I think we should not introduce the NetworkManager.ShutdownInProgress (see my other comment above) and also not inject this extra check here.

[NoelStephensUnity]

It is just below:

if (!NetworkManager.ShutdownInProgress)
{

Basically, if we a shutting down then ignore the parenting code...

[NoelStephensUnity]

Really, we should have a few states for the NetworkManager to provide the NetworkManager's current state.
This is a minimal fix in order to determine if we are in the middle of a shutdown.

[0xFA11]

instead of introducing this semi-fix/band-aid, what if we were to make NetworkManager.Singleton = null after we call NetworkManager.Shutdown (therefore destroy NOs first then set it to null)?

[NoelStephensUnity]

That would work when there is a single instance running, but our MultiInstsance tests would fail.

[0xFA11]

huh? why?

[NoelStephensUnity]

Because the NetworkManager.Singleton is one NetworkManager instance of (n) NetworkManager instances.
If you have 1 host and 4 clients running in a MultiInstance integration test and this code in NetworkManager is invoked during the shutdown of one of the clients:

        private void OnDestroy()
        {
            Shutdown();

            UnityEngine.SceneManagement.SceneManager.sceneUnloaded -= OnSceneUnloaded;

            if (Singleton == this)
            {
                Singleton = null;
            }
        }

But the NetworkManager of the client shutting down is not the same as the one assigned to the NetworkManager.Singleton, then it will not be set to null. Forcing it to null in the integration/unit test could lead to other issues where code may depend upon the Singleton. If only 1 client shuts down but the rest and the host stay active but we set the singleton to null... it would be not only introducing another semi-fix/band-aid...but the semi-fix/band-aid would be using a property for a purpose it was not intended and very likely would require us to fix other issues that might crop up by setting it to null while other NetworkManagers were still connected and active.

I didn't like adding yet another Boolean value to describe a "state of the NetworkManager". and is why I was thinking we might want to discuss coming up with some states for NetworkManager that let us know (some we already know but are various Boolean properties as opposed to just a single enum value):

• The NetworkManager is being started
• The NetworkManager is being initialized
• The NetworkManager is connected at the transport
• The NetworkManager is connected to network session
• The NetworkManager is disconnecting the transport
• The NetworkManager is shutting down
• The NetworkManager is shut down.
  Perhaps not all of the above, but in that general direction.

[0xFA11]

I'm not suggesting to force NetworkManager.Singleton = null, I'm saying if we call network destroy after something sets NetworkManager.Singleton = null then let's call network destroy on objects first and later let that other code do NetworkManager.Singleton = null

[NoelStephensUnity]

Well, either case it appears we have another bool that signals the shutdown process that popped up recently.
For now, I will just make it this:
public bool ShutdownInProgress { get { return m_ShuttingDown; } }

I could make it internal but then users wouldn't be able to check this value for any edge case scenarios, which I could just use the m_ShuttingDown property but make it internal which would require me to change it to just ShuttingDown.

[unity-cla-assistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[0xFA11]

NetworkManager.Singleton check should be another if-check with potentially another log message IMO.

[NoelStephensUnity]

Will revisit this.

[0xFA11]

instead of introducing this semi-fix/band-aid, what if we were to make NetworkManager.Singleton = null after we call NetworkManager.Shutdown (therefore destroy NOs first then set it to null)?

[0xFA11]

if (NetworkManager == null || !NetworkManager.IsListening)
{
    transform.parent = m_CachedParent;
    Debug.LogException(new NotListeningException($"{nameof(NetworkManager)} is not listening, start a server or host before reparenting"));
    return;
}

is this the check you're referring to?

if so, we could split that logic up into 2 pieces: NetworkManager == null and NetworkManager.IsListening.
we'd return early if NetworkManager == null
we'd log exception if !NetworkManager.IsListening

also, I think we should not introduce the NetworkManager.ShutdownInProgress (see my other comment above) and also not inject this extra check here.