refactor!: NetworkPrefabHandler and INetworkPrefabInstanceHandler API & XmlDoc changes #977

NoelStephensUnity · 2021-07-23T20:09:43Z

Improved comments and added additional XML Documentation for the NetworkPrefabHandler and associated INetworkPrefabInstanceHandler.
Renamed the two methods within INetworkPrefabInstanceHandler to better reflect what their function was and to avoid user confusion.
Provided the ability for Hosts to properly display NetworkPrefab overrides.
Now when hosts spawn the NetworkPrefab override, messages to clients about the overridden NetworkObject will send the source NetworkPrefab's GlobalObjetIdHash value as opposed to the overriden NetworkPrefab's.
The above two refactors are applied to NetworkPrefab Handlers
Host can now register 1 or more NetworkPrefab overrides with the NetworkPrefabHandler, which allows for the destory method of the INetworkPrefabInstanceHandler implementation to be invoked on the host.
Removed areas where we were recursively calling OnDespawnObject where the prefab message handler was detected on a NetworkObject.
NetworkPrefabPool has been updated to reflect some of the above changes
Improved the GenericNetworkObjectBehaviour and adjusted it to account for some of the above changes as well as clients now will no longer have a "flicker" in re-cycled and re-spawned NetworkObjects that was due to latency in the new NetworkTransform position (I think when snapshot handles spawning and despawning this could be removed).
Added new method NetworkManager.GetNetworkPrefabOverride that is specific to Prefab Handler Overrides, Host mode, and the above changes (although there could be other uses for this)

Renamed the two methods within INetworkPrefabInstanceHandler to better reflect what their function was and to avoid user confusion. Improved comments and added additional XML Documentation for the NetworkPrefabHandler and associated INetworkPrefabInstanceHandler

Minor updates on the ClientInstantiateOverride parameter names

0xFA11 · 2021-07-26T02:27:48Z

com.unity.multiplayer.mlapi/Runtime/Spawning/NetworkPrefabHandler.cs

+        NetworkObject ClientInstantiateOverride(ulong ownerClientId, Vector3 position, Quaternion rotation);
+
+        /// <summary>
+        /// Client and Server
+        /// Once an implementation is registered with the <see cref="NetworkPrefabHandler"/>, this method will be called every time
+        /// a Network Prefab associated <see cref="NetworkObject"/> is destroyed.
+        /// If <see cref="NetworkObject.Despawn(bool)"/> is invoked with the default destroy parameter (i.e. false) then this method will not be invoked.
+        ///
+        /// Note on Pooling: When invoked, you do not need to despawn or destroy the NetworkObject as long as you want your pool to persist.
+        /// The most common approach is to make the <see cref="NetworkObject"/> inactive by calling <see cref="GameObject.SetActive(bool)"/>.
+        /// </summary>
+        /// <param name="networkObject">The <see cref="NetworkObject"/> being destroyed</param>
+        void DestroyOverride(NetworkObject networkObject);


I'm not sure if I'd go for ClientInstantiateOverride & DestroyOverride instead of HandleNetworkPrefabSpawn & HandleNetworkPrefabDestroy under an interface called INetworkPrefabInstanceHandler.

I think I'd drop "-Override" suffix and keep "Handle-" prefix around.

Also "Client-" prefix sounds a little risky to me in terms of our future potential and its API-breaking nature.
We might expand this feature's impact area and even make it work for both client and server sides. That would require another API-breaking change just to drop "Client-" prefix or we might have to add another super similar API that starts with "Server-" prefix to not break the old API (which would not be ideal IMHO).

Only thing I'd change here would be HandleNetworkPrefabDestroy → HandleNetworkPrefabDespawn to match NetworkBehaviour.OnNetworkSpawn() and NetworkBehaviour.OnNetworkDespawn() naming convention.

Also, it'd be tempting to keep client-specific comments around but I think they should go away and make feature non-client specific. Client-specific comment may go under NetworkPrefabHandler since it is the source where client-specific nature is coming from (and even that might change in the future to cover both client and server side, hence my earlier comments above).

So the old naming here was very inconsistent. Spawn, Destroy don't match. We either want HandleNetworkPrefabDespawn or rename the Spawn to Instantiate.

Now the reason we want for Instantiate/Destroy was is because you can spawn/despawn a networkobject in MLAPI without triggering these handlers (GameObject.Destroy on the server for instance skips the handler). And what they really do is override a GameObject Instatiate / Destroy function and replace it with something else.

I prefer the Override suffix over Handle prefix. Because HandleNetworkPrefabDestroy does not indicate whether this handler is run in addition to the destroy or instead of the destroy. Dropping the Client prefix on InstantiateOverride sounds good if we consider future potential.

so it's:

HandleNetworkPrefabSpawn

HandleNetworkPrefabDespawn

vs

InstantiateOverride

DestroyOverride

:)

I believe it's fair to say that we'd go for Instantiate/Destroy.

so my final suggestion would be:

HandleInstantiate

HandleDestroy

:P

HandleInstantiate, HandleDestroy sounds good to me 👍

The only issue I have with "HandleInstantiate" is that this is only invoked on the client side and could be misleading. However, that can be made clear in documentation.

How about we even further simplify this? I mean it is a "Handler" so not sure we have to put "handle" in front of all of the methods. Why not:

Instantiate

Destroy

this is only invoked on the client side

hence my first comment, it is currently client-only but we could expand it to cover both server and client.

it is a "Handler" so ...

I think that's also a fair point. INetworkPrefabInstanceHandler.Instantiate & INetworkPrefabInstanceHandler.Destroy it is :)

Renamed the interface methods. Updated comments. Refactored NetworkPrefabHandler to handle GitHub Issue 990 as well as made adjustments to be able to send the appropriate GlobalObjectIdHash as a host. This also includes a public facing method in the NetworkManager to be able to lookup a NetworkManager defined NetworkPrefab Override for some associated cases relating to Host mode and NetworkPrefabHandlers and NetworkPrefab Overrides. Made adjustments to the SceneTransitioning test and associated scenes and assets.

0xFA11 · 2021-07-28T20:26:12Z

testproject/Assets/Tests/Manual/Scripts/GenericNetworkObjectBehaviour.cs

@@ -86,8 +86,7 @@ private void OnTriggerEnter(Collider other)
                }
                else
                {
-                    NetworkObject.Despawn();
-                    NetworkObject.gameObject.SetActive(false);


did we drop this intentionally?
I'm talking about NetworkObject.gameObject.SetActive(false);

Review it again as I just updated.
It should make more sense...

0xFA11 · 2021-07-28T20:27:54Z

testproject/Assets/Tests/Manual/Scripts/NetworkPrefabPool.cs

@@ -85,6 +82,21 @@ private void RegisterCustomPrefabHandler()
            }
        }

+
+        private void DeRegisterCustomPrefabHandler()


super-nit :)

Suggested change

private void DeRegisterCustomPrefabHandler()

private void DeregisterCustomPrefabHandler()

Added the case where no prefab handler override is registered but there is still a host. Added a lookup table to quickly get the proper source GlobalObjectIdHash value for the NetworkPrefab that has an override in the NetworkManager. The first scene in the SceneTransitioningTest uses a NetworkManager defined prefab override, in host mode the host will pool while clients will not pool (no need since there is no way to override the instantiation) and the host will always send the source NetworkPrefab's GlobalObjectIdHash value as opposed to the target override's GlobalObjectIdHash. Fixed Fatih's super-nit-pick :)

Removing an artifact property from a previous merge.

Fixing missing change to deregister and issue with despawning during fixed update.

Need to reset the position for the object when no handler exists otherwise the trigger will fire immediately and cause it to despawn immediately.

Removing despawn replication and from places where they are no longer needed either due to the now proper message ordering. Updated the GenericNetowrkObjectBehaviour such that clients disable the MeshRenderer component and then have a delayed re-enabling of it upon being spawned in order to avoid the latent update in the transform's position (removes the flickering of NetworkObjects on the client). Added the ability to make the SwitchSceneHandler have a delayed "auto switch" in order to run cycled long burn runs to assure everything is working "as expected". Fixed an access violation in StatsDisplay ( minor thing that was bugging me).

0xFA11

I'm seeing a few other changes going in after I approved this PR.
I think I should re-review this again some time — please hmu when you think it's ready to be reviewed again :)

NoelStephensUnity · 2021-07-28T23:39:09Z

com.unity.multiplayer.mlapi/Runtime/Spawning/NetworkSpawnManager.cs

@@ -504,14 +504,14 @@ internal void ServerDestroySpawnedSceneObjects()
                    // This **needs** to be here until we overhaul NetworkSceneManager due to dependencies
                    // that occur shortly after NetworkSceneManager invokes ServerDestroySpawnedSceneObjects
                    // within the NetworkSceneManager.SwitchScene method.
-                    SpawnedObjectsList.Remove(sobj);
+


This is really a short term change and will be updated in PR-955 Additive Scene Loading.

NoelStephensUnity · 2021-07-28T23:41:55Z

com.unity.multiplayer.mlapi/Runtime/Spawning/NetworkSpawnManager.cs

@@ -559,7 +559,10 @@ internal void DestroySceneObjects()
                        if (NetworkManager.PrefabHandler.ContainsHandler(networkObjects[i]))
                        {
                            NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(networkObjects[i]);
-                            OnDespawnObject(networkObjects[i], false);
+                            if (SpawnedObjects.ContainsKey(networkObjects[i].NetworkObjectId))


More short term updates until PR-955.

Refactored a better client side "no blinking when spawning" solution. Fixed an issue where everyone was sending destroy messages to everyone else.

NoelStephensUnity · 2021-07-29T14:58:15Z

com.unity.multiplayer.mlapi/Runtime/Spawning/NetworkSpawnManager.cs

@@ -504,14 +504,14 @@ internal void ServerDestroySpawnedSceneObjects()
                    // This **needs** to be here until we overhaul NetworkSceneManager due to dependencies
                    // that occur shortly after NetworkSceneManager invokes ServerDestroySpawnedSceneObjects
                    // within the NetworkSceneManager.SwitchScene method.
-                    SpawnedObjectsList.Remove(sobj);
+
                    if (NetworkManager.PrefabHandler != null && NetworkManager.PrefabHandler.ContainsHandler(sobj))
                    {
                        NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(sobj);


This I am leaving up to the user to handle how they want to deal with scene transitions and prefab handler overrides.

NoelStephensUnity · 2021-07-29T15:06:33Z

com.unity.multiplayer.mlapi/Runtime/Spawning/NetworkSpawnManager.cs

@@ -681,7 +684,6 @@ internal void OnDespawnObject(NetworkObject networkObject, bool destroyGameObjec
                if (NetworkManager.PrefabHandler.ContainsHandler(networkObject))
                {
                    NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(networkObject);
-                    OnDespawnObject(networkObject, false);


@LukeStampfli , I could you double check this? I did some testing on my end and I think this is no longer needed with the more recent updates (I think this was for some message ordering issue, but that should be fixed now).
Gladly will revert this is you think there is a special case for having that OnDespawnObject here.

I tried understand this but not sure why we had it. It does a recursive call into OnDespawnObject again for some reason I can't find a reason why we would want to do that. So from what I know this is not needed?

Added a more simplified example that uses a INetworkPrefabInstanceHandler implementation as a 1:1 NetworkObject pool.

LukeStampfli

Looks all good and great XML docs!

Fixed issue with spawning: - Sending to every client for each client within the observers list - Making the client ownership equal to the client it was sending to This fixes the issue with connecting more than 1 client to a host or server.

LukeStampfli

The additional fix to the SendSpawnCallForObject looks good.

This updates the object pooling example to reflect the changes from PR-977: Unity-Technologies/com.unity.netcode.gameobjects#977

* refactor and updates This updates the object pooling example to reflect the changes from PR-977: Unity-Technologies/com.unity.netcode.gameobjects#977 * additional notes This adds additional information about using more than one prefab with an override. This also adds a link to the to-be-existing folder in the master branch. * refactor Removed some code from advanced example that was not being used. * refactor Updating to most recent changes from PR-955 (yet to be merged) * refactor making changes to reflect the final version that was merged into the develop branch. Co-authored-by: Briancoughlin <76997526+Briancoughlin@users.noreply.github.com>

* refactor and updates This updates the object pooling example to reflect the changes from PR-977: Unity-Technologies/com.unity.netcode.gameobjects#977 * additional notes This adds additional information about using more than one prefab with an override. This also adds a link to the to-be-existing folder in the master branch. * refactor Removed some code from advanced example that was not being used. * refactor Updating to most recent changes from PR-955 (yet to be merged) * MTTDOC-317 initial page creation * #MTTDOC-317 update to main page * refactor making changes to reflect the final version that was merged into the develop branch. * MTTDOC-317 updating to ensure successful build * MTTDOC-317 fixing some typos and naming * update Ran through this one more time removing any references to MLAPI. I also tried to improve upon the text copy flow (might need a second pair of eyes) * update minor language tweak and italicized the reference to object pooling link * update left out the trailing * * Update object-pooling.md * Update object-pooling.md * Update object-pooling.md * Delete networktime-ticks.md Co-authored-by: Brian Coughlin <brian.coughlin@unity3d.com> Co-authored-by: Briancoughlin <76997526+Briancoughlin@users.noreply.github.com>

This updates the object pooling example to reflect the changes from PR-977: Unity-Technologies/com.unity.netcode.gameobjects#977

* refactor and updates This updates the object pooling example to reflect the changes from PR-977: Unity-Technologies/com.unity.netcode.gameobjects#977 * additional notes This adds additional information about using more than one prefab with an override. This also adds a link to the to-be-existing folder in the master branch. * refactor Removed some code from advanced example that was not being used. * refactor Updating to most recent changes from PR-955 (yet to be merged) * refactor making changes to reflect the final version that was merged into the develop branch. Co-authored-by: Briancoughlin <76997526+Briancoughlin@users.noreply.github.com>

* refactor and updates This updates the object pooling example to reflect the changes from PR-977: Unity-Technologies/com.unity.netcode.gameobjects#977 * additional notes This adds additional information about using more than one prefab with an override. This also adds a link to the to-be-existing folder in the master branch. * refactor Removed some code from advanced example that was not being used. * refactor Updating to most recent changes from PR-955 (yet to be merged) * MTTDOC-317 initial page creation * #MTTDOC-317 update to main page * refactor making changes to reflect the final version that was merged into the develop branch. * MTTDOC-317 updating to ensure successful build * MTTDOC-317 fixing some typos and naming * update Ran through this one more time removing any references to MLAPI. I also tried to improve upon the text copy flow (might need a second pair of eyes) * update minor language tweak and italicized the reference to object pooling link * update left out the trailing * * Update object-pooling.md * Update object-pooling.md * Update object-pooling.md * Delete networktime-ticks.md Co-authored-by: Brian Coughlin <brian.coughlin@unity3d.com> Co-authored-by: Briancoughlin <76997526+Briancoughlin@users.noreply.github.com>

refactor and style

311875b

Renamed the two methods within INetworkPrefabInstanceHandler to better reflect what their function was and to avoid user confusion. Improved comments and added additional XML Documentation for the NetworkPrefabHandler and associated INetworkPrefabInstanceHandler

NoelStephensUnity requested review from 0xFA11 and LukeStampfli July 23, 2021 20:10

style

e0f6143

Minor updates on the ClientInstantiateOverride parameter names

0xFA11 changed the title ~~refactor: NetworkPrefabHandler and INetworkPrefabInstanceHandler clarity and xml doc updates~~ refactor!: NetworkPrefabHandler and INetworkPrefabInstanceHandler API & XmlDoc changes Jul 26, 2021

0xFA11 reviewed Jul 26, 2021

View reviewed changes

NoelStephensUnity added 2 commits July 28, 2021 11:41

Merge branch 'develop' into refactor/INetworkPrefabInstanceHandler

c4e7f82

NoelStephensUnity requested a review from 0xFA11 July 28, 2021 19:57

Merge branch 'develop' into refactor/INetworkPrefabInstanceHandler

112a377

0xFA11 reviewed Jul 28, 2021

View reviewed changes

0xFA11 approved these changes Jul 28, 2021

View reviewed changes

0xFA11 reviewed Jul 28, 2021

View reviewed changes

NoelStephensUnity added 5 commits July 28, 2021 15:53

fix

68fb3bd

Removing an artifact property from a previous merge.

fix

8a9f273

Fixing missing change to deregister and issue with despawning during fixed update.

fix

aa97122

Need to reset the position for the object when no handler exists otherwise the trigger will fire immediately and cause it to despawn immediately.

0xFA11 suggested changes Jul 28, 2021

View reviewed changes

NoelStephensUnity requested a review from 0xFA11 July 28, 2021 23:31

NoelStephensUnity commented Jul 28, 2021

View reviewed changes

NoelStephensUnity mentioned this pull request Jul 28, 2021

INetworkPrefabInstanceHandler not called on server #990

Closed

fix and refactor

410f16c

Refactored a better client side "no blinking when spawning" solution. Fixed an issue where everyone was sending destroy messages to everyone else.

0xFA11 approved these changes Jul 29, 2021

View reviewed changes

NoelStephensUnity commented Jul 29, 2021

View reviewed changes

refactor

0839818

Added a more simplified example that uses a INetworkPrefabInstanceHandler implementation as a 1:1 NetworkObject pool.

LukeStampfli approved these changes Jul 29, 2021

View reviewed changes

fix

2047439

Fixed issue with spawning: - Sending to every client for each client within the observers list - Making the client ownership equal to the client it was sending to This fixes the issue with connecting more than 1 client to a host or server.

NoelStephensUnity requested review from 0xFA11 and LukeStampfli July 29, 2021 18:43

Merge branch 'develop' into refactor/INetworkPrefabInstanceHandler

641352e

ShadauxCat approved these changes Jul 29, 2021

View reviewed changes

LukeStampfli approved these changes Jul 29, 2021

View reviewed changes

NoelStephensUnity merged commit d6b2a48 into develop Jul 29, 2021

NoelStephensUnity deleted the refactor/INetworkPrefabInstanceHandler branch July 29, 2021 19:06

NoelStephensUnity added a commit to Unity-Technologies/com.unity.multiplayer.docs that referenced this pull request Jul 30, 2021

refactor and updates

8d3616d

This updates the object pooling example to reflect the changes from PR-977: Unity-Technologies/com.unity.netcode.gameobjects#977

This was referenced Jul 30, 2021

refactor and updates Unity-Technologies/com.unity.multiplayer.docs#246

Merged

test: network prefab pools and INetworkPrefabInstanceHandler #1004

Merged

Briancoughlin pushed a commit to Unity-Technologies/com.unity.multiplayer.docs that referenced this pull request Sep 14, 2021

refactor and updates

fbf6412

This updates the object pooling example to reflect the changes from PR-977: Unity-Technologies/com.unity.netcode.gameobjects#977

	private void DeRegisterCustomPrefabHandler()
	private void DeregisterCustomPrefabHandler()

refactor!: NetworkPrefabHandler and INetworkPrefabInstanceHandler API & XmlDoc changes #977

refactor!: NetworkPrefabHandler and INetworkPrefabInstanceHandler API & XmlDoc changes #977

Uh oh!

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

0xFA11 Jul 28, 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!

LukeStampfli Jul 29, 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!

LukeStampfli left a comment

Choose a reason for hiding this comment

Uh oh!

LukeStampfli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NoelStephensUnity commented Jul 23, 2021 •

edited

Loading

0xFA11 Jul 28, 2021 •

edited

Loading

LukeStampfli Jul 29, 2021 •

edited

Loading