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: Added depth sensing support (v2). #27586

Merged
merged 16 commits into from
Jan 31, 2024
Merged

Conversation

cabanier
Copy link
Contributor

@cabanier cabanier commented Jan 18, 2024

Related: #27154.

Simpler approach to depth sensing proposed by @mrdoob.
We copy the depth into the destination frame buffer instead and have it sorted by fl

This contribution is funded by Meta

Copy link

github-actions bot commented Jan 18, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
673.7 kB (167.3 kB) 675.3 kB (167.8 kB) +1.6 kB

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
454.5 kB (110.2 kB) 456.1 kB (110.8 kB) +1.6 kB

@cabanier cabanier force-pushed the occlusion_v2 branch 2 times, most recently from 4c6a4df to fbe9ab6 Compare January 18, 2024 18:12
@cabanier cabanier marked this pull request as ready for review January 18, 2024 18:18
@hybridherbst
Copy link
Contributor

Cool! What's your perception of it?

@cabanier
Copy link
Contributor Author

Cool! What's your perception of it?

See https://twitter.com/rcabanier/status/1748050448270135425

@hybridherbst
Copy link
Contributor

hybridherbst commented Jan 18, 2024

For some reason I get a 90° around Y rotated camera here in our scenes, and frustum culling is wrong. Does this maybe not work when the camera is parented to something or the camera is moving after the session is started?

Edit: I think the nested render call might be an issue, there's a lot of extra stuff going on – e.g. all onBeforeRender callbacks and so on will be executed again

@cabanier
Copy link
Contributor Author

For some reason I get a 90° around Y rotated camera here in our scenes, and frustum culling is wrong. Does this maybe not work when the camera is parented to something or the camera is moving after the session is started?

Yeah, maybe that all needs to move to the webxr manager. I'll take a look

@hybridherbst
Copy link
Contributor

Thanks!
It feels like performance is quite noticeably better on this version, not sure if you're seeing the same

@cabanier
Copy link
Contributor Author

Thanks! It feels like performance is quite noticeably better on this version, not sure if you're seeing the same

that's possible. We're not sampling from the surrounding pixels in this version.

@cabanier
Copy link
Contributor Author

For some reason I get a 90° around Y rotated camera here in our scenes, and frustum culling is wrong. Does this maybe not work when the camera is parented to something or the camera is moving after the session is started?

Yeah, maybe that all needs to move to the webxr manager. I'll take a look

@hybridherbst I updated the PR. It's even simpler now

@hybridherbst
Copy link
Contributor

Thank you, that works for me now too! Great to be able to compare the two approaches – and a hard decision on what's better suited for three.js 🗡️

  • v1: less pixelated, lots of potential for artistic exploration of the provided scene depth, technically quite involved
  • v2: more pixelated, better performance, not much to adjust, technically very simple

Here's a quick test with custom shaders on v1: https://twitter.com/hybridherbst/status/1748106798173733036

Maybe one possible approach could be:

  • v2 is shipped in three.js core and covers the most typical use case
  • v1 is provided as an example, using shader patching?

@cabanier
Copy link
Contributor Author

@hybridherbst maybe we can ship @mrdoob 's suggestion as an MVP but also leave the door open for experiences to do better. Maybe we can optionally turn off the automatic occlusion and let experiences define custom shaders to do it themselves.

@Mugen87 Mugen87 added this to the r161 milestone Jan 19, 2024
@Mugen87 Mugen87 changed the title Occlusion v2 WebXRManager: Added depth sensing support (v2). Jan 19, 2024
@cabanier
Copy link
Contributor Author

@mrdoob thoughts?

@mrdoob
Copy link
Owner

mrdoob commented Jan 26, 2024

Yeah! This feels so much better!

@mrdoob
Copy link
Owner

mrdoob commented Jan 26, 2024

Does this work on Quest 3 already or do I have to enable any flag?

@Maksims
Copy link

Maksims commented Jan 26, 2024

Does this work on Quest 3 already or do I have to enable any flag?

It works in latest Quest 3 Browser (OS should be updated too I guess), without a need to enable any flags.

@cabanier
Copy link
Contributor Author

Does this work on Quest 3 already or do I have to enable any flag?

It works in latest Quest 3 Browser (OS should be updated too I guess), without a need to enable any flags.

Correct. It's enabled by default. The first time you use it in browser, you should get an OS permission prompt.

@cabanier
Copy link
Contributor Author

Yeah! This feels so much better!

@mrdoob If you're ok with the changes, we can submit and then fine-tune it so people can still use a custom shader if they want
(ie to get effects like this: https://twitter.com/hybridherbst/status/1748106798173733036)

@mrdoob
Copy link
Owner

mrdoob commented Jan 31, 2024

Okay, I'm going to merge this for now but I'm getting these warnings when inside immersive + depth sensing...

Screenshot 2024-01-31 at 15 33 59 Screenshot 2024-01-31 at 15 39 53

Any ideas?

@mrdoob mrdoob merged commit 6e3137a into mrdoob:dev Jan 31, 2024
12 checks passed
@cabanier
Copy link
Contributor Author

cabanier commented Feb 1, 2024

Okay, I'm going to merge this for now but I'm getting these warnings when inside immersive + depth sensing...
Any ideas?

I've seen these as well in regular WebXR sessions. I believe it's because three is using a swapchain texture that is out of band.

@cabanier cabanier deleted the occlusion_v2 branch February 1, 2024 17:00
@mrdoob
Copy link
Owner

mrdoob commented Feb 2, 2024

I don't understand what that means...

Are we doing something wrong? Do we have to change something? And if so, what do we need to change? Is it an issue in the Oculus Browser instead? And if so, are you planning to address it?

@michaelthatsit

This comment was marked as outdated.

@michaelthatsit
Copy link

Ignore my last comment, we were clearing the renderer which is what caused the issue.

have use cases where we'd like to occlude the hands but selectively occlude planes. Is there a way to adjust the occlusion?

@cabanier
Copy link
Contributor Author

cabanier commented Feb 2, 2024

have use cases where we'd like to occlude the hands but selectively occlude planes. Is there a way to adjust the occlusion?

If you want to occlude on a per object basis, you need to do the occlusion in the shader. Maybe we should give you access to the depth texture and not populate the rendertarget with the depth pixels

@michaelthatsit
Copy link

that would be great, honestly being able to pass a distance parameter would work too. I'd love to integrate this and abandon our render order hack, but not at the cost of losing a lot of the MR features we've added.

@michaelthatsit
Copy link

filed a feature request: #27664

@mrdoob
Copy link
Owner

mrdoob commented Feb 2, 2024

@cabanier can you please answer my question?

@cabanier
Copy link
Contributor Author

cabanier commented Feb 2, 2024

I'm unsure what the glFrameBufferTexture2D error is. I don't believe that one comes from our side. I'll look into it tomorrow.

@mrdoob
Copy link
Owner

mrdoob commented Feb 2, 2024

Thank you!

@cabanier
Copy link
Contributor Author

cabanier commented Feb 2, 2024

This is a bug on the browser side. I will fix it. No changes in three are needed.

@cabanier
Copy link
Contributor Author

cabanier commented Feb 6, 2024

I found and fixed the problem in the browser. It will be part of the next browser release.

@cabanier
Copy link
Contributor Author

@mrdoob I fixed this problem using the WebXR samples. However, it is still happening in three.js.
It doesn't seem to have bad side effects but I'll investigate it a bit more. (For some reason, an internal call is flagging that the previous GL call failed. This is happening every third frame which make it look like a swapchain issue)

@cabanier
Copy link
Contributor Author

@mrdoob I fixed this problem using the WebXR samples. However, it is still happening in three.js. It doesn't seem to have bad side effects but I'll investigate it a bit more.

I dug a bit deeper. It seems that drawing the depth texture is generating an invalid operation when it executes drawElements to paint the depth into the framebuffer. Do you know why this could be?

@odysseyjason
Copy link

odysseyjason commented Apr 29, 2024

Meta Quest 3 on the PTC received Meta system software v65 on April 26th, following the update none of the Depth Sensing API samples are performing local occlusion.

Not sure if this is a Meta browser issue, threejs issue, or if Meta is no longer providing the browser with access to the depth sensor data on the Meta Quest 3.

Neither of the following are providing any local deptha api based occlusion following v65 update, and they previously did at v64.

threejs webxr_xr_dragging
playcanvas xr ar-camera depth

(this has me very sad as I find occlusion to be the COOLEST webxr feature to come around!)

@cabanier
Copy link
Contributor Author

Meta Quest 3 on the PTC received Meta system software v65 on April 26th, following the update none of the Depth Sensing API samples are performing local occlusion.

I had noticed that as well but it seems to be fixed on latest three.js when I pulled it down last week.
I suspect that @Mugen87 's changes around gl_FragDepthEXT broke this temporarily

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 29, 2024

AFAICT, the release versions should not be affected by my changes. There was a temporary issue in r163dev but it has been fixed via #27855.

@cabanier
Copy link
Contributor Author

AFAICT, the release versions should not be affected by my changes. There was a temporary issue in r163dev but it has been fixed via #27855.

I didn't look very closely what caused or fixed the issue. depth sensing is broken on the public samples but working on dev

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.

7 participants