-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
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: Added depth sensing support #27154
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
src/renderers/webgl/WebGLProgram.js
Outdated
parameters.occlusion ? '#define USE_OCCLUSION' : '#define USE_OCCLUSION', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both sides are equal; the right-hand side should be ''
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know :-)
That is why it's WIP and still a draft PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CodyJasonBennett fixed!
Super cool! Out of curiosity, have you done a perf/quality comparison between:
I’d think writing the z buffer directly loses the fuzziness but should be faster (can use early z discard, no extra texture fetch per fragment, etc) |
I have not done a perf comparison. My naive way seems to integrate nicely with three, although it's a bit special since it's a renderer feature and not per material or scene. |
I feel like this approach is much more straight forward and easier to maintain, regardless of the performance implications. I think I would try implementing this approach first before modifying all the shaders for all materials. |
src/renderers/webxr/WebXRManager.js
Outdated
|
||
if (( depthData.depthNear != session.renderState.depthNear ) || ( depthData.depthFar != session.renderState.depthFar )) { | ||
|
||
session.updateRenderState( {depthNear: depthData.depthNear, depthFar: ( depthData.depthFar > 10000 ? 10000 : depthData.depthFar ) } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like a WebXR bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spec looks like it allows infinity:
If activeState’s depthFar is greater than session’s maximum far clip plane set activeState’s depthFar to session’s maximum far clip plane.
And
The maximum far clip plane SHOULD be greater than 1000.0 (and MAY be infinite).
So if it's not working then it's a Chromium implementation issue. Idea is to allow depthFar: Infinity
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if it's not working then it's a Chromium implementation issue. Idea is to allow
depthFar: Infinity
, right?
Yes. If I pass infinity, I get an error saying the value is non-finite
546c1af
to
e0b2926
Compare
@@ -1600,7 +1615,7 @@ class WebGLRenderer { | |||
|
|||
const lightsStateVersion = lights.state.version; | |||
|
|||
const parameters = programCache.getParameters( material, lights.state, shadowsArray, scene, object ); | |||
const parameters = programCache.getParameters( material, lights.state, shadowsArray, scene, object, _occlusion ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrdoob is there a better way than passing in a renderer variable?
fwiw it looks like on Unity, at least for custom shaders, they suggest modifying the shaders and calculating the occlusion in the fragment shader:
|
We can play with the shaders in follow up patches. It might even be interesting to vary which depth values are honored, or provide a color to overlay real world objects |
5c10730
to
f0b6a7c
Compare
* Linting, add maxGeometryCount to BatchedMesh, remove undocumented functions * BatchedMesh: Update documentation
* Introduction to fragmentNode * revision
…#27233) * Fix .clear() using RenderTarget * webgpu_rtt: improve a little * cleanup * cleanup * revisions * revision * wip flipY * update example * cleanup
* WebGPURenderer: Depth Pixel & Logarithmic Depth Buffer * Update puppeteer.js
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…were possible (mrdoob#27551) * remove redundant code * use cached descriptor properties for clear() * remove unused import --------- Co-authored-by: aardgoose <angus.sawyer@email.com>
…rdoob#27544) * WebXR: add onLoad callback for XRHandMeshModel * add onLoad for XRControllerModelFactory
* add lengthSq to math nodes * forgot lengthSq doesn't exist in glsl * cleanup ---------
* WebGPURenderer: PassNode + PostProcessing * update sky color * cleanup * a bit more realistic
* WebGPURenderer improve copyTextureToBuffer * remove unecessary comment * correct comment
* camera proj inverse was missing * apply mat4 to projectionMatrixInverse
@cabanier I was able to try this out (finally!) but still had to turn the option on on v60. Looks very jaggy/pixelated even on Quest 3 but for some experiences this might be acceptable. Is there already a newer version than v60 that has it enabled by default? Also, some questions:
|
True. There are better shaders than the simple ones I added. I was hoping that we experiment with them after landing the fundamentals.
It should be enabled by default in 31.1. Is that not the version you have?
Not at the moment
Yes, for some content it might be desired that it's not occluded. (ie hands or the controller) |
Closing in favor of #27586. |
@Mugen87 I'm not sure if this issue should be closed, the topic of "how to allow the shader-based version in a sample" is still very much open. Many effects can't be achieved with the v2 implementation and vice versa, both have their place. |
At least at the moment the project is not willing to support both paths in My hope is it will be easier to support changes like this with |
I mean like this – v2 in core, v1 as sample: Of course @cabanier or @mrdoob can open separate PRs for a sample that uses custom shaders, I just think it may get lost when this PR here is closed. |
I will make an attempt to extend the WebXRManager and write a test that uses a custom shader. |
First pass at adding support for WebXR GPU based depth sensing.
Also updated the XR samples to request depth.
We can tweak shaders and parameters later.
This contribution is funded by Meta