Mrtk forward compatibility #1059
Conversation
…nHodgson/HoloToolkit-Unity into MRTK-ForwardCompatibility
…nHodgson/HoloToolkit-Unity into MRTK-ForwardCompatibility
| return PlayerSettings.GetScriptingBackend(BuildTargetGroup.WSA) == ScriptingImplementation.WinRTDotNET && DotNetAvailable(); | ||
| } | ||
|
|
||
| public static bool DotNetAvailable() |
There was a problem hiding this comment.
nit: I have a preference for naming these function IsDotNetAvailable and IsIl2CppAvailable
There was a problem hiding this comment.
Good point. Sorry I usually do the same.
|
|
||
| if (GUILayout.Button("Open Build Directory")) | ||
| { | ||
| Process.Start("explorer.exe", "/f /open," + BuildDeployPrefs.AbsoluteBuildDirectory); |
There was a problem hiding this comment.
This will open a new explorer window every time. If we used Process.Start(BuildDeployPrefs.AbsoluteBuildDirectory) instead, I think it would re-use the same explorer window and just bring it to the front.
There was a problem hiding this comment.
That was the intention to open a new window each time.
I usually have specific explorer windows open, and would be frustrated if one of them were to change on me unexpectedly.
There was a problem hiding this comment.
Sorry, to clarify the behavior is that it will only reuse the window if one is already open to that exact directory. It won't reuse any windows that are opened to different directories. You can test out the behavior by typing directory names in the Win+R run dialog.
There was a problem hiding this comment.
I also wanted to use /f to give the window focus because without it the editor stays on top.
There was a problem hiding this comment.
nvm, seems to work fine.
| // Build Platform (and save setting, if it's changed) | ||
| string curBuildPlatformString = BuildDeployPrefs.BuildPlatform; | ||
|
|
||
| BuildPlatformEnum buildPlatformOption; |
There was a problem hiding this comment.
How about something like this instead here:
BuildPlatformEnum buildPlatformOption;
if (!Enum.TryParse(curBuildPlatformString, true /*ignoreCase*/, out buildPlatformOption))
{
buildPlatformOption = BuildPlatformEnum.AnyCPU;
}There was a problem hiding this comment.
The problem is that the AnyCPU enum string doesn't match the string we need to match with (Any CPU) which gets passed to the msbuild process later.
There was a problem hiding this comment.
But in that case, it will fail the TryParse and then be set inside the if branch. Granted, that behavior isn't spelled out directly.
There was a problem hiding this comment.
I don't see System.Enum.TryParse. Is this feature .NET > 3.5?
Only System.Enum.Parse.
There was a problem hiding this comment.
Yeah, looks like it was added in 4.0... bummer.
|
Anything else Adam? |
|
@DDReaper which scene setting issues? please make an issue detailing the bugs. |
| } | ||
|
|
||
| Close(); | ||
| } | ||
|
|
||
| protected override void LoadSettings() | ||
| { | ||
| for (int i = (int)SceneSetting.CameraToOrigin; i <= (int)SceneSetting.FieldOfView; i++) | ||
| for (int i = (int)SceneSetting.CameraToOrigin; i <= (int)SceneSetting.CameraToOrigin; i++) |
There was a problem hiding this comment.
This not starting from i=0 causes AddMixedRealityCamera to not appear in the menu
There was a problem hiding this comment.
Fixed in #1072
There was a problem hiding this comment.
I thought I had fixed this. Sorry about that. Must have slipped in my rebase retry.
Redone PR of #1048 (Messed up my rebase)
Gets some of the forwards compatibility issues out of the way.
Most changes are to the Build and Deploy windows and Mixed Reality Toolkit Menus
Fixes #851
Fixes #895
Fixes #1018
Fixes #1019