-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add a multistep configurator to facilitate project setup after importing MRTK #9652
Conversation
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Nice addition as a walk-through guide for new users. Might also suggest a last step for additional reading and pointers to getting started guides (with pretty pictures :D ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code design side of things looks good, will pull down and make pass of running through the tool itself and add more comments in the future.
Assets/MRTK/Core/Inspectors/Setup/MixedRealityEditorSettings.cs
Outdated
Show resolved
Hide resolved
Assets/MRTK/Core/Inspectors/Setup/MixedRealityProjectConfiguratorWindow.cs
Outdated
Show resolved
Hide resolved
{ MRConfig.SpatialPerceptionCapability, true }, | ||
public static MixedRealityProjectConfiguratorWindow Instance { get; private set; } | ||
|
||
public static bool IsOpen => Instance != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can Instance ever be set to null? Double checking where how this variable is used even though this was the way we did it before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping on this @MaxWang-MS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instance cannot be set to null but by default it is null before initialization. We have static functions to show the window and we use this to see if there has been an instance of the window that we can reuse.
Assets/MRTK/Core/Inspectors/Setup/MixedRealityProjectConfiguratorWindow.cs
Outdated
Show resolved
Hide resolved
Co-Authored-By: RogPodge <[email protected]>
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
} | ||
if (GUILayout.Button("Learn more")) | ||
{ | ||
Application.OpenURL(XRPipelineDocsUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clicking on this button didn't open a URL for me in any of my browsers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is expected as the page has yet to be created... That will be a separate PR in a separate repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can alter the button behavior to make it clearer to users that the page is not yet available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just updated the string to point it to a page on our MR docs site describing different Unity versions and choices of plugins. I will update the link to the new MRTK docs page once it gets published.
Assets/MRTK/Core/Inspectors/Setup/MixedRealityProjectConfiguratorWindow.cs
Outdated
Show resolved
Hide resolved
Assets/MRTK/Core/Inspectors/Setup/MixedRealityProjectConfiguratorWindow.cs
Outdated
Show resolved
Hide resolved
Tested out in editor and noticed a few readability/wording fixes. Looks good and effective otherwise! |
Co-Authored-By: RogPodge <[email protected]> Co-Authored-By: Kurtis <[email protected]>
None of the remaining requested changes are blocking
Assets/MRTK/Core/Inspectors/Setup/MixedRealityEditorSettings.cs
Outdated
Show resolved
Hide resolved
Co-Authored-By: RogPodge <[email protected]> Co-Authored-By: Kurtis <[email protected]>
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Assets/MRTK/Core/Inspectors/Setup/MixedRealityProjectConfiguratorWindow.cs
Outdated
Show resolved
Hide resolved
Assets/MRTK/Core/Inspectors/Setup/MixedRealityProjectConfiguratorWindow.cs
Outdated
Show resolved
Hide resolved
{ | ||
BuildTargetGroup currentBuildTarget = BuildPipeline.GetBuildTargetGroup(EditorUserBuildSettings.activeBuildTarget); | ||
XRGeneralSettings settingsOfCurrentTarget = XRGeneralSettingsPerBuildTarget.XRGeneralSettingsForBuildTarget(currentBuildTarget); | ||
#pragma warning disable CS0618 // Suppressing the warning to support xr management plugin 3.x and 4.x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a different way we should be getting the loaders wherever the warning is happening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intended as I do not want to add more version defines... Again this is needed to support 3.x and 4.x XR Plugin Management package.
Assets/MRTK/Core/Inspectors/Setup/MixedRealityProjectConfiguratorWindow.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Kurtis <[email protected]>
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't yet pulled it down to try the flow myself, but the code seems good and the demos I've seen are awesome!
{ MRConfig.SpatialPerceptionCapability, true }, | ||
public static MixedRealityProjectConfiguratorWindow Instance { get; private set; } | ||
|
||
public static bool IsOpen => Instance != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping on this @MaxWang-MS
Overview
This PR adds a multistep configurator to help guide users through the project setup phase after importing MRTK. The configurator is based on the original one-page window and now covers more settings. The configurator aims to provide a more streamlined experience for novices and pros alike.
Features
Screenshots
Please note the "TO DO" shown in some of the following screenshots are not by mistake - the documentation page has yet to be created and thus TO DO is used as a placeholder.
Changes