fix: client owned NetworkObject with prefabhandler destroy order incorrect on host-server side (Backport)

[NoelStephensUnity]

This is a back-port of #3200

Replication of the issue is as follows:

• Using a client-server network topology
  • At least one client must be connected to a host or server.
• NetworkObjects are spawned with:
  • Ownership belonging to a connected client.
    • Does not occur if they are owned by the server.
  • Don't destroy with owner is not enabled (i.e. it will destroy when the client leaves)
    • Does not occur if don't destroy with owner is enabled.
• While the NetworkObjects (owned by the client) are still spawned, the client disconnects.

NGO V1: On the host/server side, the Destroy method is called prior to the OnNetworkDespawn.

This also includes:

• Improvements to integration test helpers and some fixes to NetworkObject.InstantiateAndSpawn.
• Bug fixes for NetworkObject.InstantiateAndSpawn.
• NetworkPrefabOverrideTests was backported from v2.

fix: #3190

Changelog

Fixed: Issue where a spawned NetworkObject that was registered with a prefab handler and owned by a client would invoke destroy more than once on the host-server side if the client disconnected while the NetworkObject was still spawned.
Fixed: Issue where NetworkObject.InstantiateAndSpawn would not honor the isPlayerObject parameter.
Fixed: Issue where NetworkObject.InstantiateAndSpawn would not make an instance of a network prefab but would spawn the network prefab itself.

Testing and Documentation

• Includes integration test: NetworkPrefabOverrideTests.
• No documentation changes or additions were necessary.

[EmandM]

Just a thought, this is a lot of nested ifs. What are your thoughts on nested ifs vs early exits in this type of Netcode context?

[NoelStephensUnity]

Well, I wouldn't exit early because there is other code that needs to run but I did change this to check if it is null and if so then it just continues.

Nesting doesn't bother me personally and in the end the op codes generated are pretty much identical where it ends up jumping back to the top and grabbing the next element (continue) or returning from the method (exit early).

However, it did remove one layer of a nested if which can be easier to read for users...so that is a good catch.

[EmandM]

Oh, does the backport need a CHANGELOG entry?

[NoelStephensUnity]

Oh, does the backport need a CHANGELOG entry?

Yes indeed it did. Thanks for the reminder. It is pushed.