fix: NetworkSceneManager exception when DontDestroyOnLoad NetworkObjects are being synchronized #1090

NoelStephensUnity · 2021-08-25T19:07:18Z

MTT-1136
This fix pertains to the following GitHub-1088 issue.
This fix includes a validating manual test check for Github-701 issue in testproject\Assets\Tests\Manual\SceneTransitioning\SceneTransitioningTest that demonstrates how the NetworkSceneManager.OnSceneEvent can be used to detect when clients are ready to receive RPCs.

This fixes the dont destroy on load issue in NetworkSceneManager

This provides manual tests to verify that don't destroy on load will work when NetworkManger's don't destroy property is enabled or disabled.

This should fix all scenarios with one scenario causing a warning message that might not be needed but I am leaving it in until I get a consensus on handling the scenario where a NetworkObject "will" be moved to the DontDestroyOnLoad scene but during the initial syncrhonization it does not exist.

Changed some of the names used, added and updated comments, and applied the changes to the assets based on the name changes.

…/github.com/Unity-Technologies/com.unity.netcode.gameobjects into fix/develop-dontdestroy-networkscenemanager

VALERE91Unity · 2021-08-25T19:24:21Z

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

@@ -245,13 +245,41 @@ internal void SetTheSceneBeingSynchronized(int serverSceneHandle)
                // If the scene was not found (invalid) or was not loaded then throw an exception


Maybe removing this comment since we are not throwing anymore

Adding the unit test to verify the baseline DontDestroyOnLoad scene synchronization is working when NetworkManager.DestroyOnLoad is enabled and disabled.

updating comments based on PR suggestions.

mattwalsh-unity · 2021-08-25T21:16:32Z

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

                if (!SceneBeingSynchronized.IsValid() || !SceneBeingSynchronized.isLoaded)
                {
-                    throw new Exception($"[{nameof(NetworkSceneManager)}- {nameof(ScenesLoaded)}] Could not find the appropriate scene to set as being synchronized!");
+                    // Let's go ahead and use the currently active scene under the scenario where a NetworkObject is determined to exist in a scene that the NetworkSceneManager is not aware of


So in the older code, we halted and caught fire if 1) the scene being sync'd was invalid or 2) it was not loaded.

Case 1) Seems like we want to still halt and catch fire for situation 1). Changing to some other scene would seem to mask a problem that needs to be dealt with. I mean, why would we think we would want to pop back to the active scene if the scene we're going to is invalid.

Case 2) same kind of thing; if the scene is valid but not loaded yet, perhaps there is another handling - I'm just not sure why we'd just pick whatever scene happens to be loaded now

This is specific to in-scene placed NetworkObjects during the client side deserialization phase for:

Newly joined client synchronization where all scenes are already loaded and the client is just running through the in-scene placed NetworkObjects (deserialization) to be instantiated.

A level is loaded and the client is synchronizing in-scene placed NetworkObjects.

The only time the offending method (SetTheSceneBeingSynchronized) is truly pertinent is when you have the same additive scene loaded multiple times and there are in-scene NetworkObjects in each instance that need to be synchronized. We use scene handlers to distinguish between "duplicate in-scene NetworkObjects" in order to "find the right NetworkObject" so we can apply the appropriate NetworkObjectId to that specific instance.
For all other cases, the GlobalObjectIdHash should be unique and will not matter really which scene is the current scene being synchronized.
What scenario would cause us to use the currently active scene?
If you set NetworkManager.DontDestroy to false and then have a NetworkObject that moves itself into the DontDestroyOnLoad scene when it is instantiated, then when a client is trying to "synchronize" to this specific scenario the first NetworkObject the client is trying to deserialize with this kind of behavior will not have a DontDestroyOnLoad scene (yet) and so it is perfectly fine to return the currently active scene. Upon that first NetworkObject instantiating (after deserialization) the DontDestroyOnLoad scene will exist so any other NetworkObject being deserialized (with the same behavior) on the client from that point forward will set the DontDestroyOnLoad scene to be the "scene being synchronized".

It is one of those chicken or the egg scenarios:
DontDestroyOnLoad is only created when you move a GameObject into that scene using the GameObject.DontDestroyOnLoad method. If nothing has been moved into the DontDestroyOnLoad scene, then it wont exist and isn't something that can be instantiated. I had thought about adding a chunk of extra code that would tell the client to "pre-seed" the DontDestroyOnLoad scene (i.e. create a temporary GameObject before deserialization and then destroy it afterwards) but that seemed a bit over the top. :)

I think I may just need to trust you on this one, not sure I can parse this out / come to understand all the related systems as quickly as we need

This was refactored a bit and might make more sense at this point. But yeah.., there are a lot of different possible scenarios to consider for sure.

mattwalsh-unity · 2021-08-25T21:20:51Z

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

+                }
+                else
+                {
+                    // The next scenario is if a user intentionally moves NetworkObjects into a DontDestroyOnLoad scene


When we say "user intentionally moves NO into a DDOL", is this like the user is manipulating objects manually in the editor while things are running? I guess I didn't think of this as something we needed to explicitly support. Or is this if code moves a NO in the DDOL scene?

It is if the user's code intentionally moves it into the DDOL scene.
If you had a NetworkBehaviour that had this code in it:

/// <summary> /// When enabled, we move ourself to the DontDestroyOnLoad scene /// </summary> private void OnEnable() { DontDestroyOnLoad(this); }

Then it would get moved into the DDOL Scene when it was instantiated... and that would apply to both dynamically spawned and in-scene placed NetworkObjects.

Fixing renamed method.

testproject/Assets/Tests/Manual/DontDestroyOnLoad/DontDestroyOnLoadSpawnHandler.cs

testproject/Assets/Tests/Manual/DontDestroyOnLoad/ObjectToNotDestroyBehaviour.cs

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

Verifying alternate DontDestroyOnLoad test case mixed with checking when we can or cannot send RPCs to a client.

Cleaning up our DDOL GameObject.

Removed the NetworkObject property and assignment code from the PlayerMovementManager as it is not needed. Updated comments in NetworkSceneManager regarding DDOL.

JesseOlmer · 2021-08-27T21:22:28Z

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

+            else // Otherwise, we have to create a GameObject and move it into the DDOL to get the scene
+            {
+
+#if UNITY_EDITOR || DEVELOPMENT_BUILD


Use UNITY_INCLUDE_TESTS instead of these...

@JesseOlmer I am going to fix those in PR-1080 if that is ok?

andrews-unity · 2021-08-27T21:27:12Z

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

+        /// <summary>
+        /// Handle cleaning up the DDOL GameObject
+        /// </summary>
+        public void Dispose()


Is this called anywhere I could not see it being called?

When it is destroyed?
public class NetworkSceneManager:IDisposable

This is removed with the most recent commit.

andrews-unity · 2021-08-27T21:27:48Z

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

+#endif
+                // Create our DDOL GameObject and move it into the DDOL scene so we can register the DDOL with
+                // the NetworkSceneManager.
+                m_DDOLObject = new GameObject("DDOL-NWSM");


can we just cache off the scene handle and then destroy the object right away? thus not polluting the scene with a useless object ?

Let me see if that works.

It indeed did work to do it that way! ⚡ ZAPPED that code out of there!

We don't need to keep the DDOL object around after we have the DDOL scene. Removed IDisposable from NetworkSceneManager Removed the private m_DDOLObject Removed the Dispose method.

…nsform * develop: (26 commits) fix: client connected InvokeOnClientConnectedCallback with scene management disabled (#1123) fix: removed `public` class `NetcodeObserver` (MTT-1157) (#1122) feat: add NetworkMessageSent/Received metrics (#1112) feat: snapshot. MTU sizing option for Snapshot. MTT-1087 (#1111) Add metrics for transport bytes sent and received (#1104) fix: Missing end profiling sample (#1118) chore: support standalone mode for netcode runtimetests (#1115) feat: Change MetricNames for a more complex value type (#1109) feat: Track scene event metrics (#1089) style: whitespace fixes (#1117) feat: replace scene registration with scenes in build list (#1080) fix: mtt-857 GitHub issue 915 (#1099) fix: NetworkSceneManager exception when DontDestroyOnLoad NetworkObjects are being synchronized (#1090) feat: NetworkTransform Custom Editor Inspector UI (#1101) refactor: remove TempGlobalObjectIdHashOverride (#1105) fix: MTT-1124 Counters are now reported in sync with other metrics (#1096) refactor: convert using var statements to using var declarations (#1100) chore: updated all of the namespaces to match the tools package change (#1095) refactor!: remove network variable settings, network behaviour cleanup (#1097) fix: mtt-1088 review. Safer handling of out-of-order or old messages (#1091) ... # Conflicts: # com.unity.netcode.gameobjects/Prototyping/NetworkTransform.cs

…cts are being synchronized (Unity-Technologies#1090) * fix This fixes the dont destroy on load issue in NetworkSceneManager * test This provides manual tests to verify that don't destroy on load will work when NetworkManger's don't destroy property is enabled or disabled. * fix This should fix all scenarios with one scenario causing a warning message that might not be needed but I am leaving it in until I get a consensus on handling the scenario where a NetworkObject "will" be moved to the DontDestroyOnLoad scene but during the initial syncrhonization it does not exist. * style and refactor Changed some of the names used, added and updated comments, and applied the changes to the assets based on the name changes. * test Adding the unit test to verify the baseline DontDestroyOnLoad scene synchronization is working when NetworkManager.DestroyOnLoad is enabled and disabled. * style updating comments based on PR suggestions. * fix Fixing renamed method. * fix Verifying alternate DontDestroyOnLoad test case mixed with checking when we can or cannot send RPCs to a client. * refactor Cleaning up our DDOL GameObject. * refactor and style Removed the NetworkObject property and assignment code from the PlayerMovementManager as it is not needed. Updated comments in NetworkSceneManager regarding DDOL. * refactor We don't need to keep the DDOL object around after we have the DDOL scene. Removed IDisposable from NetworkSceneManager Removed the private m_DDOLObject Removed the Dispose method. Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com>

NoelStephensUnity added 3 commits August 25, 2021 12:43

fix

7e1435c

This fixes the dont destroy on load issue in NetworkSceneManager

test

bf8eed9

This provides manual tests to verify that don't destroy on load will work when NetworkManger's don't destroy property is enabled or disabled.

0xFA11 requested review from 0xFA11, Cosmin-B, LukeStampfli and VALERE91Unity and removed request for Cosmin-B August 25, 2021 19:10

0xFA11 and others added 3 commits August 25, 2021 20:11

Merge branch 'develop' into fix/develop-dontdestroy-networkscenemanager

cf396ba

style and refactor

6a69408

Changed some of the names used, added and updated comments, and applied the changes to the assets based on the name changes.

Merge branch 'fix/develop-dontdestroy-networkscenemanager' of https:/…

dab508c

…/github.com/Unity-Technologies/com.unity.netcode.gameobjects into fix/develop-dontdestroy-networkscenemanager

VALERE91Unity reviewed Aug 25, 2021

View reviewed changes

NoelStephensUnity added 2 commits August 25, 2021 15:59

test

ad98f1a

Adding the unit test to verify the baseline DontDestroyOnLoad scene synchronization is working when NetworkManager.DestroyOnLoad is enabled and disabled.

style

41a4440

updating comments based on PR suggestions.

mattwalsh-unity reviewed Aug 25, 2021

View reviewed changes

NoelStephensUnity added 3 commits August 25, 2021 19:49

Merge branch 'develop' into fix/develop-dontdestroy-networkscenemanager

91832b3

Merge branch 'develop' into fix/develop-dontdestroy-networkscenemanager

b89b8c1

fix

f0b4654

Fixing renamed method.

mattwalsh-unity approved these changes Aug 26, 2021

View reviewed changes

LukeStampfli reviewed Aug 26, 2021

View reviewed changes

testproject/Assets/Tests/Manual/DontDestroyOnLoad/DontDestroyOnLoadSpawnHandler.cs Show resolved Hide resolved

LukeStampfli reviewed Aug 26, 2021

View reviewed changes

testproject/Assets/Tests/Manual/DontDestroyOnLoad/ObjectToNotDestroyBehaviour.cs Show resolved Hide resolved

LukeStampfli reviewed Aug 26, 2021

View reviewed changes

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs Show resolved Hide resolved

LukeStampfli reviewed Aug 26, 2021

View reviewed changes

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs Show resolved Hide resolved

NoelStephensUnity added 3 commits August 26, 2021 22:44

fix

2cefbfc

Verifying alternate DontDestroyOnLoad test case mixed with checking when we can or cannot send RPCs to a client.

refactor

e44b814

Cleaning up our DDOL GameObject.

Merge branch 'develop' into fix/develop-dontdestroy-networkscenemanager

5b46060

LukeStampfli approved these changes Aug 27, 2021

View reviewed changes

refactor and style

8012218

Removed the NetworkObject property and assignment code from the PlayerMovementManager as it is not needed. Updated comments in NetworkSceneManager regarding DDOL.

JesseOlmer reviewed Aug 27, 2021

View reviewed changes

andrews-unity reviewed Aug 27, 2021

View reviewed changes

NoelStephensUnity added 2 commits August 27, 2021 17:56

refactor

44fe231

We don't need to keep the DDOL object around after we have the DDOL scene. Removed IDisposable from NetworkSceneManager Removed the private m_DDOLObject Removed the Dispose method.

Merge branch 'develop' into fix/develop-dontdestroy-networkscenemanager

4e74837

andrews-unity approved these changes Aug 27, 2021

View reviewed changes

NoelStephensUnity merged commit ced4138 into develop Aug 27, 2021

NoelStephensUnity deleted the fix/develop-dontdestroy-networkscenemanager branch August 27, 2021 23:28

NoelStephensUnity mentioned this pull request Sep 11, 2021

DontDestroyOnLoad NetObjects is throwing an excpetion in SceneManagement #1088

Closed

		@@ -245,13 +245,41 @@ internal void SetTheSceneBeingSynchronized(int serverSceneHandle)
		// If the scene was not found (invalid) or was not loaded then throw an exception

fix: NetworkSceneManager exception when DontDestroyOnLoad NetworkObjects are being synchronized #1090

fix: NetworkSceneManager exception when DontDestroyOnLoad NetworkObjects are being synchronized #1090

Uh oh!

Conversation

NoelStephensUnity commented Aug 25, 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!

NoelStephensUnity Aug 25, 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!

NoelStephensUnity Aug 27, 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!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NoelStephensUnity commented Aug 25, 2021 •

edited

Loading

NoelStephensUnity Aug 25, 2021 •

edited

Loading

NoelStephensUnity Aug 27, 2021 •

edited

Loading