Skip to content

fix: imps self-destroying on clients when late-joining with multiple additive scenes loaded #574

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 4, 2022
Merged

fix: imps self-destroying on clients when late-joining with multiple additive scenes loaded #574

merged 6 commits into from
Apr 4, 2022

Conversation

LPLafontaineB
Copy link
Contributor

Description (*)

This PR makes sure ClientCharacterVisualization is only enabled on clients when it is spawned. This prevents Update from being called at the wrong time, causing it to self-destruct. This PR also removes an unneeded property and moves the OnDestroy code to OnNetworkDespawn to better mirror its initialization in OnNetworkSpawn.

Related Pull Requests

Issue Number(s) (*)

Fixes issue(s): MTT-2433

Manual testing scenarios

  1. Start a game as a host
  2. Start the game and move to the second door to load the TransitionArea additive scene
  3. As a client, join the game instance
  4. See that the imps in the first room are visible

Questions or comments

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@LPLafontaineB LPLafontaineB added 2-Easy This PR is trivial and can be reviewed quickly 1-Needs Review PR needs attention from the assignee and reviewers labels Mar 18, 2022
fernando-cortez
fernando-cortez reviewed Mar 28, 2022
View reviewed changes
@fernando-cortez fernando-cortez added 2-Reviewed with Comments PR requires owner's attention and removed 1-Needs Review PR needs attention from the assignee and reviewers labels Mar 28, 2022
@LPLafontaineB LPLafontaineB added the 1-Needs Review PR needs attention from the assignee and reviewers label Mar 29, 2022
fernando-cortez
fernando-cortez reviewed Mar 29, 2022
View reviewed changes
Copy link
Collaborator

@fernando-cortez fernando-cortez left a comment

Choose a reason for hiding this comment

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

Last nitpick, but good to go otherwise.

@fernando-cortez fernando-cortez removed the 1-Needs Review PR needs attention from the assignee and reviewers label Mar 29, 2022
@fernando-cortez fernando-cortez added 2-One More Review One review in, one to go and removed 2-Reviewed with Comments PR requires owner's attention labels Mar 29, 2022
@SamuelBellomo
Copy link
Contributor

Noel is looking at this bug here https://jira.unity3d.com/browse/MTT-2924
Should we put this PR on hold?

@LPLafontaineB
Copy link
Contributor Author

Noel is looking at this bug here https://jira.unity3d.com/browse/MTT-2924 Should we put this PR on hold?

This PR is not related to that bug (I think you have #399 in mind). This is for an issue on our side, where in the case of multiple scenes being loaded additively, when synchronizing, there are a few frames on the client where Update and FixedUpdate are called before OnNetworkSpawn happens.

SamuelBellomo
SamuelBellomo approved these changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@SamuelBellomo SamuelBellomo left a comment

Choose a reason for hiding this comment

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

approving once other conversations are solved

@fernando-cortez fernando-cortez added 3-Good to Merge and removed 2-One More Review One review in, one to go labels Apr 4, 2022
@LPLafontaineB LPLafontaineB merged commit 2adab56 into develop Apr 4, 2022
@LPLafontaineB LPLafontaineB deleted the fix/imps-not-spawning-on-clients branch April 4, 2022 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fernando-cortez fernando-cortez fernando-cortez approved these changes

@LukeStampfli LukeStampfli LukeStampfli approved these changes

@SamuelBellomo SamuelBellomo SamuelBellomo approved these changes

@pdeschain pdeschain Awaiting requested review from pdeschain

Assignees

@pdeschain pdeschain

@fernando-cortez fernando-cortez

Labels
2-Easy This PR is trivial and can be reviewed quickly 3-Good to Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@LPLafontaineB @SamuelBellomo @LukeStampfli @fernando-cortez @pdeschain