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: Avoid invalid projection matrices when using depth sensing #29120

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

mrxz
Copy link
Contributor

@mrxz mrxz commented Aug 12, 2024

Related issue: #29098

Description

The depthFar and depthNear values coming from the depth-sensing module could result in invalid projection matrices (NaN values). These errors would propagate (left, right, XR and user camera) resulting in a broken state after exiting the XR session.

This PR includes three changes:

  • Only recompute a union projection matrix when far plane is finite. In case of an infinite far plane, reuse the projection matrix of the left view. Offsetting the camera is sufficient to fully include both view volumes, assuming the projections are symmetric (see https://computergraphics.stackexchange.com/a/1737)
  • Don't update the far and near properties of the user camera, instead always treat these as input to set the far/near of the renderState. When depth-sensing is active, the relevant depthNear/depthFar values are used for rendering, but not written back to the user-camera.
  • Remove calls to updateProjectionMatrix added in 6e3137. These calls aren't needed, as the projection matrices for the left and right view come directly from the WebXR API and are computed in setProjectionFromUnion for cameraXR. In fact, as the comment above it indicates, the new far/near values only take effect next frame, so immediately computing them is incorrect for that frame.

This contribution is funded by Fern Solutions

Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
685.1 kB (169.6 kB) 685.2 kB (169.6 kB) +43 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
462 kB (111.4 kB) 462 kB (111.4 kB) +43 B
@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 13, 2024

@cabanier Looking good?

@mrdoob mrdoob added this to the r168 milestone Aug 14, 2024
@cabanier
Copy link
Contributor

@cabanier Looking good?

Does this update the cameras for the left and right eye? It seems like only the main camera is updated and I seem to remember that one is only used to do culling.

@mrxz
Copy link
Contributor Author

mrxz commented Aug 21, 2024

Does this update the cameras for the left and right eye?

Yes, their near/far are updated here (like before) and their projection matrices are taken from the XRView each frame. There should be no reason to manually (re)construct the projection matrices.

@cabanier
Copy link
Contributor

Does this update the cameras for the left and right eye?

Yes, their near/far are updated here (like before) and their projection matrices are taken from the XRView each frame. There should be no reason to manually (re)construct the projection matrices.

Great! lgtm!

@Mugen87 Mugen87 merged commit ad1fe86 into mrdoob:dev Aug 21, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants