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

Fix multiview #22

Closed
wants to merge 6 commits into from
Closed

Fix multiview #22

wants to merge 6 commits into from

Conversation

mrxz
Copy link

@mrxz mrxz commented Jan 31, 2025

Related issue: #21

Description
The multiview changes appear to be in a broken state (did it ever work in an A-Frame release?), causing two distinct problems:

  1. Setting multiviewStereo: true had no effect as the flag was never passed along to the WebXRManager
  2. When enabling the stencilBuffer with multiview stereo disabled, it would no longer setup the depth/stencil attachment.

This PR fixes both issues. We should probably be more careful when rebasing these changes onto new Three.js releases, as I suspect that is how the second issue slipped in. A closer look might be warranted to ensure the code hasn't diverged in other places as well.

Copy link

github-actions bot commented Jan 31, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 342.52
79.81
342.49
79.81
-33 B
-4 B
WebGPU 517.06
143.53
517.06
143.53
+0 B
+0 B
WebGPU Nodes 516.52
143.42
516.52
143.42
+0 B
+0 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 471.52
113.78
471.48
113.77
-33 B
-4 B
WebGPU 589.05
159.74
589.05
159.74
+0 B
+0 B
WebGPU Nodes 544.43
149.3
544.43
149.3
+0 B
+0 B

@dmarcos
Copy link
Member

dmarcos commented Feb 1, 2025

Yeah. multiview worked at some point. The code is a bit tricky to maintain. Question if it's worth keeping

enzofrancescaHM and others added 2 commits February 1, 2025 12:33
* Update WebXRManager.js

* Update WebGLRenderer.js

* Update WebGLRenderer.js

deleted useless blank lines

* Update WebXRManager.js

- Deleted useless code and blank lines
- refectored _getRenderTarget to getRenderTarget

* Update WebGLRenderer.js

Reflect the refactoring of getRenderTarget function

Improve compatibility of Post-Processing with VR (mrdoob#23)

* Update EffectComposer.js for better compatibility with VR

* Update Pass.js for better compatibility with VR
@dmarcos
Copy link
Member

dmarcos commented Feb 1, 2025

needs rebase

@mrxz
Copy link
Author

mrxz commented Feb 2, 2025

@dmarcos rebased

Yeah. multiview worked at some point. The code is a bit tricky to maintain. Question if it's worth keeping

There is value in the feature, but given its many caveats it almost requires you to develop with it on from the start. But I expect most users only stumbling upon this feature later in development, trying it, noticing a regression in their app, and turning it off again.

@dmarcos
Copy link
Member

dmarcos commented Feb 2, 2025

@dmarcos rebased

Yeah. multiview worked at some point. The code is a bit tricky to maintain. Question if it's worth keeping

There is value in the feature, but given its many caveats it almost requires you to develop with it on from the start. But I expect most users only stumbling upon this feature later in development, trying it, noticing a regression in their app, and turning it off again.

Yeah. If people use it happy to keep but hard to know. If Meta helped promote it and A-Frame with it could be also worth it. Not sure yet what to do.

@vincentfretin
Copy link

Your call, merge this PR or revert the two commits. Otherwise scenes are completely broken with stencil:true since r173 for me.

@dmarcos dmarcos force-pushed the dev branch 2 times, most recently from c965ee5 to aacca17 Compare February 2, 2025 19:47
@dmarcos
Copy link
Member

dmarcos commented Feb 2, 2025

Thanks. I folded this into the multiview commit to keep things organized. The fix should be out in super-three r173.2

@dmarcos dmarcos closed this Feb 2, 2025
@vincentfretin
Copy link

Thanks. Next time you can also squash 1aba49d into the multiview commit, that's a fix for the introduced multiview changes actually.

@dmarcos
Copy link
Member

dmarcos commented Feb 3, 2025

Thanks. Next time you can also squash 1aba49d into the multiview commit, that's a fix for the introduced multiview changes actually.

done

@vincentfretin
Copy link

@dmarcos I'm not sure what you did, aframe master with 0.173.3
https://cdn.jsdelivr.net/gh/aframevr/aframe@32ac53b83ac21eac82c265bbc3fa581cfb9247f4/dist/aframe-master.min.js
is giving me a setPoseTarget not defined error on renderer.xr on the aframe-inspector repo.
The previous aframe master with 0.173.2 is working
https://cdn.jsdelivr.net/gh/aframevr/aframe@ebd1e61aac0b62bbf5453736b0e8e478aaf67a4f/dist/aframe-master.min.js

image
image

@vincentfretin
Copy link

The build folder in super-three branch super-r173-2 and super-r173-3 are the same, so that's okay here.
Something went wrong with the a-frobot, it got the original three release instead of the super-three release it seems, none of the patches are in the latest aframe build.

@dmarcos
Copy link
Member

dmarcos commented Feb 4, 2025

The build folder in super-three branch super-r173-2 and super-r173-3 are the same, so that's okay here. Something went wrong with the a-frobot, it got the original three release instead of the super-three release it seems, none of the patches are in the latest aframe build.

Something went wrong when publishing on npm. I might have published before generating the new builds by mistake. Sorry for that. I published r173.4 and A-Frame is also updated. Let me know if it works.

@vincentfretin
Copy link

Human mistake, bot wasn't implicated ;) All good now.

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.

6 participants