Skip to content

fix: SceneEventProgress OnComplete not invoked on timeouts [MTT-4772] #2222

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 Sep 27, 2022

This PR addresses several issues with SceneEventProgress that could result in it never completing as well as several scenarios where it would never time out.

MTT-4772

Changelog

  • Fixed: issue where SceneEventProgress would not complete if a client late joins while it is still in progress.
  • Fixed: issue where SceneEventProgress would not complete if a client disconnects.
  • Fixed: issues with detecting if a SceneEventProgress has timed out.

Testing and Documentation

  • Includes SceneEventProgress specific integration tests.
  • No documentation changes or additions were necessary.

MTT-4772
This is still WIP.
Either there is an issue with the integration test and disconnecting a client while loading or a bug with some of the updates to handling disconnecting clients during a scene event.
Need to track any clients that disconnected to add them to the list of clients that did not complete the scene event.
Reducing the logic used to determine when the SceneEventProgress is finished.
Renamed a few properties and methods for code readability purposes.
NoelStephensUnity and others added 4 commits September 28, 2022 13:10
Added additional comments for clarity
Fixed the logic behind determining when the SceneEventProgress should complete.
Added several tests to verify that SceneEventProgress will complete if all clients finished the scene event, if clients disconnect while it is still in progress, will time out if one or more clients never finish, and finally will still complete if one or more clients late join while it is still in progress.
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review September 28, 2022 20:58
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner September 28, 2022 20:58
MTT-4772
Updating changelog with what was fixed.
Realized using random to determine if a client should or should not do something could result in no clients having done something (i.e. disconnect, late join, never finish).
This update assures two clients will perform the action being tested.
Completely removed the entire SceneEventAction crazy.
Replaced it with an even more simplified version of SceneEventProgress.
We will no longer allocate memory for SceneEventAction.  😆
(and it is cleaner)  👍
fixing grammar in a comment
removing two CR/LFs
moving a comment above a few lines of code (otherwise it doesn't make sense).
>.<
Last comment adjustment
Renaming the SetSceneAsyncOperation to just SetAsyncOperation.
@NoelStephensUnity NoelStephensUnity merged commit 5da0247 into develop Oct 3, 2022
@NoelStephensUnity NoelStephensUnity deleted the fix/sceneeventprogress-timedout-and-clients-connected-logic-mtt-4772 branch October 3, 2022 19:53
jakobbbb pushed a commit to GooseGirlGames/com.unity.netcode.gameobjects that referenced this pull request Feb 22, 2023
…Unity-Technologies#2222)

* fix
Need to track any clients that disconnected to add them to the list of clients that did not complete the scene event.
Reducing the logic used to determine when the SceneEventProgress is finished.
Renamed a few properties and methods for code readability purposes.
Fixed the logic behind determining when the SceneEventProgress should complete.
Renaming the SetSceneAsyncOperation to just SetAsyncOperation.

* refactor
Completely removed the entire SceneEventAction.
Replaced it with an even more simplified version of SceneEventProgress.
We will no longer allocate memory for SceneEventAction.

* style
Added additional comments for clarity
fixing grammar in a comment
removing two CR/LFs
moving a comment above a few lines of code (otherwise it doesn't make sense).
Last comment adjustment

* test
Added several tests to verify that SceneEventProgress will complete if all clients finished the scene event, if clients disconnect while it is still in progress, will time out if one or more clients never finish, and finally will still complete if one or more clients late join while it is still in progress.
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