Skip to content
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.

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

rotate hand models if webxr api is being used #4396

Merged

Conversation

dbradleyfl
Copy link
Contributor

Description:
Between version 1.0.0 and 1.0.1 the rotation of the hand-controls model changed such that they no longer match the users physical hand positions. They are rotated 90 off of where they should be. I created #4388 and figured I'd propose a solution.

Webxr controller orientation seems to be what is causing this improper rotation on the oculus quest browser (where the webxr api is avialable). In the firefox reality browser (no webxr api currently) on quest the rotation is correct. This change fixes the hand rotation if aframe is using the webXR api.

Changes proposed:

  • check for xr on the navigator in the hand-controls component
  • if xr available the hand models should be rotated
  • if xr not available the hand models will match controller orientations

closed #4389 in favor of this solution

@dmarcos
Copy link
Member

dmarcos commented Jan 13, 2020

Why is the reported orientation different between WebVR and WebXR? Worth investigating to discard underlaying issues

@dbradleyfl
Copy link
Contributor Author

@dmarcos I thought the different orientations reported between VR and XR apis was a known thing based on the two different modelPivotRotation values for the oculus-touch-controls here. The oculus-touch-controls modelPivotRotation value changes depending on which api is being used, so I figured the same treatment hadn't been applied to the hand-controls yet?

@dmarcos
Copy link
Member

dmarcos commented Jan 15, 2020

Proper fix is getting rid of orientationOffset and apply the rotation to the model instead. tracked-controls-webxr doesn't have an orientationOffset property and the value provided is ignored on WebXR.

@dbradleyfl
Copy link
Contributor Author

Thanks for looking into this @dmarcos. Two questions:

By "getting rid of orientationOffset" wouldn't we break the WebVR implementation that depends on that here?

By "apply the rotation to the model instead" do you mean open up the gltf file and rotate it, or do the rotation to the mesh on that line like I did here?

@dmarcos
Copy link
Member

dmarcos commented Jan 15, 2020

I meant getting rid of this line:

https://github.com/aframevr/aframe/blob/master/src/components/hand-controls.js#L184

And apply the same orientation to the hand model here:

https://github.com/aframevr/aframe/blob/master/src/components/hand-controls.js#L195

var handModelOrientation = hand === 'left' ? Math.PI / 2 : -Math.PI / 2;
mesh.rotation.set(0, 0, handModelOrientation); 

@dbradleyfl dbradleyfl force-pushed the fix-hand-controls-for-oculus-touch-webxr branch from c4208a6 to 8921a52 Compare January 15, 2020 05:30
@@ -192,7 +191,8 @@ module.exports.Component = registerComponent('hand-controls', {
self.clips = gltf.animations;
el.setObject3D('mesh', mesh);
mesh.position.set(0, 0, 0);
mesh.rotation.set(0, 0, 0);
var handModelOrientation = hand === 'left' ? Math.PI / 2 : -Math.PI / 2;
Copy link
Member

Choose a reason for hiding this comment

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

We can move the declaration to the top of the function

@dbradleyfl dbradleyfl force-pushed the fix-hand-controls-for-oculus-touch-webxr branch from 8921a52 to e1ea234 Compare January 15, 2020 18:55
@@ -175,13 +175,13 @@ module.exports.Component = registerComponent('hand-controls', {
var controlConfiguration;
var el = this.el;
var hand = this.data;
var handModelOrientation = hand === 'left' ? Math.PI / 2 : -Math.PI / 2;
Copy link
Member

@dmarcos dmarcos Jan 15, 2020

Choose a reason for hiding this comment

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

We can move this declaration inside the model load handler:

https://github.com/aframevr/aframe/pull/4396/files#diff-02099d98da3e9b5683f5a6ac5092aec4R189

@dbradleyfl dbradleyfl force-pushed the fix-hand-controls-for-oculus-touch-webxr branch from e1ea234 to edfb0aa Compare January 15, 2020 19:02
@dmarcos
Copy link
Member

dmarcos commented Jan 15, 2020

Thanks for the effort. Much appreciated.

@dmarcos dmarcos merged commit 746be94 into aframevr:master Jan 15, 2020
@Sandstedt
Copy link

Seems like these changes has made it into v1.0.4 (or perhaps before), but I still experiences the issue of the hand is rotated.

Tried in Firefox 79 on Windows 10, on Oculus Quest using link.

@dmarcos
Copy link
Member

dmarcos commented Aug 17, 2020

@Sandstedt
Copy link

@dmarcos I'm using the "default" https://aframe.io/releases/1.0.4/aframe.min.js. Not sure what branch that is from.

Also tried your cdn link, but it's the same problem.

@Sandstedt
Copy link

Ok, I tried it in the Quest Browser, and there it work perfect. The wrongly rotated hand is only on Desktop Firefox 79. (In Chrome or Edge 84 I can't even get a-frame to run in VR mode).

But as I understand it, desktop VR is a bit of a mess right now. So maybe call this the day and come back to it in a year :D

@dmarcos
Copy link
Member

dmarcos commented Aug 18, 2020

@dbradleyfl
Copy link
Contributor Author

Is the Firefox browser still using the WebVR api (instead of WebXR)? Originally I saw the same issue with Firefox on Quest, and had an if / else that checked webxr and applied the rotation differently, but I think we decided to remove that. I can't recall why.

@dmarcos
Copy link
Member

dmarcos commented Aug 18, 2020

Yes Firefox desktop is still on WebVR

@Sandstedt
Copy link

@dmarcos Already tried that, and same result.

But maybe this is an edge case? I just use it this way while developing. But if it's the same using Rift S, then its probably a bug worth checking out.

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

Successfully merging this pull request may close these issues.

3 participants