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

Improve WebXR layers feature testing #30112

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Conversation

toji
Copy link
Contributor

@toji toji commented Dec 12, 2024

Description

Currently Three.js tests for WebXR layers support by checking to see if the layers attribute is present in the session.renderState object. This isn't ideal, however.

It's possible that the layers attribute may be present but not usable if the session did not request the 'layers' feature, if they did request it but the browser denied it, or if the browser supports layers but the backend being used does not.

A more reliable way to check for layers support is to look at the session.enabledFeatures list and check to see if it contains the 'layers' string. If so the the session is confirmed to have layers support and the rest of the code can be used as-is. This PR makes that change to both the layers sample and the WebXRManager.

Copy link

github-actions bot commented Dec 12, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.21
79.01
339.24
79.02
+35 B
+15 B
WebGPU 486.17
134.91
486.17
134.91
+0 B
+0 B
WebGPU Nodes 485.63
134.81
485.63
134.81
+0 B
+0 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 465.1
112.06
465.13
112.08
+35 B
+15 B
WebGPU 555.31
150.34
555.31
150.34
+0 B
+0 B
WebGPU Nodes 511.22
140.1
511.22
140.1
+0 B
+0 B

@mrdoob mrdoob added this to the r172 milestone Dec 13, 2024
@mrdoob mrdoob merged commit b6f7ba2 into mrdoob:dev Dec 13, 2024
12 checks passed
@toji toji deleted the xr-layers-check branch December 13, 2024 18:15
@cabanier
Copy link
Contributor

cabanier commented Jan 4, 2025

@toji @Mugen87 I didn't see this change until now but this is not the correct way to test for layers support.
By design, you don't have to request layers in order to have support for a single projection layer.

I think this commit should be reverted.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 5, 2025

I'm afraid I do not understand the topic well enough to make an informed decision so I defer to @toji .

Related: It seems this PR causes issues with at least on type of headset: #30263

Edit: It is now confirmed that this PR breaks WebXR on PICO 4, see #30263 (comment).

@cabanier
Copy link
Contributor

cabanier commented Jan 6, 2025

Edit: It is now confirmed that this PR breaks WebXR on PICO 4, see #30263 (comment).

This is likely because the Pico browser lags Chromium by several releases so it doesn't support enabledFeatures which leads the code down a path that triggers a bug in the Pico brwoser.
The change in WebXRManager should be reverted since it's not the correct way to check for projectionLayer support.

@toji
Copy link
Contributor Author

toji commented Jan 7, 2025

Apologies for any breakage which this may have caused!

Rik is correct in that you shouldn't need the layers feature for just a single projectionLayer. I had forgotten this, so the patch will exclude some cases where the single layer can be used.

However, the previous code wasn't ideal either. Specifically, I'm working on WebXR/WebGPU integration which will rely on using the layers render state. We do not (yet) have support for WebGL projection layers, though. This unfortunately means that enabling WebXR/WebGPU support breaks Three.js XR content in Chrome because it expects to use layers unconditionally.

You can argue that this is a Chrome implementation issue and that perhaps WebXR/WebGPU support should be gated on a minimal WebGL Layers implementation, but unfortunately at this point I can't guarantee that I'll be given the time to implement that.

In my opinion the check should at least shift from looking for the existence of layers on the render state to looking for the existence of the createProjectionLayer method on XRWebGLBinding, because if that's present then it should definitely work correctly.

(I think the check is probably OK for webxr_vr_layers.html, since the whole point of that demo is to show usage of multiple layers?)

A question I have for @cabanier: Is there a performance or capabilities reason (on Meta devices or in general) why developers might prefer to use a single projection layer instead of an XRWebGLLayer if multiple layers aren't available? It's a more unified interface with multi-layer, but otherwise I thought they were pretty analogous?

Finally, I'm not sure why Pico would be hitting a bug with enabledFeatures, since the code does check to ensure it's not undefined prior to doing the feature check. I originally wrote the code as session.enabledFeatures?.includes(...) which may have worked better, but some part of Three's build toolchain didn't like that syntax. Regardless, though, if we refactored the code to check for the binding function then this should be a non-issue.

@cabanier
Copy link
Contributor

cabanier commented Jan 7, 2025

(I think the check is probably OK for webxr_vr_layers.html, since the whole point of that demo is to show usage of multiple layers?)

I agree. That code can stay.

A question I have for @cabanier: Is there a performance or capabilities reason (on Meta devices or in general) why developers might prefer to use a single projection layer instead of an XRWebGLLayer if multiple layers aren't available? It's a more unified interface with multi-layer, but otherwise I thought they were pretty analogous?

projection layers give you more control over multisampling. They also support texture arrays which are required for multiview.

Finally, I'm not sure why Pico would be hitting a bug with enabledFeatures, since the code does check to ensure it's not undefined prior to doing the feature check. I originally wrote the code as session.enabledFeatures?.includes(...) which may have worked better.

Yes, this must be a bug in the pico browser. It would be nice if someone could debug where this is going wrong.

@cabanier
Copy link
Contributor

cabanier commented Jan 8, 2025

I got hold of a pico headset and was able to reproduce the problem.
For some reason, their render process crashes if you use a XRWebGLLayer with three.js. This is a bug in their side but I'm unsure how I can report this.

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.

4 participants