fix: client not correctly disconnected during character selection

[LPLafontaineB]

Description (*)

This PR resolves the issue causing clients to not be correctly disconnected when they exit a game during character selection, if the game was started by a host who previously started another game and exited during character selection.

Related Pull Requests

Issue Number(s) (*)

Fixes issue(s): MTT-1507

Manual testing scenarios

1. ...
2. ...

Questions or comments

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[LPLafontaineB]

This doesn't look like the right way to fix the issue. Basically what is happening is that when a host exits during character selection, it asks the gameNetPortel to shutdown the netmanager, which does not call the OnDespawn method of ServerCharSelectState. This leads to the callbacks not unregistering from the netmanager, which means that during a new game, those callbacks that now point to a detroyed object are still registered to the netmanager and are called. This is what caused the null references.

Other solutions would be to move what is in the OnDespawn to the OnDestroy event, which would be called when switching scenes, or maybe to have the netmanager call the OnDespawn events of networkobjects when it is shutdown or to unregister all its callbacks itself.
I'm not sure if it is something that should be our responsibility or the netmanager's

[fernando-cortez]

At first glance, I would think that OnNetworkDespawn() should be fired on NetworkObjects when the network is shutdown. I'm not sure if we should manually have to do this game-side, either.

Doing the cleanup on OnNetwokDespawn() and on OnDestroy() for ServerCharSelectState would work here though since we just transition scenes after shutting down the network.

[NoelStephensUnity]

When NetworkObjects are destroyed is part of a deeper issue in regards to spawning and despawning. There are scenarios where users might not want to destroy NetworkObjects when the NetworkManager shuts down (i.e. object pools are one good example).
This GitHub issue-1054 was resolved in PR-1183.

What we may need here is the ability to mark NetworkObjects to persist after the NetworkManager is shutdown (perhaps the default setting is that they don't persist) and then that way we could destroy all NetworkObjects that do not have that setting while also keeping those that do? (We might want to create a user story for this)

[LPLafontaineB]

I'm not sure it is the same issue. In this specific case, we load a new scene right after, so the object will be destroyed anyway, which is what was intended. The issue is that the callbacks that we added to the net manager's events are currently removed in the OnNetworkDespawn() event, which is not called when the net manager is shut down. Then, when we host a new game, the callbacks of the now destroyed object are still called.

[SamuelBellomo]

The linked PR above is changing tests only.
Is there a fix incoming for this? @NoelStephensUnity please note we shouldn't fix things sample side that are going to get fixed soon by a new SDK update. Will a fix for this come in a 1.x soon?

[NoelStephensUnity]

@SamuelBellomo
So I have been mulling over this issue...need to see if there is a Jira ticket already open for it... but it definitely has to do with the NetworkManager shutdown process.
I think something like this in NetworkSpawnManager should do the trick:

internal void DespawnNetworkObjects()
{
    var networkObjects = UnityEngine.Object.FindObjectsOfType<NetworkObject>();

    for (int i = 0; i < networkObjects.Length; i++)
    {
        if (networkObjects[i].NetworkManager == NetworkManager)
        {
            if (NetworkManager.PrefabHandler.ContainsHandler(networkObjects[i]))
            {
                NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(networkObjects[i]);
                // Leave destruction up to the handler
                OnDespawnObject(networkObjects[i], false);
            }
            else
            {
                // If it is an in-scene placed NetworkObject then just despawn
                // and let it be destroyed when the scene is unloaded.  Otherwise,
                // despawn and destroy it.
                var shouldDestroy = !(networkObjects[i].IsSceneObject != null
                            && networkObjects[i].IsSceneObject.Value);

                OnDespawnObject(networkObjects[i], shouldDestroy);
            }
        }
    }
}

Then in the NetworkManager.Shutdown:

if (SpawnManager != null)
{
    //SpawnManager.DestroyNonSceneObjects();
    SpawnManager.DespawnNetworkObjects();
    SpawnManager.ServerResetShudownStateForSceneObjects();

    SpawnManager = null;
}

I believe this will fix the issues you are experiencing. I already tested the above code adding an override for OnNetworkDespawn in the NetworkPrefabHandlerAdditive class and after loading an additive scene with that component and then exiting back to the main menu (i.e. immediately shutting down) it does invoke the OnNetworkDespawn method:

If I uncomment the DestroyNonSceneObjects() and comment out the DespawnNetworkObjects() in the NetworkManager.Shutdown it does not invoke.

Will look to see if there is an already existing Jira Ticket and if not I will open one up and include the above in a PR for this issue along with tests to validate ( which I need to noodle over that a little ).

While I do that, it would help if someone could temporarily apply the above suggested fix to a version of Boss Room that doesn't have this fix (or revert back the singleton reference) and see if this indeed fixes this issue?

[NoelStephensUnity]

PR-1390 has several updates (including an update to the above code) that are required in many cases but some need to be discussed to determine the best approach... but if you wanted to see if Boss Room would work...test it with the PR's branch.

I will also test it on my end sometime tomorrow.

[LPLafontaineB]

I just tested with the PR branch and it seems to work, I cannot reproduce the bug.

[NoelStephensUnity]

Need to wait until @MFatihMAR returns from vacation since some of the adjustments made in PR-1390 makes adjustments to some areas he is the T.O. of and so the earliest it will get merged will be sometime late next week.
Not sure when the next backport is slated, but hopefully 1390 will make it out in the next one.

[LPLafontaineB]

Issue fixed on SDK side. Closing this PR