Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Aug 27, 2021
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
7e1435c
fix
NoelStephensUnity Aug 25, 2021
bf8eed9
test
NoelStephensUnity Aug 25, 2021
ed6fb3c
fix
NoelStephensUnity Aug 25, 2021
cf396ba
Merge branch 'develop' into fix/develop-dontdestroy-networkscenemanager
0xFA11 Aug 25, 2021
6a69408
style and refactor
NoelStephensUnity Aug 25, 2021
dab508c
Merge branch 'fix/develop-dontdestroy-networkscenemanager' of https:/…
NoelStephensUnity Aug 25, 2021
ad98f1a
test
NoelStephensUnity Aug 25, 2021
41a4440
style
NoelStephensUnity Aug 25, 2021
91832b3
Merge branch 'develop' into fix/develop-dontdestroy-networkscenemanager
NoelStephensUnity Aug 26, 2021
b89b8c1
Merge branch 'develop' into fix/develop-dontdestroy-networkscenemanager
NoelStephensUnity Aug 26, 2021
f0b4654
fix
NoelStephensUnity Aug 26, 2021
2cefbfc
fix
NoelStephensUnity Aug 27, 2021
e44b814
refactor
NoelStephensUnity Aug 27, 2021
5b46060
Merge branch 'develop' into fix/develop-dontdestroy-networkscenemanager
NoelStephensUnity Aug 27, 2021
8012218
refactor and style
NoelStephensUnity Aug 27, 2021
44fe231
refactor
NoelStephensUnity Aug 27, 2021
4e74837
Merge branch 'develop' into fix/develop-dontdestroy-networkscenemanager
NoelStephensUnity Aug 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,13 +245,41 @@ internal void SetTheSceneBeingSynchronized(int serverSceneHandle)
// If the scene was not found (invalid) or was not loaded then throw an exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe removing this comment since we are not throwing anymore

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@NoelStephensUnity NoelStephensUnity Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@NoelStephensUnity NoelStephensUnity Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

SceneBeingSynchronized = SceneManager.GetActiveScene();

// Otherwise, there is some other scenario we are not handling.
Debug.LogWarning($"[{nameof(NetworkSceneManager)}- {nameof(ScenesLoaded)}] Could not find the appropriate scene to set as being synchronized! Using the currently active scene.");
}
}
else
{
// This should never happen, but in the event it does...
throw new Exception($"[{nameof(SceneEventData)}- Scene Handle Mismatch] {nameof(serverSceneHandle)} could not be found in {nameof(ServerSceneHandleToClientSceneHandle)}!");
// Most common scenario for DontDestroyOnLoad is when NetworkManager is set to not be destroyed
if (m_NetworkManager.DontDestroy)
{
if (serverSceneHandle == m_NetworkManager.gameObject.scene.handle)
{
SceneBeingSynchronized = m_NetworkManager.gameObject.scene;
return;
}
}
else
{
// The next scenario is if a user intentionally moves NetworkObjects into a DontDestroyOnLoad scene
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var dontDestroyOnLoadScene = SceneManager.GetSceneByName("DontDestroyOnLoad");
if (dontDestroyOnLoadScene.IsValid() && dontDestroyOnLoadScene.isLoaded && dontDestroyOnLoadScene.handle == serverSceneHandle)
{
SceneBeingSynchronized = dontDestroyOnLoadScene;
return;
}

// 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
// or the NetworkObject has yet to be moved to that specific scene (i.e. no DontDestroyOnLoad scene exists yet).
SceneBeingSynchronized = SceneManager.GetActiveScene();

// Otherwise, there is some other scenario we are not handling.
Debug.LogWarning($"[{nameof(SceneEventData)}- Scene Handle Mismatch] {nameof(serverSceneHandle)} could not be found in {nameof(ServerSceneHandleToClientSceneHandle)}. Using the currently active scene.");
}
}
}

Expand Down
19 changes: 19 additions & 0 deletions ...oject/Assets/References/Scene/DestroyNetworkManager_DontDestroyOnLoadSceneReference.asset
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
%YAML 1.1
%TAG !u! tag:unity3d.com,2011:
--- !u!114 &11400000
MonoBehaviour:
m_ObjectHideFlags: 0
m_CorrespondingSourceObject: {fileID: 0}
m_PrefabInstance: {fileID: 0}
m_PrefabAsset: {fileID: 0}
m_GameObject: {fileID: 0}
m_Enabled: 1
m_EditorHideFlags: 0
m_Script: {fileID: 11500000, guid: 39a16938ffb5cd846a9f6df7a686a9c4, type: 3}
m_Name: DestroyNetworkManager_DontDestroyOnLoadSceneReference
m_EditorClassIdentifier:
SceneToReference: {fileID: 102900000, guid: 7dc1ab12373402546befc7d54e447258, type: 3}
m_IncludedScenes: []
m_DisplayName: DestroyNetworkManager DontDestroyOnLoad Test
m_ReferencedScenes:
- DontDestroyOnLoad_DestroyNetworkManagerTest
8 changes: 8 additions & 0 deletions .../Assets/References/Scene/DestroyNetworkManager_DontDestroyOnLoadSceneReference.asset.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 19 additions & 0 deletions testproject/Assets/References/Scene/DontDestroyOnLoadSceneReference.asset
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
%YAML 1.1
%TAG !u! tag:unity3d.com,2011:
--- !u!114 &11400000
MonoBehaviour:
m_ObjectHideFlags: 0
m_CorrespondingSourceObject: {fileID: 0}
m_PrefabInstance: {fileID: 0}
m_PrefabAsset: {fileID: 0}
m_GameObject: {fileID: 0}
m_Enabled: 1
m_EditorHideFlags: 0
m_Script: {fileID: 11500000, guid: 39a16938ffb5cd846a9f6df7a686a9c4, type: 3}
m_Name: DontDestroyOnLoadSceneReference
m_EditorClassIdentifier:
SceneToReference: {fileID: 102900000, guid: ff98b91da4ee7ff44bc3aa8a57ad5c12, type: 3}
m_IncludedScenes: []
m_DisplayName: DontDestroyOnLoad Test
m_ReferencedScenes:
- DontDestroyOnLoadTest
8 changes: 8 additions & 0 deletions testproject/Assets/References/Scene/DontDestroyOnLoadSceneReference.asset.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions testproject/Assets/Tests/Manual/DontDestroyOnLoad.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 25 additions & 0 deletions testproject/Assets/Tests/Manual/DontDestroyOnLoad/DontDestroyOnLoadSpawnHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
using UnityEngine;
using Unity.Netcode;

namespace TestProject.ManualTests
{
/// <summary>
/// Spawns a single NetworkPrefab
/// </summary>
public class DontDestroyOnLoadSpawnHandler : NetworkBehaviour
{
public GameObject NetworkPrefabToCreate;

public override void OnNetworkSpawn()
{
if (NetworkPrefabToCreate != null && IsServer)
{
var newGameObject = Instantiate(NetworkPrefabToCreate);
var networkObject = newGameObject.GetComponent<NetworkObject>();
networkObject.Spawn();
}

base.OnNetworkSpawn();
}
}
}
11 changes: 11 additions & 0 deletions testproject/Assets/Tests/Manual/DontDestroyOnLoad/DontDestroyOnLoadSpawnHandler.cs.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading