Unity-Technologies / EditorXR

Author XR in XR
Other
925 stars 167 forks source link

Remove partner input SDK dependencies #548

Closed amirebrahimi closed 5 years ago

amirebrahimi commented 5 years ago

Purpose of this PR

Remove dependencies on the Oculus and SteamVR plugins Haptics is now cross-platform

Testing status

I've tested importing into an empty project for 2018.3, 2019.1, and 2019.2 2018.3.12f1 is the new minimum required Unity version 2019.1+ works fine

I've manually tested with a Rift using Oculus and OpenVR I've manually tested a Vive using OpenVR

Technical risk

Technical risk: 2 (medium) Halo risk: 2 (medium)

Comments to reviewers

Try and use this for a while for development to see if any bugs crop up I haven't thoroughly tested haptics, so there may be some bugs there

Corysia commented 5 years ago

I'm really excited about this PR. Half the time, I'm editing my project on a Mac and it doesn't play well with SteamVR and/or the Oculus Utilities being in my asset tree. Is it possible to try this out in 2019.2?

amirebrahimi commented 5 years ago

Yes, this currently works on latest 2019.1 and 2019.2. Backport to 2018.3 is in queue, but not sure what version it will land.

Corysia commented 5 years ago

Yes, this currently works on latest 2019.1 and 2019.2. Backport to 2018.3 is in queue, but not sure what version it will land.

It appears UnityEngine.SpacialTracking was removed in 2019.2.

amirebrahimi commented 5 years ago

You have to add back the legacyinputhelpers package in the package manager. It might be a preview package still, so check that box if you don’t see it.

Corysia commented 5 years ago

You have to add back the legacyinputhelpers package in the package manager. It might be a preview package still, so check that box if you don’t see it.

Great! That worked. It's an empty project, but Mac is happy, which is a great sign for me. I'll play around with it tonight in Windows and more on the Mac tomorrow. Thank you very much for the help.

amirebrahimi commented 5 years ago

Okay, so all dependencies are now in. Verified that 2018.3.12f1 has support (missing piece). I'd say we're good for final review and merge back.

mtschoen-unity commented 5 years ago

I did some testing on this last night and pushed some changes to the readme and moved the new SeedXRInputBindings to the editor assembly to fix builds. Input and haptics on Rift and Vive worked for me. Great work.

I was noticing an issue where one of the vive grips was "stuck on" during one test run, but I remember this being a hardware issue in the past. I wasn't able to reproduce the issue on current development, but I also wasn't able to reliably reproduce it on this branch.

The tests currently fail, so I'll look into that today. The compilation tests fail because some of the new code relies on C# 6.0 and, as far as I can tell, we can't tell EditorUtility.CompileCSharp to compile against the new runtime. I'll see if an mcs.rsp file would work here. Otherwise, we might just have to cut the CompiliationTests.

The build tests are also failing, which looks to be an issue with legacyinputhelpers. It appears that there is already an internal SpatialTracking.dll that gets built into players, and the build pipeline tries to add it twice because the inputhelpers package defines its runtime assembly by the same name. It seems that this is required for the TrackedPoseDriver implementation therein to work, so that it has access to certain internals. I'll look into whether we can work around this issue.

When this merges in, I'll mark the parent commit with a 2018.2 tag to indicate that it is the last version supported by 2018.2. It's unfortunate that EditorXR will now cause compiler errors in older versions of Unity, or in the legacy runtime. If this becomes an issue, we can just bring back the compiler directives to at least suppress errors, even if the tools won't actually work in earlier versions.

mtschoen-unity commented 5 years ago

Update from today: The issue with legacyinputhelpers was my misunderstanding. It should not be used in 2018.3, but is necessary in 2019.1 and above. For 2018.3, just the empty asmdef is enough to satisfy the reference in EXR.asmdef, and everything else works just fine. I've updated the Readme again to reflect this.

The CompileTest issue was simple enough: there was only one place where we're using C# 6.0 features, (an out var) so I switched it to use legacy syntax. I have brought this up with the scripting team to see if there is a way to do these kinds of tests with the new runtime going forward.

There was another test breakage that took most of the day to track down. Because of the new input setup, the EditingContextManagerTests end up triggering the proxy's activeChanged, which triggers the set up of proxies, menus, etc. This ended up leaving a bunch of DontSave cruft in the scene which, when the tests finish and a new scene is loaded, trigger rather mysterious Errors reading Can't destroy Transform component of 'SomeEXRObject'. I tracked it down to this issue https://issuetracker.unity3d.com/issues/unity-test-tools-is-attempting-to-destroy-a-transform-component which seems to suggest that any objects marked DontSave that exist after a test run will cause this issue.

The fix was just to call VRView.activeView.Close(); to trigger a normal EXR shutdown. We should fix this to properly shut down EXR without having to reference the view, since it shouldn't be entirely necessary to manage context lifecycles. I moved the ObjectUtils.Destroy(go) to the setup method because that gameobject and its components don't need to stick around during the tests. I wanted to rule it out as something which would interfere with the Teardown procedure.

I'm not sure if this was a pre-existing issue, but I also added a EditorApplication.delayCall -= OnSelectionChanged; in EditorVR's OnDestroy. This prevents a MissingReferenceException which occurs after these tests are run. The tests execute synchronously, and by the time that delayCall fires, the objects which it accesses have been destroyed.

Finally, I was about to delete the NoEditorVR test, since we decided not to fix compile errors in earlier versions of Unity. However, I decided to try and see what the minimum amount of effort would be to fix them, and it's not that bad. Basically EditorVR and all of its Nested classes get wrapped in #if UNITY_2018_3_OR_NEWER, and a couple of other classes that use newer APIs get the same treatment. It was only a couple of dozen files at the end of the day. Now we get the nice behavior where a dialog box appears telling you to download 2018.3.12 or newer.

mtschoen-unity commented 5 years ago

One last note on this: I've noticed a bug in player builds where the spatial menu gets stuck-on when using Vive controllers. This is likely related to how ~Vive~ hand indices are determined. Should be a simple fix, which I'll update in a later PR.

Edit: the bug is present on both Vive and Rift