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: Use reported XRView.eye for setting stereo layers #29872

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

harryhjsh
Copy link
Contributor

@harryhjsh harryhjsh commented Nov 13, 2024

Description

Inspired by #29742, this PR uses the reported XRView.eye when creating cameras for an XR device's views - as well as finalising #29742 for cameras beyond the first two.

I've removed the explicit cameraL and cameraR variables, as well as the separate cameras array in favour of directly using cameraXR.cameras, and relying entirely on the device's reported XRViewerPose. The knock-on of this is that all the cameras are created on receiving the device's first XRViewerPose, rather than at the instantiation of WebXRManager. (relevant: #23972).

This leads to two additional assumptions:

The combination of these two things mean that the main regression I've been able to envision would be that, for a device which doesn't report its eyes correctly, the right eye would be a duplicate of the left eye (whereas currently we're assuming that the second view is always the right eye).

The upside is that for devices with more than two views (even if those views are all "none"-eyed), pre-rendered stereo content (e.g. webxr_vr_video) will be rendered correctly.

If these assumptions are sound, I think that moving the camera creation after frame.getViewerPose() is reasonable, as it allows the XRManager to rely on the spec for eye differentiation - but I'm happy to scale this back to just ensure that all the child cameras' layers are kept in sync if this seems too sweeping.

@harryhjsh
Copy link
Contributor Author

Three additional questions:

  • I can't work out why it's necessary to enable layers based on the camera's view index (here). Am I missing something, or can this be removed?
  • Is using the camera's userData to store what eye it's relevant to a reasonable method for this, or would something else be more preferable?
  • Some (even cursory) testing on a headset would be appreciated, as I'm working with emulators only at this stage.

Thank you!

Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.37
79.06
339.43
79.08
+68 B
+23 B
WebGPU 477.96
132.56
477.96
132.56
+0 B
+0 B
WebGPU Nodes 477.43
132.44
477.43
132.44
+0 B
+0 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 464.82
112.02
464.88
112.03
+64 B
+16 B
WebGPU 546.89
148.19
546.89
148.19
+0 B
+0 B
WebGPU Nodes 502.77
137.89
502.77
137.89
+0 B
+0 B

@harryhjsh harryhjsh changed the title WebXRManager: Use reported XRView.eye for stereo handling WebXRManager: Use reported XRView.eye for stereo layers Nov 13, 2024
@harryhjsh harryhjsh marked this pull request as ready for review November 13, 2024 15:57
@harryhjsh harryhjsh changed the title WebXRManager: Use reported XRView.eye for stereo layers WebXRManager: Use reported XRView.eye for setting stereo layers Nov 13, 2024
@harryhjsh
Copy link
Contributor Author

Realise I may not have been explicit enough about what the point of this is, so just to sum it up if necessary:

  • This would fix Configure WebXR camera layers #24354 when targeting XR devices with more than two views, e.g. CAVE setups.
  • Also try and ensure that pre-rendered stereo content is drawn correctly for similar devices (as it is, no views beyond the first two are ever rendered when following the example laid out in webxr_vr_video (i.e. explicitly setting layers 1 and 2 on the mesh targeting the left/right eyes respectively)).

If the more general WebXRManager changes aren't desired, I still think it's worth applying the changes from #29742 to cameras beyond the first two, which I'd be happy to adjust this PR for if that's the consensus. The idea was to remove duplication of the separate cameras array vs cameraXR.cameras, as well as make an attempt at rendering correct content for a wider range of XR devices by setting layers based on XRView.eye from the off, but I can see how this might be controversial if people don't agree that the assumptions outlined above are valid.

Happy to discuss if anything else is needed for anyone to want to have a look at this - 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
Development

Successfully merging this pull request may close these issues.

1 participant