Skip to content

fix: NetworkManager doesn't destroy itself on multi instance #765

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 16 commits into from
Apr 29, 2021

Conversation

TwoTenPvP
Copy link
Contributor

No description provided.

@@ -721,14 +721,6 @@ private void Awake()

private void OnEnable()
{
if (Singleton != null && Singleton != this)
{
Destroy(gameObject);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super familiar with what this code does, but when I read the PR title I anticipated to see "if multi-scene, don't destroy, else still do" but what I see here is we don't destroy the game object anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it makes no sense for us to destroy it. I can't remember why I did it that way.

Basically what this used to do was:

If there was a NetworkManager already, destroy myself.

Now it doesn't destroy it anymore. That's the only difference.

Copy link
Contributor

@0xFA11 0xFA11 left a comment

Choose a reason for hiding this comment

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

see my comments for 1 minor, 1 major changes.

Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

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

Other than the outstanding comments/request for change.
The rest looks good to me.

Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com>
Base automatically changed from singleton-removal-find-objects to develop April 27, 2021 20:48
@TwoTenPvP TwoTenPvP enabled auto-merge (squash) April 27, 2021 20:53
@TwoTenPvP TwoTenPvP disabled auto-merge April 29, 2021 10:39
@TwoTenPvP TwoTenPvP enabled auto-merge (squash) April 29, 2021 11:11
@TwoTenPvP TwoTenPvP merged commit 3e1aef3 into develop Apr 29, 2021
@TwoTenPvP TwoTenPvP deleted the singleton-removal-singleton-set branch April 29, 2021 13:28
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.

4 participants