Skip to content

fix: defer ObjectSceneChanged events to occur after client has finished synchronizing#2502

Merged
ShadauxCat merged 6 commits intorelease/1.4.0from
fix/objectscenechanged-defer-after-synchronize
Apr 12, 2023
Merged

fix: defer ObjectSceneChanged events to occur after client has finished synchronizing#2502
ShadauxCat merged 6 commits intorelease/1.4.0from
fix/objectscenechanged-defer-after-synchronize

Conversation

@NoelStephensUnity
Copy link
Copy Markdown
Member

Fixes the issue where a client could receive a SceneEventType.ObjectSceneChanged message while in the middle of synchronization which could cause an exception to be thrown due to a NetworkObject not yet being instantiated and spawned.

MTT-6188

Changelog

  • Fixed: Issue where during client synchronization the synchronizing client could receive a ObjectSceneChanged message before the client-side NetworkObject instance had been instantiated and spawned.

Testing and Documentation

  • Includes integration test updates to NetworkObjectSceneMigrationTests.MigrateIntoNewSceneTest
  • No documentation changes or additions were necessary.

This resolves the issue with NetworkObject scene change notifications that are received by a client while the client is performing its initial synchronization.
Needed to make the DeferredObjectsMovedEvents live in the client's NetworkSceneManager instance since each SceneEventData message is a unique instance.
Validates that a client synchronizing will defer SceneEventType.ObjectSceneChanged scene event messages until the client is finished synchronizing.
renaming SceneEventData.ProcessDeferredObjectsMovedEvents to SceneEventData.ProcessDeferredObjectSceneChangedEvents for better clarity of what the method is processing.
Adding changelog entry
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review April 12, 2023 01:43
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner April 12, 2023 01:43
updating comment to reflect the correct method calling NetworkSceneManager.MigrateNetworkObjectsIntoScenes

var deferredObjectsMovedEvent = new NetworkSceneManager.DeferredObjectsMovedEvent()
{
ObjectsMigratedTable = new Dictionary<int, List<ulong>>()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since these can be created dynamically at any time here, I worry about the GC pressure that might be added if a lot of these come in rapidly. Could this be changed from Dictionary<int, List<ulong>> to NativeHashMap<int, NativeList<ulong>>? Allocating with Allocator.Persistent would be required but that's still better than GC allocation... especially given the potential number of calls to new List<ulong>() below.

Copy link
Copy Markdown
Member Author

@NoelStephensUnity NoelStephensUnity Apr 12, 2023

Choose a reason for hiding this comment

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

This will only happen under the following conditions:

  • A client is going through the initial synchronization process after the client has connected and the connection has been approved.
  • While the client is processing the synchronization message (i.e. loading scenes, instantiating NetworkObjects, etc) the client receives and processes a ObjectSceneChanged event.

So, regarding memory allocation it is basically during a period of time where GC will always be at its "highest peak". Once a client is synchronized this section of code will never get invoked...so in reality the DeferredObjectsMovedEvent structure wont be allocated at any time... only during client synchronization and only if the client receives any ObjectSceneChanged events while processing the synchronization message...which each ObjectSceneChanged event can contain (n) number of scene handles with associated NetworkObject identifiers (i.e. the server batches NetworkObject scene changes together for each frame... so if a user migrates 100 NetworkObjects into a different scene all at once there will be 1 ObjectSceneChanged event that contains the target scene handle and all of the NetworkObject identifiers to migrate into the target scene...which under this scenario would allocate 1 DeferredObjectsMovedEvent structure to defer the processing of that event to after the client has finished synchronizing).

@ShadauxCat ShadauxCat merged commit e2582e4 into release/1.4.0 Apr 12, 2023
@ShadauxCat ShadauxCat deleted the fix/objectscenechanged-defer-after-synchronize branch April 12, 2023 17:33
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.

2 participants