refactor: decouple PendingSoftSyncObjects from NetworkSpawnManager #913

NoelStephensUnity · 2021-06-21T14:42:19Z

As described in MTT_819, part of the scene management refactoring process includes first removing specific dependencies from other classes. This is the first phase of this process. This task also includes the removal of NetworkSceneManager.IsSpawnedObjectsPendingInDontDestroyOnLoad

Acceptance Tests
The manual scene transitioning tests need to be used for this PR as it was determined specific changes (Phase 3 of the proposal) are required to attempt to write an automated scene transitioning test that covers in-scene placed NetworkObjects and dynamically spawned NetworkObjects.

Additional Updates
Before implementation of the automated unit test, there was a need to update some of the classes and assets used in the existing manual scene transitioning test based on more current changes ( i.e. NetworkTransform requires rigid bodies to be set to IsKinematic on non-owner/authorized clients otherwise they will get errors logged which would cause issues with unit testing).
While I was in the related regions of code, if I noticed misspelled words in comments or the like I included those as well.

This removes NetworkSpawnManager.PendingSoftSynchObjects and replaces it with NetworkSceneManager.ScenePlacedObjects. The primary difference between the two is that the ScenePlacedObjects is populated immediately after a level is loaded or when being approved and already within the current scene.

Migrating the ScenePlacedObjects dictionary up to the rest of the NetworkkSceneManager's dictionary definitions. Removing the PendingSoftSyncObjects dictionary from NetworkSpawnManager.

Use the ScenePlacedObjects dictionary to generate the client(s) switch scene stream buffer. Refactored PopulateScenePlacedObjects method to not set the IsSceneObject to true and to also check and make sure the NetworkObject instance's NetworkManager reference is the same NetworkManager instance as the NetworkSceneManager's NetworkManager instance in question in order to maintain compatibility with the MultiInstanceHelper class for Mult-Instance unit tests.

Renaming OnSceneUnloadClient and OnSceneUnloadServer to OnClientLoadedScene and OnServerLoadedScene

Fixed some remaining checks (player prefab) as to whether the rigid body should be kinematic or not to prevent the mega spam. During the revert changes of the NetworkTransform changes to better reflect what is in the develop branch, missed the else if. Fixed a minor issue with the slider in the SceneTransitioning manual test to better visually reflect what the value is as well as handle the scenario if it starts at 0 boxes per second it needed to start the coroutine and if set to zero and the coroutine was running it terminates the coroutine.

This is a fix for the NetworkObjectOnSpawnTest where it was spawning a GameObject twice that contained a component that would throw an exception if you spawned a NetworkObject.

An unintended project version change

Checking to see if Yamato fails on this...

This test verifies that "cleaning in-scene placed network objects" within the connection approved method was hiding potential bugs from other unit tests.

Removing the unit test duplicated GameObject instance and all of the associated code to verify this was how the bug was missed by TestRunner and Yamato.

reverting back to the original values for these 3 files that were mistakenly committed.

0xFA11 · 2021-06-28T12:39:04Z

com.unity.multiplayer.mlapi/Runtime/SceneManagement/NetworkSceneManager.cs

+        /// -- Before any "DontDestroyOnLoad" NetworkObjects have been added back into the scene.
+        /// Added the ability to choose not to clear the scene placed objects for additive scene loading.
+        /// </summary>
+        internal void PopulateScenePlacedObjects(bool clearScenePlacedObjects = true)


minor (personal opinion):

Suggested change

internal void PopulateScenePlacedObjects(bool clearScenePlacedObjects = true)

internal void UpdateScenePlacedObjects(bool clearScenePlacedObjects = true)

0xFA11 · 2021-06-28T12:41:30Z

com.unity.multiplayer.mlapi/Runtime/SceneManagement/NetworkSceneManager.cs

            }
        }

-        private void OnSceneUnloadServer(Guid switchSceneGuid)
+        private void OnServerLoadedScene(Guid switchSceneGuid)


while you're doing some small refactors here, do you mind renaming all switchSceneGuid and other switchXXX with non-switch versions such as sceneGuid instead of switchSceneGuid? I believe it wouldn't harm the understanding but fix syntax-highlighter getting confused :)

Good idea...

Will see if there is a better term for this... I ignored some of this stuff because the later phases of the proposal would effectively remove all of that.

0xFA11 · 2021-06-28T12:44:11Z

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

@@ -25,9 +25,6 @@ public class NetworkSpawnManager
        /// </summary>
        public readonly Dictionary<ulong, NetworkObject> SpawnedObjects = new Dictionary<ulong, NetworkObject>();


I guess we will eventually get rid of NetworkSpawnManager entirely at some point. For instance, SpawnedObjects should probably be managed by NetworkSceneManager since they are properties of scenes. Even if we were to move objects between scenes, they are still "in" scenes. Managing object spawning and instances in 2 (NetworkSpawnManager & NetworkSceneManager) feels a bit weird to me :P

But obv, that's something for later — just wanted to braindump my 2-cents here.

Yeah, it is weird indeed. This update removes the dependency from NetworkSpawnManager and places it into NetworkSceneManager in preparation for the proposed direction. If we do go in the proposed general direction, then the object synchronization process will be handled differently anyway.... so keeping a list of in-scene NetworkObjects instantiated via scene loading really isn't "handling spawning"... it is just "tracking what was automatically instantiated".

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

0xFA11

overall looking good except for 1 potential bug.
also left some minor comments here and there.

…er.cs Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com>

0xFA11

bug fixed, good enough for me!

LukeStampfli · 2021-06-29T10:34:17Z

com.unity.multiplayer.mlapi/Runtime/SceneManagement/NetworkSceneManager.cs

@@ -79,6 +79,7 @@ public class NetworkSceneManager
        internal readonly Dictionary<string, uint> SceneNameToIndex = new Dictionary<string, uint>();
        internal readonly Dictionary<uint, string> SceneIndexToString = new Dictionary<uint, string>();
        internal readonly Dictionary<Guid, SceneSwitchProgress> SceneSwitchProgresses = new Dictionary<Guid, SceneSwitchProgress>();
+        internal readonly Dictionary<uint, NetworkObject> ScenePlacedObjects = new Dictionary<uint, NetworkObject>();


Just to be sure, this functions the same as the previous PendingSoftSyncObjects? So once a scene placed object get spawned it will be removed from this list? Maybe we should call this PendingScenePlacedObjects or UnspawnedScenePlacedObjects if that is the case?

LukeStampfli

More decoupling is always welcome!

…ity-Technologies/com.unity.multiplayer.mlapi into test/multiprocess-tests/orchestration * 'test/multiprocess-tests/orchestration' of github.com:Unity-Technologies/com.unity.multiplayer.mlapi: feat: log warning if detected child NetworkObjects under a NetworkPrefab (#938) fix: reducing log level for noisy log and adding details for developer log (#926) feat: users can set authority on network transform programmatically (#868) refactor: move NetworkBehaviour update to a separate non-static class (#917) test: add utils for multi instance tests (#914) test: downgrading testproject to 2020.3.12f1 (#927) refactor: decouple PendingSoftSyncObjects from NetworkSpawnManager (#913) chore: Change function signature of OnDespawnObject to accept NetworkObject (#928) fix: Empty prefab removal (#919)

…rocess-tests/base-multiprocess-tests * test/multiprocess-tests/orchestration: feat: log warning if detected child NetworkObjects under a NetworkPrefab (#938) fix: reducing log level for noisy log and adding details for developer log (#926) feat: users can set authority on network transform programmatically (#868) refactor: move NetworkBehaviour update to a separate non-static class (#917) test: add utils for multi instance tests (#914) test: downgrading testproject to 2020.3.12f1 (#927) refactor: decouple PendingSoftSyncObjects from NetworkSpawnManager (#913) chore: Change function signature of OnDespawnObject to accept NetworkObject (#928) fix: Empty prefab removal (#919)

…est/multiprocess-tests/execute-step-in-context * test/multiprocess-tests/base-multiprocess-tests: feat: log warning if detected child NetworkObjects under a NetworkPrefab (#938) fix: reducing log level for noisy log and adding details for developer log (#926) feat: users can set authority on network transform programmatically (#868) refactor: move NetworkBehaviour update to a separate non-static class (#917) test: add utils for multi instance tests (#914) test: downgrading testproject to 2020.3.12f1 (#927) refactor: decouple PendingSoftSyncObjects from NetworkSpawnManager (#913) chore: Change function signature of OnDespawnObject to accept NetworkObject (#928) fix: Empty prefab removal (#919)

…est/multiprocess-tests/adding-perf-tests-for-spawn * test/multiprocess-tests/execute-step-in-context: feat: log warning if detected child NetworkObjects under a NetworkPrefab (#938) fix: reducing log level for noisy log and adding details for developer log (#926) feat: users can set authority on network transform programmatically (#868) refactor: move NetworkBehaviour update to a separate non-static class (#917) test: add utils for multi instance tests (#914) test: downgrading testproject to 2020.3.12f1 (#927) refactor: decouple PendingSoftSyncObjects from NetworkSpawnManager (#913) chore: Change function signature of OnDespawnObject to accept NetworkObject (#928) fix: Empty prefab removal (#919) # Conflicts: # testproject/Packages/manifest.json

…to test/multiprocess-tests/adding-doc-on-how-to-use * test/multiprocess-tests/adding-perf-tests-for-spawn: (22 commits) removing comment and adding something a bit more dynamic correct spacing fix for unused method better comments adding tests to make sure things don't break better exception using proper list using latest test framework rename apply rename should be kept public for following PR naming Applying suggestions Apply suggestions from code review consistent naming feat: log warning if detected child NetworkObjects under a NetworkPrefab (#938) fix: reducing log level for noisy log and adding details for developer log (#926) feat: users can set authority on network transform programmatically (#868) refactor: move NetworkBehaviour update to a separate non-static class (#917) test: add utils for multi instance tests (#914) test: downgrading testproject to 2020.3.12f1 (#927) refactor: decouple PendingSoftSyncObjects from NetworkSpawnManager (#913) ...

NoelStephensUnity added 13 commits June 3, 2021 09:57

style and refactor

cfe461a

Migrating the ScenePlacedObjects dictionary up to the rest of the NetworkkSceneManager's dictionary definitions. Removing the PendingSoftSyncObjects dictionary from NetworkSpawnManager.

refactor

d47281e

Renaming OnSceneUnloadClient and OnSceneUnloadServer to OnClientLoadedScene and OnServerLoadedScene

Merge branch 'develop' into refactor/MTT-819

c2067fd

Merge branch 'develop' into refactor/MTT-819

fdcaaea

Fix

bdfe6b5

This is a fix for the NetworkObjectOnSpawnTest where it was spawning a GameObject twice that contained a component that would throw an exception if you spawned a NetworkObject.

fix

ab30892

An unintended project version change

Merge branch 'develop' into refactor/MTT-819

7b90aaf

bug test

34b33b7

Checking to see if Yamato fails on this...

test

83afebc

This test verifies that "cleaning in-scene placed network objects" within the connection approved method was hiding potential bugs from other unit tests.

fix

242ac60

Removing the unit test duplicated GameObject instance and all of the associated code to verify this was how the bug was missed by TestRunner and Yamato.

NoelStephensUnity marked this pull request as ready for review June 21, 2021 21:23

NoelStephensUnity requested review from TwoTenPvP and SamuelBellomo June 21, 2021 21:29

fix

c31e9a0

reverting back to the original values for these 3 files that were mistakenly committed.

NoelStephensUnity mentioned this pull request Jun 23, 2021

feat: Scene Registration Refactoring - New UIX Features #918

Closed

NoelStephensUnity requested review from 0xFA11 and LukeStampfli June 28, 2021 12:31

0xFA11 changed the title ~~refactor: MTT_819 decoupling pending soft synch objects from spawn manager~~ refactor: decouple PendingSoftSyncObjects from NetworkSpawnManager Jun 28, 2021

0xFA11 reviewed Jun 28, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/Spawning/NetworkSpawnManager.cs Outdated Show resolved Hide resolved

0xFA11 suggested changes Jun 28, 2021

View reviewed changes

Update com.unity.multiplayer.mlapi/Runtime/Spawning/NetworkSpawnManag…

80c5f44

…er.cs Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com>

0xFA11 approved these changes Jun 28, 2021

View reviewed changes

LukeStampfli reviewed Jun 29, 2021

View reviewed changes

LukeStampfli approved these changes Jun 29, 2021

View reviewed changes

Merge remote-tracking branch 'origin/develop' into refactor/MTT-819

446b4d4

NoelStephensUnity enabled auto-merge (squash) June 29, 2021 16:07

NoelStephensUnity merged commit fae7e8c into develop Jun 29, 2021

NoelStephensUnity deleted the refactor/MTT-819 branch June 29, 2021 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: decouple PendingSoftSyncObjects from NetworkSpawnManager #913

refactor: decouple PendingSoftSyncObjects from NetworkSpawnManager #913

Uh oh!

NoelStephensUnity commented Jun 21, 2021 •

edited

Loading

Uh oh!

0xFA11 Jun 28, 2021

Uh oh!

0xFA11 Jun 28, 2021

Uh oh!

NoelStephensUnity Jun 28, 2021

Uh oh!

NoelStephensUnity Jun 28, 2021

Uh oh!

0xFA11 Jun 28, 2021

Uh oh!

NoelStephensUnity Jun 28, 2021

Uh oh!

Uh oh!

0xFA11 left a comment

Uh oh!

0xFA11 left a comment

Uh oh!

LukeStampfli Jun 29, 2021

Uh oh!

LukeStampfli left a comment

Uh oh!

Uh oh!

	internal void PopulateScenePlacedObjects(bool clearScenePlacedObjects = true)
	internal void UpdateScenePlacedObjects(bool clearScenePlacedObjects = true)

		@@ -25,9 +25,6 @@ public class NetworkSpawnManager
		/// </summary>
		public readonly Dictionary<ulong, NetworkObject> SpawnedObjects = new Dictionary<ulong, NetworkObject>();

refactor: decouple PendingSoftSyncObjects from NetworkSpawnManager #913

refactor: decouple PendingSoftSyncObjects from NetworkSpawnManager #913

Uh oh!

Conversation

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

Uh oh!

Uh oh!

0xFA11 Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

0xFA11 Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

0xFA11 Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0xFA11 left a comment

Choose a reason for hiding this comment

Uh oh!

0xFA11 left a comment

Choose a reason for hiding this comment

Uh oh!

LukeStampfli Jun 29, 2021

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 Jun 21, 2021 •

edited

Loading