Skip to content
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

updated playmode/editor mode change transitions #704

Merged

Conversation

StephenHodgson
Copy link
Member

@StephenHodgson StephenHodgson commented Nov 27, 2020

XRTK - Mixed Reality Toolkit Pull Request

Overview

  • Disable and destroy all services as soon as we get the quitting event
  • Properly use nameof
  • Fixed manifest

Disable and destroy all services as soon as we get the quitting event
@StephenHodgson StephenHodgson added Bug Something isn't working Ready for review PR finished primary development, open for review labels Nov 27, 2020
SimonDarksideJ
SimonDarksideJ previously approved these changes Nov 27, 2020
Copy link
Contributor

@SimonDarksideJ SimonDarksideJ left a comment

Choose a reason for hiding this comment

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

Makes sense, testing will tell :D

@StephenHodgson StephenHodgson merged commit 607b7f6 into development Nov 28, 2020
@StephenHodgson StephenHodgson deleted the fix/errors-when-exiting-playmode-in-editor branch November 28, 2020 02:21
@@ -274,7 +274,12 @@ private void InitializeInstance()
DontDestroyOnLoad(instance.transform.root);
}

Application.quitting += () => IsApplicationQuitting = true;
Application.quitting += () =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't have used a lambda here but I guess it's too late.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a personal preference. Application exit and cleanup is an important event and imo deserves a proper method definition. That makes it for others easier to find the code executed when the toolkit is "deinitialized" in the future, instead of having to find it somewhere in between other code.

But as I said, it's not super important. I just wouldn't have done it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an event you can subscribe to anywhere. We do provide this public flag for if the application is quitting via the property, but I don't really think it makes a difference if the Application.quitting event can be subscribed from anywhere in the codebase

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you misunderstood. All I am saying is I'd create a definition private void MixedRealityToolkit_OnQuitting() in MixedRealityToolkit instead of an anonymous method. Because that way if someone is looking up "What does the XRTK do, when the application quits?", it is much easier to find. All good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that makes sense !👍

XRTK-Build-Bot pushed a commit that referenced this pull request Dec 25, 2020
* updated playmode/editor mode change transitions

Disable and destroy all services as soon as we get the quitting event

* use nameof

* fixed project manifest

* properly use nameof

* fixed tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Ready for review PR finished primary development, open for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants