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

remove extra renderpass when using depth sensing #27743

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

Conversation

cabanier
Copy link
Contributor

@cabanier cabanier commented Feb 14, 2024

The current implementation of depth sensing introduces an extra renderpass.
I was tracing some WebXR experiences with depth sensing and noticed that if there's also a shadowmap, the WebXR texture ends up being flushed. In addition to the extra overhead, this also removes foveation (because it is no longer applied after a flush)

This change makes the painting of the depth values part of the regular draw pass.

Output of the render pass profiler with old code:

Surface 0    | 3360x1760 | color 32bit, depth 24bit, stencil 64bit, MSAA 4, Mode: 1 (HwBinning) | 72  288x320 bins ( 56  rendered) |  4.93 ms | 174 stages : Binning : 0.268ms Render : 0.807ms StoreColor : 0.442ms Blit : 0.005ms Preempt : 2.14ms StoreDepthStencil : 0.672ms
Surface 1    | 4096x4096 | color 32bit, depth 24bit, stencil 0 bit, MSAA 1, Mode: 0 (Direct)    | 1   4096x4096 bins ( 1   rendered) |  1.39 ms | 4   stages : Render : 0.798ms Blit : 0.006ms Preempt : 0.581ms
Surface 2    | 3360x1760 | color 32bit, depth 24bit, stencil 64bit, MSAA 4, Mode: 1 (HwBinning) | 63  384x256 bins ( 63  rendered) |  6.11 ms | 189 stages : Binning : 0.074ms LoadColor : 0.102ms Render : 1.723ms StoreColor : 0.099ms Preempt : 3.385ms LoadDepthStencil : 0.112ms
Surface 0 = clear + drawing of the depth pixels
Surface 1 = shadowmap (4kx4k?)
Surface 2 = drawing of the regular scene

Notice saving and restoring of the depth stencil buffer which adds overhead

Output of the render pass profiler with new code:

Surface 0    | 3360x1760 | color 32bit, depth 24bit, stencil 64bit, MSAA 4, Mode: 0 (Direct)    | 1   3360x1760 bins ( 1   rendered) |  0.00 ms | 1   stages : Render : 0.003ms
Surface 1    | 4096x4096 | color 32bit, depth 24bit, stencil 0 bit, MSAA 1, Mode: 0 (Direct)    | 1   4096x4096 bins ( 1   rendered) |  0.85 ms | 2   stages : Render : 0.844ms Blit : 0.005ms
Surface 2    | 3360x1760 | color 32bit, depth 24bit, stencil 64bit, MSAA 4, Mode: 1 (HwBinning) | 72  288x320 bins ( 46  rendered) |  8.49 ms | 95  stages : Binning : 0.101ms Render : 2.033ms StoreColor : 0.403ms Blit : 0.005ms Preempt : 5.594ms
Surface 0 = clear
Surface 1 = shadowmap
Surface 2 = drawing of the depth pixels + drawing of the regular scene

This contribution is funded by Meta

@cabanier cabanier changed the title reduce extra renderpass with depth sensing remove extra renderpass when using depth sensing Feb 14, 2024
Copy link

github-actions bot commented Feb 14, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
676.3 kB (168.2 kB) 676.4 kB (168.2 kB) +113 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
457.1 kB (111 kB) 457.3 kB (111 kB) +113 B

@cabanier
Copy link
Contributor Author

@mrdoob thoughts? This will reduce overhead on mobile gpu's.

I'm still trying to find out where that invalidOperation error is coming from. :-\

@cabanier cabanier marked this pull request as ready for review February 14, 2024 01:51
@mrdoob
Copy link
Owner

mrdoob commented Feb 15, 2024

I'm thinking of refactoring the current implementation.
I'll work on it tomorrow.

@cabanier
Copy link
Contributor Author

I'm thinking of refactoring the current implementation. I'll work on it tomorrow.

Sounds good! That gl warning is throwing me for a loop so maybe it will go away with the refactor.

@cabanier
Copy link
Contributor Author

If you want to debug renderpasses, you could use ovrgpuprofiler
If you see depth being stored/loaded, there is a problem.

@mrdoob
Copy link
Owner

mrdoob commented Feb 16, 2024

Hmm... Does the Quest Pro support depth sensing?

@cabanier
Copy link
Contributor Author

Hmm... Does the Quest Pro support depth sensing?

It does not unfortunately.

@mrdoob
Copy link
Owner

mrdoob commented Feb 16, 2024

Ah! Won't be able to work on this until Monday then...

@cabanier
Copy link
Contributor Author

I'm thinking of refactoring the current implementation. I'll work on it tomorrow.

Did you end up refactoring it?

@Mugen87 Mugen87 modified the milestone: r164 Apr 17, 2024
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.

None yet

3 participants