-
Notifications
You must be signed in to change notification settings - Fork 450
feat: additive scene loading and networkscenemanager refactoring #955
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
feat: additive scene loading and networkscenemanager refactoring #955
Conversation
Adjusting scene related commands. Keeping the original commands in place, but providing two new commands: SCENE_EVENT: This will contain sub-commands and additional information specific to the command. SCENE_NOTIFICATION: This is more for providing a mechanism to send various notifications (i.e. scene switch complete, scene loaded, scene unloaded, errors, etc) with the ability to include additional information about the event.
Added new SCENE_EVENT that will handle "all things scene relative" with the only exception of the approval process. That will need to be handled in a different PR as that requires a bunch of additional changes. Focus for this branch is to get additive scene loading and unloading working.
Added additive scene unloading into the mix. Currently no events are wired for additive loading complete or additive unloading complete, but console logging on the server side verifies that the client sends the appropriate scene event notification once finished loading or unloading. Cleaned up some of the test project code to make sure the client isn't creating a pool to make sure the client is only replicating what the server is telling it to as well as various other minor test project tweaks for additive scene loading demonstration purposes.
Removed static properties in NetworkSceneManager that were no longer being used. Fixed issue with host not having the proper network prefab override (i.e. running as host vs running as server). Moved SceneEventData into its own .cs file. Excluding the local notification events, this is very close to being finalized, with the exception of custom prefab overrides and additive scene loading. This still has issues with late joining (most likely this has to do with the NetworkPrefabPool code base, still investigating this issue).
Improved the NetworkPrefabPool and NetworkPrefabPoolAdditive implementations to properly handle the various potential use case scenarios. These changes assure that both the client and the server leverage from the pool. This also takes into account NetworkManager defined NetworkPrefab overrides. Added temporary LoadSceneEvent callback that notifies if an additive scene is being loaded or unloaded. Updated the NetworkPrefabPoolAdditive sample to demonstrate how a user can assure their NetworkObjects are "spawned" in the same additive scene as their spawn generator. This pattern will always destroy all NetworkObjects if the additive scene is unloaded. Added sorting of NetworkObjects just prior to the server side serializing them in order to assure INetworkPrefabInstanceHandler implementations are registered first, this is required in order to assure that the handler is registered before any NetworkObjects associated with it are spawned.
Some temporary fixes for message ordering and buffering during late-join until this gets merged with the message ordering updates and we add the ability to buffer messages while a player is late-joining. Primarily in the GenericNetworkObjectBehaviour class. Added a proposed solution to handle order of operations if user decides to allow all NetworkObjects from a prefab pool to be spawned in the active scene. This requires the ability to separate NetworkObjects (server side) by associated scene so that they will be synchronized properly with their spawn generator's pool. Now tracking which additive scenes have been loaded to be assured they are unloaded when doing a full scene switch (i.e. singlemode loading), otherwise there can be issues that arise with currently loaded additive scenes.
Two issues were discovered while getting the parenting test working: 1.) t was doing a move objects to scene during the client side synchronization process (which doesn't need to happen) and as such was setting all parents to null which was throwing an exception. 2.) The server side wasn't filtering out any pre-loaded scenes during the synchronization process.
Minor test project fixes and additional tests for the AdditiveSceneToggleHandler to test back to back scene loading during a scene transition. Removed the distinguishing difference between switchscene and loadscene, left the switchscene as a wrapper around loadscene. Updated existing profiling code base to remove legacy scene manager profiler checks.
/// </summary> | ||
public event NotifyClientAllClientsLoadedSceneDelegate OnNotifyClientAllClientsLoadedScene; | ||
internal static bool IsTesting; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually do not like seeing this at all. I remember using this check somewhere down there to skip test scenes from being considered for synchronization but I'd handle that case differently. For instance, I'd wrap that code-block with #if UNITY_EDITOR
(and maybe something like BUILD_TESTING
if there's one?) and then rely on some other flag somewhere else, ideally either a Unity API or our testing helper flag telling us "hey! you're actually running inside a test". seeing this flag here and being used by tests sounds weird to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be renamed and is only specific to multiInstance unit tests. I will investigate if there is a way to handle this in an more potentially automated way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MFatihMAR
I removed IsTesting and now handle this only when in UNITY_EDITOR and NetworkSceneManager detects whether or not it is running under an NUnit test context or not (i.e. IsTesting?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitted a better solution:
Don't check for any kind of testing at all (whether automated or setting a property) and make adjustments to the problematic unit test and change a property in NetworkSceneManager from private to internal.
…feat/MTT-820-AdditiveSceneLoading
This resolves the issue where disabling scene management would result in no NetworkObjectSynchronization of client relative observed NetworkObjects. This also further separates NetworkObject serialization even further from the NetworkSceneManager and is contained either in: NetworkSpawnManager, SceneEventData, and NetworkObject itself. This will help in future efforts to further unify this process with some form of "NetworkObjectSynchronization" system that ties together NetworkSceneManager, Snapshot, and NetworkSpawnManager related client/server NetworkObject synchronization/registration related tasks into a single management system that should work independently of other systems (at least something in that general direction).
------------------------------------------------------------- After further consideration, the best idea for the whole unit test issue is to: 1.) Not try to detect it ( silly idea of mine ) 2,) Make changes to the NetworkSceneManager's private m_ScenesLoaded property by renaming it to ScenesLoaded and making it internal. 3.) Update the one Unit test that was failing to populate ScenesLoaded and ServerSceneHandleToClientSceneHandle tables so the SetTheSceneBeingSynchronized method could properly be tested during the NetworkObjectSceneSerializationFailure test. It turns out my brain decided to take a "(dis)connect" day a few days too early. No need to check for unit tests at all for this PR. ------------------------------------------------------------- This also includes a minor adjustment for when the NetworkPrefabPool (test project script) unsubscribes from the NetworkSceneManager.OnSceneEvent to avoid a null access exception.
Took Fatih's suggestion regarding ScenesLoaded being over-complicated and now the ScenesLoaded is just a dictionary of scene handle keys and each key entry value is the Scene it is associated with. This did reduce some of the complexity in the unloading methods and the GetAndAddNewlyLoadedSceneByName method. Updated the NetworkObjectSceneSerializationFailure unit test to reflect the above changes.
Noticed one minor portion of the client loading process that really shouldn't be outside of SceneEventData. Removed 1 method call from NetworkSceneManager and 1 method from SceneEventData.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy with the changes about sycning when not enabled and it addresses my concerns.
…nsform * develop: feat: additive scene loading and networkscenemanager refactoring (#955) # Conflicts: # com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs
…ty-Technologies#955) * feat This is a quick and dirty additive scene loading implementation example with minimal changes to the develop branch. * refator Updating basic scene registration to accept SceneAssets as opposed to string based names. * refactor Removing the automatic addition of a scene asset to NetworkConfig. This has been a problematic approach to assuring the scene containing the NetworkManager is included. * refactor - wip Adjusting scene related commands. Keeping the original commands in place, but providing two new commands: SCENE_EVENT: This will contain sub-commands and additional information specific to the command. SCENE_NOTIFICATION: This is more for providing a mechanism to send various notifications (i.e. scene switch complete, scene loaded, scene unloaded, errors, etc) with the ability to include additional information about the event. * feat Added new SCENE_EVENT that will handle "all things scene relative" with the only exception of the approval process. That will need to be handled in a different PR as that requires a bunch of additional changes. Focus for this branch is to get additive scene loading and unloading working. * refactor Created the LoadScene method which will only load scenes additively. Broke up some of the commonly shared initializations and checks. This helps to show what the real differences between single and additive scene loading are. (like two method calls) * refactor Just cleaning up the code, putting place holder notes, and adding some comments * style Just comment and style adjustments * refactor and feat Added additive scene unloading into the mix. Currently no events are wired for additive loading complete or additive unloading complete, but console logging on the server side verifies that the client sends the appropriate scene event notification once finished loading or unloading. Cleaned up some of the test project code to make sure the client isn't creating a pool to make sure the client is only replicating what the server is telling it to as well as various other minor test project tweaks for additive scene loading demonstration purposes. * refactor Swapped out the switch scene cycle hack for actual toggle buttons that allow one to load each sample additive scene or unload it based on the state of the toggle button. * feat This includes new assets to demonstrate additive scene loading and scene switching. Known issues are when additive scenes have been loaded that they are not synchronized with clients when they join. * wip A wip commit that is progress towards removing scene and object synchronization from the approval process. * refactor naming issue and making sure we scope the pooled network reader. * wip Closer to decoupling approval from the approval process. There are still some issues with regards to the active scene and this could potentially be fixed by doing client synchronization first and then finalizing the approval. Fixed an issue with NetworkManager replicating upon exiting. * feat Joining with additive scenes already loaded as well as late joining players are synchronizing properly. The handle approval process is no longer directly tied to scene loading. Minor updates to stats display to show the currently active scene * refactor Removed static properties in NetworkSceneManager that were no longer being used. Fixed issue with host not having the proper network prefab override (i.e. running as host vs running as server). Moved SceneEventData into its own .cs file. Excluding the local notification events, this is very close to being finalized, with the exception of custom prefab overrides and additive scene loading. This still has issues with late joining (most likely this has to do with the NetworkPrefabPool code base, still investigating this issue). * feat Improved the NetworkPrefabPool and NetworkPrefabPoolAdditive implementations to properly handle the various potential use case scenarios. These changes assure that both the client and the server leverage from the pool. This also takes into account NetworkManager defined NetworkPrefab overrides. Added temporary LoadSceneEvent callback that notifies if an additive scene is being loaded or unloaded. Updated the NetworkPrefabPoolAdditive sample to demonstrate how a user can assure their NetworkObjects are "spawned" in the same additive scene as their spawn generator. This pattern will always destroy all NetworkObjects if the additive scene is unloaded. Added sorting of NetworkObjects just prior to the server side serializing them in order to assure INetworkPrefabInstanceHandler implementations are registered first, this is required in order to assure that the handler is registered before any NetworkObjects associated with it are spawned. * feat and refactor Some temporary fixes for message ordering and buffering during late-join until this gets merged with the message ordering updates and we add the ability to buffer messages while a player is late-joining. Primarily in the GenericNetworkObjectBehaviour class. Added a proposed solution to handle order of operations if user decides to allow all NetworkObjects from a prefab pool to be spawned in the active scene. This requires the ability to separate NetworkObjects (server side) by associated scene so that they will be synchronized properly with their spawn generator's pool. Now tracking which additive scenes have been loaded to be assured they are unloaded when doing a full scene switch (i.e. singlemode loading), otherwise there can be issues that arise with currently loaded additive scenes. * refactor Minor checks for an object still being around during clean up. * fix Fixes the basic scene transitioning test failure. * fix Fixing other unit test issues. Removing the temporary block for clients to receive messages (it was an artifact from testing). Still have a few tests that are failing. * fix Two issues were discovered while getting the parenting test working: 1.) t was doing a move objects to scene during the client side synchronization process (which doesn't need to happen) and as such was setting all parents to null which was throwing an exception. 2.) The server side wasn't filtering out any pre-loaded scenes during the synchronization process. * fix Adding missing scene assets for the multiprocess test. Making sure the SceneLoading test destroys the network manager instance. * style PascalCased my enums and added event in front to avoid any NDA complications for the time being. * refactor and fix Minor test project fixes and additional tests for the AdditiveSceneToggleHandler to test back to back scene loading during a scene transition. Removed the distinguishing difference between switchscene and loadscene, left the switchscene as a wrapper around loadscene. Updated existing profiling code base to remove legacy scene manager profiler checks. * fix Removing Scene Loading Test: This test needs to be completely refactored and no longer is valid. WIP * refactor and feat Did some minor refactoring to make Load and Unload the only two methods that need to be called. Removed the switch event related enum types since they are no longer used. Added a re-synchronization step to handle edge case scenarios where a late joining client might miss destroy messages during the initial synchronization process. This includes adding an additional DependentSceneName property to NetworkObject. This lets the NetworkSceneManager know that a scene other than the NetworkObject's current scene is dependent upon this NetworkObject and to not instantiate it until that dependent scene is loaded. Removed a bunch of user-side code (i.e. test project) from GenericNetworkObject that was required to handle the above edge case scenarios regarding scene dependencies. Adjusted the NetworkPrefabPool and NetworkPrefabPoolAdditive to account for this addition, and removed a chunk of user-side code that would be required to handle this scenario. Refactored SceneEventData to not implement INetworkSerializable which removes additional allocation for the NetworkSerializer during read and write operations. Also adjusted the constructor to accept a NetworkManager instance to make it multi-instance compatible. Set the SceneTransitioningBase1 NetworkManager config to not recycle NetworkObjectIds due to an issue in how they are recycled. (topic for discussion) * style comments and naming * Refactor and Fix Fixed issue with rogue dynamically spawned NetowrkObjects that were spawned during a client scene synchronization sequence where it required a Custom NetworkPrefab hanler to spawn properly but the scene containing that custom network prefab handler had yet to be loaded so it was never associated with the pool. Migrated the additive scene loading scene transitioning tests into the manual tests region of TestProject * FIx and Style Fix for client side where during resync the GameObject was being destroyed but the spawn manager's lists tracking spawned NetworkObjects was not being updated. The symptom was when a NetworkObjectId was recycled for a new NetoworkObject the client would still have that NetworkObjectId registered. This also includes some style updates. * refactor and fix Refactoring things based on more recent changes. Predominantly the Message Ordering changes and the time synchronization. * fix Removing some adjustments I did not intend on checking in. * refactor removed and re-added scenes in build list * refactor Increasing timeout for one at a time. * refactor Reverted minor modifications to transform test. * refactor Removing temporary fix for issue with NetworkBuffer. Adding a test to the NetworkTransformTests to see if frame rate has anything to do with the seemingly random failures only on MAC. * test Updating this adjustment to see if this makes it through the mac tests. * test Forgot to remove the applied frame rate. * refactor Cleaning up merge from develop. Starting to work on the local notification side of things. * refactor Work in progress for local notifications, how network prefab handler pools destroy objects, and how NetworkObjects marked for or not marked for do not destroy on load are handled. * fix Checking for NetworkManager to exist before disabling. * fix This includes fixes for the namespace changes. * fix Standards check for no longer needed using statments * fix temporary fix for scene loading test. * refactor Updating additive pooling with additive scene unloading. Added the ability to specify whether NetworkObjects should be destroyed on scene unload or not. If the SpawnInSourceScene is set to true and the DestroyOnUnload is not set, then the already spawned NetworkObjects are moved to the currently active scene to allow them to persist until they naturally are destroyed. Fixed issues with most objects already destroyed messages. Still have a few that pop up from time to time. Fixed issue with client not sending responses back to server. Refactored NetworkPrefabHandler and INetworkPrefabInstanceHandler so that the destroy method returns a bool value which determines if the NetworkObject should actually be destroyed or not (there are edge case scenarios this fixes). Updated several tests to reflect these changes. * fix merge fix. * refactor and fix Refactoring the Stats Display a bit. Added check for NetworkBehaviour Updater to not try to access the NetworkObject if it is null. This can happen under scenarios where too many objects are being spawned and destroyed at the same time. * style Fixing non-required using statements. * refactor and style Refactored the prefab pool examples for the changed INetworkPrefabInstanceHandler interface. Made some adjustments to the code based on suggestions. In particular added a prefix to the SceneEventData.SceneEventTypes to help identify which is server to client and client to server. * refactor Removing completely irrelevant code... * refactor Updates to account for the namespace change...again. * feat Implemented the full scene event notification system that provides detailed information for both client(s) and server. Made some modifications to the StatsDisplay that provides visual queues for when each scene event occurs. * style slightly increased the player prefab size * feat Improved notifications and detection of notifications for manual verification that they are triggering properly. Added a SceneEventNotificationQueue test project script that is to be used with the NetworkManager to detect edge case notifications (i.e. in the middle of a scene transition). Fixed an edge case scenario with NetworkObject.Despawn that could occur during exiting the application and the SpawnManager no longer exists. * fix, refactor, and style re-fixing an issue with sending spawn calls to every client (n) times based on the number of clients connected (until it is re-fixed by the offender) Started the removal of certain NetworkSceneManager event notifications that are no longer required due to the more recent SceneEvent notifications. Removed the set active scene scene event for MS-1 (just not enough time to implement and be sure that it works perfectly) Removed a manual test for SceneManager callbacks that won't be used any longer. Updated XML documentation in various areas. * feat SwitchSceneProgress changed to SceneEventProgress and is no longer a returned value when starting a scene event (load or unload). This is still used internally to track when all clients have loaded the scene in question, but that is primarily used for SceneLoadingMode.Single. Removed the coroutine callback from NetworkManager and placed it within the SceneEventProgress class. * feat Finalized replacing all notification related events with scene events. SceneEvent now includes two additional types: S2C_LoadComplete: Server to client(s) all clients loaded scene S2C_UnLoadComplete: Server to client(s) all clients unloaded scene These replace the event notifications that specified all clients have loaded a specific scene, with the additional functionality that this notification is sent for both loading and unloading for all loading modes. Removed the legacy internal messages and related methods for all clients done loading a scene. This should mark the final overhaul for notifications * style Did one last XML Documentation pass. * refactor Removing two exceptions that will not be needed. * refactor Added the ability to disable the re-synchronization for future snapshot development purposes. * refactor Removed debug information from SceneEventData. * style and refactor Minor clean up on some comments. Minor refactoring of property and method accessibility. * fix and style replacing exception message reference to scene switch with scene event. Fixing check for scene not being loaded check in UnloadScene. * fix Message ordering seemed to have issues with application target frame rate. Checking this and setting it prior to running a test seems to fix the issue with failing on the FixedUpdate side of things. * refactor reverting timeouts to 5 seconds each. * refactor Removing the target frame rate check in the SetUp. * refactor Removing check for NetworkObject being null in NetworkBehaviourUpdater, this is no longer needed. Removing artifact debug output (testing purposes) that was missed. * fix Seconds vs frame count Mac failed. * style added comment * style removing the TODO and my initials from MTT-860 comments and providing a bit more of an explanation * style Added or updated some comments for better clarity. * refactor Assigning the TargetClientId for NetworkVariable writing. This could change with the snapshot stuff and the up-and-coming change in read/write permissions, but for the time being making sure this is set only for S2C_Sync events. * style removing MLAPI from sceneIndex * refactor and feat This includes several updates: We now load all scenes first and then we synchronize all NetworkObjects after all scenes have loaded (during the client synchronization process). This means users no longer have to register any form of dependency with NetworkObjects. This includes the additional modifications to support loading the same additive scene (within scene placed NetworkObjects) multiple times. This did require a bit of refactoring and some improvements in how we organize our InScenePlacedObjects. The SceneTransitioningAdditive manual tests (SceneTransitioningBase1 is root scene) include demonstration of loading/unloading the same additive scene repeatedly. * Fix Lots of fixes with the new requirements. We will need to come up with a better way to instantiate NetworkObjects for unit tests, but this includes all of the fixes required to get all current unit tests working again. * fix Fixing minor issue with the standard NetworkPrefabPool (really need to merge these two classes) where it wasn't removing the registration from NetworkPrefabHandler when a scene was unloaded (only in the traditional SceneTransitioningTest without additive scene loading). * fix and style removed the unused namespace from NetworkPrefabHandlerTests * fix Fixing some tools unit test related issues. * fix Removing my tools fix as that was the wrong place to fix that issues. The error causing issue actually came before those two tests where something was being instantiated but never destroyed. If this fails then I might need to do the same thing with the Setup side of things as well. * fix test Last try of the day... just delete every NetworkObject that exists when we start a mutli-instance test. * Test For real, this is my last fix test and then I will ask Tools team about the issues involved in trying to debug their tests locally. * style updated comments and added some xml doc see refs. * test updating test projects build settings scenes in build list with new scene to work with additive scene manual testing levels * Update com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs Co-authored-by: Luke Stampfli <43687322+LukeStampfli@users.noreply.github.com> * fix Minor stats display issue with another sneaky resources meta file that somehow made it into a previous commit. * stle Renamed NotifyPlayerConnected to ApprovedPlayerSpawn * refactor removing unused m_ObservedObjects. * style Adding additional comment to further clarify usage of a multidimensional dictionary * minor editor UI polish * `./standards.py --fix` * refactor and style Removing a check that is no longer needed. Improved an exception message. Improved some comments. Removed a CR * Update Applying Fatih's super-nit. Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com> * test fix Fixing an issue that can occur in a testproject helper script where the wrong scene could be applied to the wrong "toggler" when loaded. * fix Fixing committed suggested change bug. * fix unit test Fixing the change in exception thrown when trying to call NetworkSceneManager.Load or NetworkSceneManager.Unload without having set NetworkConfig.EnableSceneManagement. * fix: eliminate bad use-after-free(destroy) pattern * remove obsolete comment * refactor Removes returning bool to determine if NetworkPrefabHandler should destroy an object with Fatih's awesome fixes from PR-1068 Unity-Technologies#1068 * refactor and fix This resolves the issue where disabling scene management would result in no NetworkObjectSynchronization of client relative observed NetworkObjects. This also further separates NetworkObject serialization even further from the NetworkSceneManager and is contained either in: NetworkSpawnManager, SceneEventData, and NetworkObject itself. This will help in future efforts to further unify this process with some form of "NetworkObjectSynchronization" system that ties together NetworkSceneManager, Snapshot, and NetworkSpawnManager related client/server NetworkObject synchronization/registration related tasks into a single management system that should work independently of other systems (at least something in that general direction). * refactor Removing the need to set NetworkSceneManager.IsTesting to true. Now NetworkSceneManager automatically detects if a unit test is running. * refactor unit test check ------------------------------------------------------------- After further consideration, the best idea for the whole unit test issue is to: 1.) Not try to detect it ( silly idea of mine ) 2,) Make changes to the NetworkSceneManager's private m_ScenesLoaded property by renaming it to ScenesLoaded and making it internal. 3.) Update the one Unit test that was failing to populate ScenesLoaded and ServerSceneHandleToClientSceneHandle tables so the SetTheSceneBeingSynchronized method could properly be tested during the NetworkObjectSceneSerializationFailure test. It turns out my brain decided to take a "(dis)connect" day a few days too early. No need to check for unit tests at all for this PR. ------------------------------------------------------------- This also includes a minor adjustment for when the NetworkPrefabPool (test project script) unsubscribes from the NetworkSceneManager.OnSceneEvent to avoid a null access exception. * refactor Removing the check for a null SpawnManager in NetworkObject.Despawn as this no longer seems to be an issue after the more recent merges in develop. * refactor Took Fatih's suggestion regarding ScenesLoaded being over-complicated and now the ScenesLoaded is just a dictionary of scene handle keys and each key entry value is the Scene it is associated with. This did reduce some of the complexity in the unloading methods and the GetAndAddNewlyLoadedSceneByName method. Updated the NetworkObjectSceneSerializationFailure unit test to reflect the above changes. * refactor minor Noticed one minor portion of the client loading process that really shouldn't be outside of SceneEventData. Removed 1 method call from NetworkSceneManager and 1 method from SceneEventData. * style Removed namespace no longer being used. (Standards check failed) * `./standards.py --fix` Co-authored-by: Luke Stampfli <43687322+LukeStampfli@users.noreply.github.com> Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com>
MTT-820
Summary
As described in the RFC for this PR, in order for users to be able to use additive scene loading and benefit from multi-scene editing workflows there are some base-line features that Netcode for GameObjects needs to support.
This is the second and third phase of the NetworkSceneManager refactoring and preparation for additive scene loading.
Acceptance Criteria
Tests: