Skip to content

fix: NetworkSceneManager clearing scene placed NetworkObjects list when ClientSynchronizationMode is additive #2383

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.

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

Conversation

NoelStephensUnity
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity commented Jan 16, 2023

When the NetworkSceneManager.ClientSynchronizationMode is set to LoadSceneMode.Additive and the client is being synchronized, NetworkSceneManager should not clear its scene placed objects list as there might already be in-scene NetworkObjects populated. This can happen if the default active scene is already loaded on the host and client(s) side and before one or more client(s) connect the active scene gets changed on the server side.
Pertains to #2377

MTT-3363
MTT-5356

This also resolves issues related to seamless scene transitions and scene streaming related issues when the NetworkSceneManager.ClientSynchronizationMode is set to LoadSceneMode.Additive.

This also includes added NetworkObject scene migration synchronization capabilities that will synchronize clients when a NetworkObject is migrated to a different scene as well as synchronizes late joining clients.
This also includes the ability to synchronize clients when the currently active scene is changed on the server side along with the ability to set a NetworkObject to auto-synchronize itself with the change (i.e. automatically migrates to the new active scene).

Internal Companion Project: This project demonstrates the various fixes and additions related to this PR.

Changelog

Added

  • Added: NetworkSceneManager.ActiveSceneSynchronizationEnabled property, disabled by default, that enables client synchronization of server-side active scene changes.
  • Added: NetworkObject.ActiveSceneSynchronization, disabled by default, that will automatically migrate a NetworkObject to a newly assigned active scene.
  • Added: NetworkObject.SceneMigrationSynchronization, enabled by default, that will synchronize client(s) when a NetworkObject is migrated into a new scene on the server side via SceneManager.MoveGameObjectToScene.

Changed

  • Updated: NetworkSceneManager to migrate dynamically spawned NetworkObjects with DestroyWithScene set to false into the active scene if their current scene is unloaded.
  • Updated: the server to synchronize its local NetworkSceneManager.ClientSynchronizationMode during the initial client synchronization.

Fixed:

  • Fixed: issue when the NetworkSceneManager.ClientSynchronizationMode is LoadSceneMode.Additive and the server changes the currently active scene prior to a client connecting then upon a client connecting and being synchronized the NetworkSceneManager would clear its internal ScenePlacedObjects list that could already be populated.
  • Fixed: issue where a client would load duplicate scenes of already preloaded scenes during the initial client synchronization and NetworkSceneManager.ClientSynchronizationMode was set to LoadSceneMode.Additive.

Testing and Documentation

  • Includes integration test additions and updates:
    • ClientSynchronizationModeTests (added for additive client synchronization mode tests)
    • NetworkObjectSceneMigrationTests (added for NetworkObject scene migration tests)
    • ParentDynamicUnderInScenePlacedHelper (updated to test Client Synch Additive + Parenting)
    • ParentingInSceneObjectsTests (updated to test Client Synch Additive + Parenting In-Scene Placed)

When the ClientSynchronizationMode is additive and the client is being synchronized, we need to not clear the ScenePlacedObjects list as there might already be ScenePlacedObjects populated.
Updating changelog entry with fix.
A more robust fix for running the server-side NetworkSceneManager in client synchronization mode additive.
Updating existing integration tests and the additive scene loading test to be able to manually test.
NoelStephensUnity and others added 15 commits January 19, 2023 14:52
Adding a piece to the puzzle where a client has scenes that don't need to be loaded or is disconnected, scenes are still loaded, and some short period later reconnects but certain scenes on the server-side are no longer loaded... under this scenario the client side will unload those scenes.

Still figuring out how to best distinguish between scenes you want to keep loaded but don't get synchronized and those that do get synchronized and you do want to unload.
Auto scene migration for don't destroy with scene (migrates to the currently active scene when the scene is unloaded) and added auto active scene synchronization where, when set,  they automatically migrate into the new active scene.

Includes auto-scene synchronization for late joining clients and dynamically spawned NetworkObjects.
some minor adjustments due to changes.
Removed the scene migration in spawn as it ended up not really making sense to do it during spawn (i.e. synchronization already handles this and if active scene synchronization is enabled then connected clients will spawn in the proper scene already).
Added some missed asserts on time out to the parent dynamic under inscene placed test and removed a section of initialization code no longer needed.
Removing commented out code.
adding a comment about why we are now synchronizing DestroyWithScene
Reverting the change with DestroyWithScene (breaking API change).
Still will get synchronized, but users will have to set this themselves.
Removed commented code.
Fixed issue with migrating into the appropriate scene.
After some deliberation over whether to include this update, I decided it would make more sense to include full NetworkObject scene migration synchronization.  This includes the additional changes required to assure that clients are synchronized when a dynamically spawned NetworkObject is moved into a new scene.
The additional integration tests for this PR. (WIP)
Includes 3 additional empty scenes to verify object scene migration.
Also a minor style issue addressed in NetworkSceneManager.
updating the changelog
Adjusting a mix-up in the added section.
Fixing isolation test failure on change log.
Adding tests for NetworkObject scene migration.
Updating scene manager handler to include moving objects from a scene to the DDOL (for unloading a scene only).
Fixing minor issue where AddSceneObject was always passing false for the destroy with scene parameter.
Added last test, ClientSynchronizationModeTests, for this PR that validates preloaded scenes will be used and in-scene placed NetworkObjects will be synchronized even when the active scene changes which could change the order in which scenes are loaded and synchronized.

This includes minor adjustments to one of the existing test.
This includes the additional INetworkSceneManager.SetClientSynchronizationMode method that now builds a list of preloaded scenes if the client synchronization mode is additive.
Making the parent dynamic under inscene placed run in client synchronization additive mode again.
removing commented out code.
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review February 13, 2023 01:05
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner February 13, 2023 01:05
Found issue where NetworkSceneManager wasn't unsubscribing from active scene changed notifications.
{
// Disable resynchronization for this test to avoid issues with trying
// to synchronize them.
NetworkSceneManager.DisableReSynchronization = true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is integration test specific and is not needed under normal operating conditions (i.e. runtime or in editor).

// Now change the active scene
SceneManager.SetActiveScene(m_ScenesLoaded[1]);
// We have to do this
Object.DontDestroyOnLoad(m_TestPrefabAutoSynchActiveScene);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another integration test thing where we don't want to destroy the temporary prefab instance during an integration test.

NoelStephensUnity and others added 7 commits February 24, 2023 17:31
Migrating last PRs changelog into the Unreleased section.
Removed the HandleToScene dictionary. It was duplicating ScenesLoaded.
Replaced all places that used HandleToScene with ScenesLoaded.
Fixing an issue when ClientSynchronizationMode is LoadSceneMode.Single and the scene to be loaded is the primary scene and is already loaded on the client side. Caught this issue running the manual test: PreserveNetworkObjectsOnShutdown.

Migrating the "shouldPassThrough" logic to the handler as this behavior/logic is slightly different when integration testing due to the fact that there can only be one active scene during an integration test.
Updating the IntegrationTestSceneHandler with the recent changes to ISceneManagerHandler.
Fixing some minor issues preventing some of the manual tests from functioning properly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants