Skip to content

fix: Migrate OnClientConnectedCallback invocation into StartHost [MTT-4972] #2277

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

Merged
merged 5 commits into from
Oct 26, 2022

Conversation

NoelStephensUnity
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity commented Oct 25, 2022

This assures that any NetworkBehaviour components attached to in-scene placed NetworkObjects will have their associated netcode/NetworkManager properties properly initialized when OnClientConnected is invoked.

MTT-4972

Changelog

  • Fixed: The issue where NetworkManager.OnClientConnectedCallback was being invoked before in-scene placed NetworkObjects had been spawned when starting NetworkManager as a host.

Testing and Documentation

  • Includes integration test (wip)

migrate InvokeOnClientConnectedCallback for host only.
adding comment
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner October 25, 2022 20:19
@JesseOlmer
Copy link
Contributor

Is there a bug # this is associated with?

HandleConnectionApproval just above in the same method invokes onclientconnected... either directly if scene management is disabled, or at the end of the Synchronize scene event if scene management is enabled.

Isn't this change just going to fire it multiple times?

@NoelStephensUnity NoelStephensUnity marked this pull request as draft October 25, 2022 21:23
@NoelStephensUnity
Copy link
Collaborator Author

NoelStephensUnity commented Oct 25, 2022

Is there a bug # this is associated with?

HandleConnectionApproval just above in the same method invokes onclientconnected... either directly if scene management is disabled, or at the end of the Synchronize scene event if scene management is enabled.

Isn't this change just going to fire it multiple times?

Yeah, I forgot to mark it as draft. It was just a quick first pass example of what it would sort of look like. (still needs a ticket)
Whether scene management is enabled or disabled, it will only get invoked in StartHost (clients will only execute that block of code I think you are referring to within HandleConnectionApproved)
It will not get invoked within NetworkSceneManager during a Synchronize scene event as that is only invoked on clients and not on the server-side (server is already synchronized).

@NoelStephensUnity NoelStephensUnity changed the title fix: Migrate OnClientConnected into StartHost fix: Migrate OnClientConnected into StartHost [MTT-4972] Oct 26, 2022
@NoelStephensUnity NoelStephensUnity changed the title fix: Migrate OnClientConnected into StartHost [MTT-4972] fix: Migrate OnClientConnectedCallback invocation into StartHost [MTT-4972] Oct 26, 2022
MTT-4972
Added the fixed entry for this PR.
MTT-4972
adding validation test to existing NetworkManagerTests
MTT-4972
Updating the integration test to validate that this update does not impact anything when scene management is disabled.
Unloading the loaded scene.
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review October 26, 2022 01:34
@NoelStephensUnity NoelStephensUnity merged commit b5f82ee into develop Oct 26, 2022
@NoelStephensUnity NoelStephensUnity deleted the fix/host-onclientconnected-migration branch October 26, 2022 23:55
jakobbbb pushed a commit to GooseGirlGames/com.unity.netcode.gameobjects that referenced this pull request Feb 22, 2023
…-4972] (Unity-Technologies#2277)

* fix
Migrate InvokeOnClientConnectedCallback into StartHost (for host only).

* test
Updating the NetworkManagerTests to validate this PR, it also validates that this update does not impact anything when scene management is disabled.
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