Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

Fix WMR hand controller profiles referencing SDK #102

Merged
merged 2 commits into from
Nov 18, 2020

Conversation

FejZa
Copy link
Contributor

@FejZa FejZa commented Nov 18, 2020

XRTK - Mixed Reality Toolkit Pull Request

Overview

Created duplicate profiles required for the hand controller on the WMR platform. It's unfortunate that we have to
duplicate profiles this way but until we find another solution, if that's needed, this fixes the issue with the profiles being broken
in the current preview release.

Changes

  • Created profiles for WMR hand controller configuration

@FejZa FejZa self-assigned this Nov 18, 2020
@FejZa FejZa added the Ready for review PR finished primary development, open for review label Nov 18, 2020
@StephenHodgson
Copy link
Contributor

We can probably remove the ones in the SDK then

@FejZa
Copy link
Contributor Author

FejZa commented Nov 18, 2020

They are unfortunately needed for the simulated hand still

@StephenHodgson
Copy link
Contributor

Shouldn't the simulated hand profiles stand on their own?

@FejZa
Copy link
Contributor Author

FejZa commented Nov 18, 2020

Not sure I can follow. Simulated hands live in XRTK-Core and Core does not have any profiles right? So simulated needs to rely on SDK being imported to work.

@StephenHodgson
Copy link
Contributor

StephenHodgson commented Nov 18, 2020

Right, so then why is it required for simulated hands in your prev comment?

@FejZa
Copy link
Contributor Author

FejZa commented Nov 18, 2020

If we remove the hand input profiles from SDK, then simulation will be broken since it's using them.

@StephenHodgson
Copy link
Contributor

I get that we need to keep the hand profiles required, but just remove the wmr specific ones.

@StephenHodgson
Copy link
Contributor

How did we fix this in Oculus @SimonDarksideJ ?

@FejZa
Copy link
Contributor Author

FejZa commented Nov 18, 2020

Ah now I understand haha. There was no WMR specifc ones in SDK. So far I've been using the same profiles for Simulation, WMR and Oculus. But now I'll have to have each platform have their own. Although they are the same value wise.

@SimonDarksideJ
Copy link
Contributor

Oculus has the same issue @StephenHodgson , We found that Oculus and WMR are sharing the Hands Configuration in the SDK. Since SDK is optional, it would break those platforms if it was there.

Same issue as we had with the controllers, which was fixed by creating profiles IN those platforms and assigning them correctly.

@FejZa
Copy link
Contributor Author

FejZa commented Nov 18, 2020

Oculus was fixed by doing the same thing. @SimonDarksideJ just did it for Oculus Touch etc. but not for hands though. I think hands weren't merged back then. So I'll catch up now and do the same thing for Oculus hands.

Copy link
Contributor

@SimonDarksideJ SimonDarksideJ left a comment

Choose a reason for hiding this comment

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

Looks good, will just have to wait until it's merged and a new preview asset generated to test.

@StephenHodgson StephenHodgson merged commit 1ce2685 into development Nov 18, 2020
@StephenHodgson StephenHodgson deleted the fix/hand-profiles branch November 18, 2020 22:36
XRTK-Build-Bot pushed a commit that referenced this pull request Dec 25, 2020
* Fix WMR hand controller profiles referencing SDK

* Move data provider profile to hands folder
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Ready for review PR finished primary development, open for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants