Skip to content

fix: client reconnect using NetworkSceneTable state [MTT-3363] #1886

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

Closed

Conversation

NoelStephensUnity
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity commented Apr 19, 2022

(WIP)Refactoring: removing public facing API changes
When a client is disconnected from a network session for external reasons there is no easy way for a user to reconnect the client without first unloading all scenes loaded by the server and have the client reload all scenes.

This PR introduces some additional methods that allows a user to save off the current NetworkSceneTable state that can be applied when reconnecting in order to avoid having to reload already loaded scenes.

MTT-3363

Changelog

  • Added: NetworkSceneManager.GetNetworkSceneTableState and NetworkSceneManager.SetNetworkSceneTableState to provide users with a way to reconnect to a network session without having to unload and reload the scenes.

Testing and Documentation

  • Includes manual testing capabilities in SceneTransitioningAdditive.
  • Includes integration test.
  • Includes integration test updates.

MTT-3285
This fixes the issue with ValidateSceneBeforeLoading not continuing to synchronize if one scene is skipped over.
adding some debug information (LogLevel.Developer) so users can track when a client declines to load a scene
This is the primary full fix for client reconnection and restoration of already loaded scenes.
Still looking into integration tests, but this includes an update to SceneTransitioningAdditive that allows the host/server to disconnect all connected clients and the clients will automatically attempt to reconnect without having to exit SceneTransitioningAdditive and enter back into it again using the fix applied in this PR.
This also provides a way to manually test clients disconnecting and unloading some of the scenes loaded prior to the client being disconnected to assure the client will always have the appropriate scenes loaded and properly synchronizes dynamically spawned and in-scene placed NetworkObjects.
Removing white spaces
Fixing and issue with the manual test making the disconnect clients button hidden on server after scene transitioning.
minor adjustment for tools SceneEvent test to wait for the resynchronization prior to waiting for the SceneEventType.Load.
removing some extra CR/LFs
Updating SceneTransitioningAdditive test to use UnityTransport and making sure the scene to switch to has a disconnect client button.
Updating some edge case scenarios to take into consideration when disconnecting and reconnecting a client and the object pools.
Making the ClientDisconnectHandler only implement the auto flush/unload scenes no longer loaded when reconnecting.
Fixes additional exposed issues:
- A very edge case, but possible, scenario where the scene to be flushed was unloaded right as the client reconnects, so just updated NetworkSceneManager to make sure the scene is valid and loaded before attempting to unload it.
- When using UnityTransport and a server disconnects a client, NetworkManager was calling Shutdown() without specifying it should not try to send any outbound messages in the queue since the server-side transport no longer has a connection to the client-side transport.
Adding the ability to start just one client after it has been stopped within NetcodeIntegrationTestHelpers.
Adding OnPostTearDown virtual method to NetcodeIntegrationTest in order to provide developers with a place to, on a per test basis, have something invoked just after all NetworkManagers were shutdown.
This is the integration test to validate the changes in this PR.
Some minor adjustments made to fix some bugs in the integration test and to get it passing when running as a stand alone testrunner build.
removing cr/lf
finishing a comment...
Scene event notification queue didn't handle the same NetworkManager disconnecting and reconnecting.
This fixes the issue where once a client disconnects and reconnects (in the SceneTransitioningAdditive manual test) the client stopped showing the scene event notifications.
SceneEventTest:
This fixes an edge case issue with SceneEventTest when listening for the received load messages where in the editor it would almost always have just received the resynchronize message when it started waiting for the load message and the found metric would be incorrect.  However, when running as a stand alone testrunner build it would always have already processed the resynchronize message (on Windows) and so the resolution was to see if the message was a resynchronize message and if so create a new wait metric and listen for the expected Load message.

PacketLossMetricsTests:
I disabled scene management as the simulated packet dropping seems to be impacted possibly by the additional resynchronization message.  Also added wait for condition checks in place of the WaitForMetricsReceived as it appears 60 frames can fail when running on Windows as a stand alone testrunner build.  This is a suggested fix, but will get MTT-Tools to determine how they want to handle this (or if this is fine).
Manual Test Related Fixes
This fixes some additional edge case scenarios that were exposed while testing the client connect-disconnect using NetworkSceneTable state fix for reconnecting a disconnected client.  All of these changes are just to be able to manually test client disconnect-reconnect scenarios (i.e. disconnect a client and switch the scene or unload one scene and load a different scene or any other number of potential combinations using SceneTransitioningAdditive)
Manual Test Fix
Missed one file.
Fixed TrackTotalNumberOfBytesSent tracking the number of bytes received and not sent.

Fixed an edge case scenario where the server sends a resynchronization message when in the middle of tracking the number of bytes sent which cause the test to fail.  Disabled scene management to avoid this scenario for this test since it is really only focusing on testing that sent/received byte metrics are working.
Fixing grammar and readability.
Added some comments.
cleaning up/improving (I hope) some of the comments.
reworded a few comments for readability purposes.
When a scene is not validated on the client side it should invoke HandleClientSceneEvent not ClientLoadedSynchronization
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner April 19, 2022 16:32
@NoelStephensUnity NoelStephensUnity marked this pull request as draft April 19, 2022 16:32
Some additional fixes for edge case scenarios (manual test assets and code only)
MTT-3363
Updating the changelog for this addition.
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review April 21, 2022 15:12
removing commented code and adjusting comments.
@NoelStephensUnity NoelStephensUnity marked this pull request as draft April 21, 2022 16:00
This fixes the issue where a client could be disconnected. the server unload and reloads the same scene while the client is disconnected, and then the client reconnects using the NetworkTableState prior to the same scene being unloaded and reloaded.  The issue was that in the GetAndAddNewlyLoadedSceneByName we needed to exclude any scenes that are in the NetworkTableState as they are the previously loaded scenes and GetAndAddNewlyLoadedSceneByName is only invoked if a scene has to be loaded and a scene only has to be reloaded if the NetworkSceneHandle doesn't already exist in the NetworkTableState.
Just added some additional debug information that I think will be useful
Updated comments and keeping some debug information around if the log level is set to developer.
Added additional debug info when a scene load fails.
Added the case for the same scene to be unloaded and reloaded while a client is disconnected and then the client connects.  This added test verifies that the scenes in the NetworkSceneTable state are not taken into consideration when trying to find a newly loaded scene.
This fixes the issue where NetworkManagerOwners are not correct in integration testing.
This also fixes the issue where the NetworkManager name labels did not match the appropriate client id.
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review April 22, 2022 17:35
@NoelStephensUnity NoelStephensUnity marked this pull request as draft April 28, 2022 14:38
@JesseOlmer JesseOlmer changed the title fix: client reconnect using NetworkSceneTable state fix: client reconnect using NetworkSceneTable state [MTT-3363] May 5, 2022
@JesseOlmer JesseOlmer closed this May 5, 2022
@JesseOlmer JesseOlmer reopened this May 5, 2022
@JesseOlmer
Copy link
Contributor

Closed and re-opened to fix the JIRA association. Please make sure to add the JIRA ticket to the title upon PR creation in the future.

@NoelStephensUnity
Copy link
Collaborator Author

closing this PR but preserving the branch

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