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

Update OnInspectorGUI for Toolkit Inspector #10936

Conversation

Proton-V
Copy link
Contributor

Changes

Overview

Fixed micro issue with MixedRealityToolkit Inspector
Issue: error stack when we have active MixedRealityToolkit Inspector in editor and active profile is changed

Fixes

  • OnInspectorGUI method in MixedRealityToolkitInspector.cs

@MaxWang-MS
Copy link
Contributor

Hi @Proton-V, thanks for opening the PR. I think by adding the if statement you are limiting the ability to switch profiles in the inspector to only when the application is not in play mode. I would propose adding an else statement containing the following call:
MixedRealityToolkit.Instance.ActiveProfile = (MixedRealityToolkitConfigurationProfile)activeProfile.objectReferenceValue;
This way one can also switch profiles in the inspector while the application is in play mode.

@Proton-V
Copy link
Contributor Author

@MaxWang-MS Hello. Of course, you're right, thanks!

Co-authored-by: MaxWang-MS <68253937+MaxWang-MS@users.noreply.github.com>
Copy link
Contributor

@MaxWang-MS MaxWang-MS left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Just curious, have you run into situations where an exception was thrown? I am wondering why you added the try catch.

@Proton-V
Copy link
Contributor Author

Proton-V commented Aug 31, 2022

Thanks for the contribution! Just curious, have you run into situations where an exception was thrown? I am wondering why you added the try catch.

I added this for future changes.
Possible that these methods may sometimes work incorrectly (later, currently i dont see this behaviour).

And i think handling error without stop action - normal way.

@MaxWang-MS
Tell me what do you think about it. I will be glad to hear your thought

Co-authored-by: MaxWang-MS <68253937+MaxWang-MS@users.noreply.github.com>
@MaxWang-MS
Copy link
Contributor

Thanks for the contribution! Just curious, have you run into situations where an exception was thrown? I am wondering why you added the try catch.

I added this for future changes. Possible that these methods may sometimes work incorrectly (later, currently i dont see this behaviour).

And i think handling error without stop action - normal way.

@MaxWang-MS Tell me what do you think about it. I will be glad to hear your thought

I don't have a super strong opinion on this, but in general I tend to use try catch when I expect things to sometimes go wrong. At the moment I don't really expect exceptions to be thrown when the profile change is initiated the correct way, so I was wondering why you added them. I don't think it hurts to keep them here though.

@MaxWang-MS MaxWang-MS merged commit f5c599f into microsoft:main Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants