Skip to content

Conversation

@RogPodge
Copy link
Contributor

Overview

Added vr controller model fallbacks to replaces our previously primitive gizmo visuals.

This PR also makes improvements to the base visualizer, allowing users to specify rotation offsets

Changes

  • Fixes: Several controller bugs to be backfilled here.

@keveleigh
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@SimonDarksideJ
Copy link
Contributor

I agree with @keveleigh , Controller model fallback is built in to the architecture for how the system retrieves the actual model to use:

  • Global Default - used if nothing else configured
  • Controller override - Set per controller

If no controller override is set, the global is used.

So what is the actual issue being resolved?

Plus most of the fixes are changing controller poses and other interactive data, not the model (other than adding new models in to source, which should be avoided, this is at the discretion of the developer in their own project.)

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

I agree with @keveleigh , Controller model fallback is built in to the architecture for how the system retrieves the actual model to use:

  • Global Default - used if nothing else configured
  • Controller override - Set per controller

If no controller override is set, the global is used.

So what is the actual issue being resolved?

Plus most of the fixes are changing controller poses and other interactive data, not the model (other than adding new models in to source, which should be avoided, this is at the discretion of the developer in their own project.)

The fallback logic is sound and works fine. This PR fixes one bug where the Global Default would be used even if the Controller override was specified. Beyond that, it adds some open source models as representations of Generic Motion Controllers/Quest Motion controllers. This was requested by our users, who have expressed some dissatisfaction with our default gizmos.

It also helps bridge the Oculus Quest story for Open XR, since controller models are not yet provided on the platform level for Oculus Devices. Quest Controller models are important for developers considering moving to the OpenXR backend.

@RogPodge RogPodge requested a review from keveleigh April 29, 2022 19:36
@RogPodge
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@RogPodge
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

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 couple minor bits. Otherwise looks good.

@@ -0,0 +1,21 @@
MIT License

Choose a reason for hiding this comment

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

Please be sure to update the NOTICES.md file to indicate our usage of these models.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

cgmanifest.json as well, which can be used to generate parts of the Notices file.

@RogPodge RogPodge force-pushed the controllerModelFallbacks branch from d67fb9b to ac0aefc Compare May 3, 2022 17:52
@RogPodge RogPodge requested a review from david-c-kline May 3, 2022 17:56
@RogPodge
Copy link
Contributor Author

RogPodge commented May 3, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@@ -0,0 +1,8 @@
fileFormatVersion: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

not: should we rename this folder? I feel like the WebXR part of the name can be misleading, and the license itself clarifies the source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what we'd rename it to. Would it be PlayCanvas, since that seems to be the ultimate parent project.
https://developer.playcanvas.com/en/tutorials/webxr-controllerhand-models/

The license mentions Amazon, but I can't quite determine where their involvement started/ended.
https://github.com/immersive-web/webxr-input-profiles/blob/main/packages/assets/LICENSE.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually looking deeper, it does seem to all originate from WebXR, so I'd say we should stick to that name

Copy link
Contributor

Choose a reason for hiding this comment

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

My comment was less around where they originate from (the license succeeds for that, I think?) and more about relating what they mean to MRTK in the folder structure. They aren't specific to WebXR in the way we use them, so I fear that naming could be confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants