Skip to content

MRTK-Quest integration/documentation #8259

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.

Sign up for GitHub

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

Jump to bottom
Merged
RogPodge merged 37 commits into from
Aug 25, 2020
Merged

MRTK-Quest integration/documentation #8259

RogPodge merged 37 commits into from
Aug 25, 2020

Conversation

@RogPodge
Copy link
Contributor

@RogPodge RogPodge commented Aug 5, 2020 •
edited
Loading

Overview

PR incorporating MRTK-Quest into MRTK. This PR is focused on getting in the MRTK-Quest handtracking components into MRTK in order to greenlight that feature for developers. MRTK-Quest as a whole as some other features (some hand gesture utilities, hand teleport pointer) which need additional evaluation and work before pulling them in.

One thing to note is that this PR has a commented out section in OculusQuestHand with teleport pointer code. That has been left in for now to make it easier to add back in a future PR.

Changes

@RogPodge RogPodge added Documentation DO NOT MERGE Platform - Oculus Issues specific to Oculus devices and the Oculus platform labels Aug 5, 2020
@RogPodge RogPodge requested review from david-c-kline and wiwei August 5, 2020 23:18
@RogPodge RogPodge self-assigned this Aug 5, 2020
CDiaz-MS
CDiaz-MS reviewed Aug 6, 2020
View reviewed changes
CDiaz-MS
CDiaz-MS reviewed Aug 6, 2020
View reviewed changes
@RogPodge RogPodge changed the title MRTK-Quest packaging pipeline/documentation [Draft] MRTK-Quest packaging pipeline/documentation Aug 6, 2020
@RogPodge RogPodge requested a review from keveleigh as a code owner August 11, 2020 20:03
@keveleigh
Copy link
Contributor

keveleigh commented Aug 11, 2020

Do you think we need to bring in the entirety of MRTK-Quest to enable hand tracking on XR SDK? It feels strange to me to have to use an extension built entirely on the Oculus package in order to light something up in XR SDK. And won't that mean controllers are now tracked by both device managers? They feel like distinct managers that aren't compatible together.

Can we build on the Oculus package directly instead and only reference the hand components? Since, either way (our own hand components vs MRTK-Quest), the Oculus package is required, right?

@RogPodge
Copy link
Contributor Author

RogPodge commented Aug 11, 2020 •
edited
Loading

Do you think we need to bring in the entirety of MRTK-Quest to enable hand tracking on XR SDK? It feels strange to me to have to use an extension built entirely on the Oculus package in order to light something up in XR SDK. And won't that mean controllers are now tracked by both device managers? They feel like distinct managers that aren't compatible together.

Can we build on the Oculus package directly instead and only reference the hand components? Since, either way (our own hand components vs MRTK-Quest), the Oculus package is required, right?

Regarding issues with the controllers being tracked with both device managers, the only issue that we can see within the quest is that you'll see both controller visualizers, which in our case, means you see the basic 3-axis visualization as well as MRTK-Quest's hands.

I don't think we need to include all of MRTK-Quest in order to light up just handtracking, though the MRTK-Quest controller is only 1 file which we don't need to include in our XRSDK controller definitions.

@keveleigh
Copy link
Contributor

keveleigh commented Aug 11, 2020

you'll see both controller visualizers, which in our case, means you see the basic 3-axis visualization as well as MRTK-Quest's hands

Won't that mean two sets of pointers for the controllers too? Which would mean two input events for every button press.

I don't think we need to include all of MRTK-Quest in order to light up just handtracking

I think we should try to light this up without any of MRTK-Quest if possible (well, that or merge them, but I feel like they both have their uses cases separately?). It feels to me like there's too much overlap to have them running at the same time. It also makes some assumptions about camera control and scene set-up that's specific to the OVRPlugin that might be good to revisit, since we'll be using XR SDK to control the camera.

Of course, you've been looking into this more than me! So, you'll know best, but it feels like something self-contained to our existing provider would work best imo.

@elbuhofantasma
Copy link
Contributor

elbuhofantasma commented Aug 14, 2020

The Build and deploy your project to Oculus Quest section has a broken image link.

https://github.com/microsoft/MixedRealityToolkit-Unity/blob/f66038c57983d75bf26b6fcef75c8f8a08a038c7/Documentation/CrossPlatform/OculusQuestMRTK.md#build-and-deploy-your-project-to-oculus-quest

wiwei
wiwei reviewed Aug 17, 2020
View reviewed changes
Assets/MRTK.ThirdParty/MRTK-Quest/Editor/MRTKQuestConfigurationChecker.cs
OVRProjectConfig projectConfig = AssetDatabase.LoadAssetAtPath<OVRProjectConfig>(configPath);
projectConfig.handTrackingSupport = OVRProjectConfig.HandTrackingSupport.ControllersAndHands;
AssetDatabase.Refresh();
#endif
Copy link
Contributor

@wiwei wiwei Aug 17, 2020

Choose a reason for hiding this comment

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

Also wondering - if OCULUSINTEGRATION_PRESENT isn't set, I assume that having this menu item greyed out would be optimal, but also this would take effort to figure out if it's needed - maybe just log something to console if this isn't set (i.e. "we did nothing because Oculus Integration hasn't been configured"?)

@RogPodge @wiwei
Update Assets/MRTK.ThirdParty/MRTK-Quest/Editor/MRTKQuestConfiguratio…
1443385 
…nChecker.cs

Co-authored-by: Will <wiwei@microsoft.com>
@RogPodge
Copy link
Contributor Author

RogPodge commented Aug 21, 2020

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 21, 2020

Azure Pipelines successfully started running 2 pipeline(s).

keveleigh
keveleigh reviewed Aug 21, 2020
View reviewed changes
...oviders/Oculus/XRSDK/MRTK.ThirdParty/MRTK-Quest/Scripts/Input/Controllers/OculusQuestHand.cs Outdated
#endregion IMixedRealityHand Implementation


public override MixedRealityInteractionMapping[] DefaultInteractions => new[]
Copy link
Contributor

@keveleigh keveleigh Aug 21, 2020

Choose a reason for hiding this comment

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

Maybe a stretch goal, but it'd be great if we eventually reconcile this class with the ArticulatedHandDefinition class, which tries to standardize several common hand concepts, like these default interactions.

Other hand classes use it like

MixedRealityToolkit-Unity/Assets/MRTK/Providers/LeapMotion/LeapMotionArticulatedHand.cs

Line 43 in 0fa524a

public override MixedRealityInteractionMapping[] DefaultInteractions => handDefinition?.DefaultInteractions;

...oviders/Oculus/XRSDK/MRTK.ThirdParty/MRTK-Quest/Scripts/Input/Controllers/OculusQuestHand.cs
}

/*
private void UpdateTeleport()
Copy link
Contributor

@keveleigh keveleigh Aug 21, 2020

Choose a reason for hiding this comment

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

Should all this teleport logic just be removed, since it's commented out? I think there are some other references to it, and some custom teleport classes being checked in (like CustomTeleportCursorHandler) and a CustomTeleportPointer reference higher up in this class.

Copy link
Contributor Author

@RogPodge RogPodge Aug 21, 2020

Choose a reason for hiding this comment

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

I think we want to add it back in after this first round of PRs, I was going to leave it in to make it easier to test/vet when we get to that point

Assets/MRTK/Providers/Oculus/XRSDK/OculusXRSDKDeviceManager.cs
if (useAvatarHands)
{
// Initialize the local avatar controller
GameObject.Instantiate(MRTKOculusConfig.Instance.LocalAvatarPrefab, cameraRig.trackingSpace);
Copy link
Contributor

@keveleigh keveleigh Aug 21, 2020

Choose a reason for hiding this comment

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

This is maybe a larger question, but do you know if we're able to talk directly to whatever APIs this avatar prefab gets the hand data from instead of needing dependencies on in-scene components from another SDK / toolkit?

Copy link
Contributor Author

@RogPodge RogPodge Aug 21, 2020

Choose a reason for hiding this comment

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

The local avatar prefab is just used to show some kind of hand model when using the quest controllers.

How things look when using the quest controllers
Without MRTK Quest
just the standard 3 axis XRSDK controllers

With MRTK Quest
When Render Avatar Hands checked you get 3d hand meshes thanks so the LocalAvatarPrefab
image

If Render Avatar Hands is unchecked you get generic oculus quest controllers visualized

Copy link
Contributor

@keveleigh keveleigh Aug 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder in this case if we need a dependency on something in the Oculus package for rendering, or could we use the rigged hands that were added recently? Since I think they're based on the joint events, it should "just work"™ and it'll be done in a bit of a more cross-platform way

Copy link
Contributor

@keveleigh keveleigh Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@RogPodge Did this get looked into or was it added?

Copy link
Contributor Author

@RogPodge RogPodge Sep 4, 2020

Choose a reason for hiding this comment

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

We ended up keeping the dependency on the Oculus package for the time being, I'll make an issue to track this improvement in a future PR.

@RogPodge
Copy link
Contributor Author

RogPodge commented Aug 22, 2020

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 22, 2020

Azure Pipelines successfully started running 2 pipeline(s).

david-c-kline
david-c-kline reviewed Aug 24, 2020
View reviewed changes
Copy link

@david-c-kline david-c-kline left a comment

Choose a reason for hiding this comment

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

a few questions and suggestions

RogPodge and others added 4 commits August 24, 2020 13:19
@RogPodge
Update Assets/MRTK/Providers/Oculus/XRSDK/MRTK.ThirdParty/MRTK-Quest-…
a5b9871 
…Utils/Scripts/HandInput/HandPoseUtils.cs

Co-authored-by: David Kline <davidkl@microsoft.com>
@RogPodge
Update Documentation/ReleaseNotes.md
b41015a 
Co-authored-by: David Kline <davidkl@microsoft.com>
@RogPodge
restructured folders, addressed documentation comments, made hand def…
3ca75fe 
…initions derive from articulated hands
@RogPodge
added #if LATER flags to denote future teleporter pointer work
627c82f
@david-c-kline
Copy link

david-c-kline commented Aug 24, 2020

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 24, 2020

Azure Pipelines successfully started running 2 pipeline(s).

@RogPodge
Copy link
Contributor Author

RogPodge commented Aug 24, 2020

currently failing because of asmdef issues and issues around the unity versions, fixing now

@RogPodge
Copy link
Contributor Author

RogPodge commented Aug 24, 2020

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 24, 2020

Azure Pipelines successfully started running 2 pipeline(s).

@RogPodge RogPodge merged commit e72cfb3 into microsoft:mrtk_development Aug 25, 2020
@RogPodge RogPodge mentioned this pull request Sep 4, 2020
Closed
@RogPodge RogPodge changed the title MRTK-Quest packaging pipeline/documentation MRTK-Quest integration/documentation Sep 10, 2020
This was referenced Sep 10, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@macborow macborow Awaiting requested review from macborow

@MenelvagorMilsom MenelvagorMilsom Awaiting requested review from MenelvagorMilsom

@CDiaz-MS CDiaz-MS Awaiting requested review from CDiaz-MS

@provencher provencher Awaiting requested review from provencher

@wiwei wiwei Awaiting requested review from wiwei

2 more reviewers

@keveleigh keveleigh keveleigh left review comments

@david-c-kline david-c-kline david-c-kline approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@RogPodge RogPodge

Labels

Documentation Platform - Oculus Issues specific to Oculus devices and the Oculus platform

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add Oculus Quest Support for MRTK

7 participants

@RogPodge @keveleigh @elbuhofantasma @david-c-kline @wiwei @provencher @CDiaz-MS