Skip to content

Conversation

@cabanier
Copy link
Contributor

@cabanier cabanier commented Dec 23, 2020

Description
The WebXR API has been updated per feedback from the W3C: https://immersive-web.github.io/webxr-hand-input
This breaks the current implementation.

This patch will update three.js so it will work with the new API. However, it will no longer work with the old API.
Since most headsets are updated quickly, this shouldn't cause too many problems.

This patch should only be merged once the API goes live.

This contribution is funded by Oculus.

@cabanier
Copy link
Contributor Author

@fernandojsg can you take a look?
I can also send you a private build if it helps.

@fernandojsg
Copy link
Collaborator

@cabanier it looks good to me. The only thing I don't really like is using numbers to refer to the joints, it makes the code harder to read. Can't we just create constants with these names and expose them directly in three even if the WebXR doens't include these constants anymore?

// Previously we had
const indexTip = controller.joints[ XRHand.INDEX_PHALANX_TIP ];

// This PR proposal
const indexTip = controller.joints[ 9 ];

// Something more readable ?
const indexTip = controller.joints[ THREE.XRHand.INDEX_PHALANX_TIP ];

@cabanier
Copy link
Contributor Author

@cabanier it looks good to me. The only thing I don't really like is using numbers to refer to the joints, it makes the code harder to read. Can't we just create constants with these names and expose them directly in three even if the WebXR doens't include these constants anymore?

...
// Something more readable ?
const indexTip = controller.joints[ THREE.XRHand.INDEX_PHALANX_TIP ];

@fernandojsg Where would I define THREE.XRHand.INDEX_PHALANX_TIP?

@cabanier
Copy link
Contributor Author

@fernandojsg I decided to get rid of the indexes and move to using joint names.

@mrdoob mrdoob added this to the r125 milestone Dec 29, 2020
@mrdoob
Copy link
Owner

mrdoob commented Dec 29, 2020

Looking good!

This patch should only be merged once the API goes live.

When do you think this would be?

@cabanier
Copy link
Contributor Author

Looking good!

Thanks!

This patch should only be merged once the API goes live.

When do you think this would be?

I believe sometime in January.

@mrdoob
Copy link
Owner

mrdoob commented Dec 29, 2020

Okay! Let us know when it's out and I'll merge this 👍

@cabanier cabanier changed the title Updated hand implementation to the new API Updated WebXR Hand implementation to the new API Jan 13, 2021
@cabanier
Copy link
Contributor Author

@mrdoob The new Oculus browser with the updated API is now rolling out to users.
Should we merge this PR?

@mrdoob mrdoob merged commit 713c426 into mrdoob:dev Jan 22, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jan 22, 2021

Thanks!

handPose = true;

for ( let i = 0; i <= window.XRHand.LITTLE_PHALANX_TIP; i ++ ) {
for ( const inputjoint of inputSource.hand.values() ) {
Copy link
Owner

Choose a reason for hiding this comment

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

Is it possible to turn this into a simple for loop instead of a for of loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be the advantage of doing it that way?

Copy link
Owner

Choose a reason for hiding this comment

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

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.

3 participants