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

WebXRManager: Update camera's local matrix and transform properties. #19085

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Apr 8, 2020

Fixed #18448.
Fixed #20920.

This would potentially overwrite user-defined values but since the world matrix is also set, I think this approach is more consistent and avoids side effects like in the above issue.

@marcofugaro
Copy link
Contributor

@mrdoob can we get also this small PR in r116? 🙏🙏🙏

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 27, 2020

@marcofugaro
Copy link
Contributor

Another user found this confusing on the discord #help channel some days ago

image

@gkjohnson
Copy link
Collaborator

gkjohnson commented Jan 4, 2021

Hiding content for another issue

I just started on some WebXR development and I think the current behavior is very confusing, though I understand why it was done that way. Either way it would be nice to have some public way to get the parameters of the VR camera including the position, rotation, and union frustum. If the matrix and local properties are going to be updated perhaps the frustum, fov, etc can be, too?

Generally parenting something like UI in VR is bad practice so you need the transform of the camera to float UI elements into place with the frustum being useful for placement. My use case was for implementing a VR example in the 3dTilesRendererJS project which requires the transform and frustum of the camera to compute custom frustum culling and error calculations to determine tile visibility. My workaround right now is to call renderer.xr.getCamera( camera ) to get the VR Camera before rendering but as far as I can tell that's not intended to be public and it's redundant because the renderer calls it, too:

let currentCamera = camera;
if ( renderer.xr.isPresenting ) {

    // this camera has the correct transform and frustum information
    currentCamera = renderer.xr.getCamera( camera );

}

Perhaps updating the VR camera should be an explicit action? For the use cases I listed above you want to get the new VR, perform some kind of operation (culling, UI updatesm, etc), then render. An autoUpdate field could be added to WebXRManager so the camera updates can be disabled if the user wants to do it themselves:

renderer.xr.autoUpdate = false;
renderer.xr.updateVRCamera( camera );

const vrCamera = renderer.xr.getVRCameraInstance();

// update stuff...

renderer.render( scene, camera );

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 4, 2021

My workaround right now is to call renderer.xr.getCamera( camera ) to get the VR Camera before rendering but as far as I can tell that's not intended to be public

That is correct. It's an internal method.

TBH, I don't think your post belongs to this PR. If you want to propose a new API in context of WebXR, please make a separate issue for this.

The PR fixes an obvious bug in WebXRManager which makes methods like Object3D.getWorldPosition() unusable when presenting. In general, all logic which forces a recomputation of the camera's world matrix will fail. To me, this is a serious issue. I clearly vote to merge this PR especially since the overhead of an additional copy() and decompose() call is negligible.

@marcofugaro
Copy link
Contributor

Yup, also it is very confusing accessing camera.position or camera.quaternion while presenting and seeing that your code doesn't work.

My use case was shooting a ball from the camera in AR.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Jan 4, 2021

Yup, also it is very confusing accessing camera.position or camera.quaternion while presenting and seeing that your code doesn't work.

Oh I definitely agree and I think this is an improvement. I didn't mean to detract from this PR just that there are other related use cases still not covered that could be tackled, as well, but I understand they expand the scope.

@mrdoob mrdoob added this to the r125 milestone Jan 25, 2021
@mrdoob mrdoob merged commit ff5573c into mrdoob:dev Jan 25, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jan 25, 2021

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Jan 25, 2021

One side effect of this change is that when we the user leaves the XR experience, the camera will be the last place where the user head was at.

Should the WebXRManager store the camera position when starting the session and restore it when the session ends? (Which is basically the previous behaviour but without being hacky).

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 25, 2021

when we the user leaves the XR experience, the camera will be the last place where the user head was at.

TBH, I always felt this is the expected behavior. But I guess the "ideal" camera behavior depends on personal preferences.

@mrdoob
Copy link
Owner

mrdoob commented Jan 25, 2021

Okay, we'll see what people say.

@marcofugaro
Copy link
Contributor

My use-case was exactly the one of model-viewer, I had a simple 3d scene with OrbitControls and an AR button.
When the session would end the camera would go back to the previous state, just before the click of the AR button.

@mrdoob
Copy link
Owner

mrdoob commented Jan 25, 2021

When the session would end the camera would go back to the previous state, just before the click of the AR button.

Isn't that what model-viewer does?

@marcofugaro
Copy link
Contributor

Isn't that what model-viewer does?

Yeah but my AR scene was dynamic 😉
Couldn't use model-viewer.

@mrdoob
Copy link
Owner

mrdoob commented Jan 25, 2021

Ah, right... Okay.

@DePasqualeOrg
Copy link

I'm dealing with this issue as well in a VR app using Three.js. In my view, it would make sense if the camera remained in the headset's last position and rotation when the user exits the XR experience.

@mrdoob
Copy link
Owner

mrdoob commented Jan 27, 2021

Try today's release. I think it should behave like that.

@DePasqualeOrg
Copy link

Try today's release. I think it should behave like that.

Works great, thanks!

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