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

WebGLRenderer: Make sure stencil works with transmission. #27799

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Feb 22, 2024

Related #26959.

Description

This PR is an alternative implementation to #26959.

When the default drawing buffer has a stencil buffer, this PR makes sure the transmission render target has one as well. This is achieved by applying the stencil context parameter to the internal render target.

This PR is more simple but less flexible than #26959 since it does not perform dynamic checks on render targets. The policy is if stencil is required in the transmission pass, is has to be turned on for the default drawing buffer.

@Mugen87 Mugen87 added this to the r162 milestone Feb 22, 2024
Copy link

github-actions bot commented Feb 22, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
669.5 kB (166.1 kB) 669.5 kB (166.1 kB) +10 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
449.7 kB (108.8 kB) 449.7 kB (108.8 kB) +10 B

@mrdoob
Copy link
Owner

mrdoob commented Feb 22, 2024

@Oletus Solves the issue for you?

@mrdoob mrdoob modified the milestones: r162, r163 Feb 29, 2024
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 4, 2024

Syncing stencil of the transmission render target with the default framebuffer seems a step in the right direction. We can discuss in #26959 if a more dynamic solution is still required.

@Mugen87 Mugen87 merged commit 7e74e4c into mrdoob:dev Mar 4, 2024
12 checks passed
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 8, 2024

@mrdoob It has been reported in #27846 (comment) that enabling stencil for transmission has a negative impact on the performance.

I still think that syncing the stencil buffer of the transmission render target with the setup of the drawing buffer is right. But that leads me to the question: Why is the stencil context attribute true by default? Couldn't we set it to false for performance reasons? Only when stencil is required by an application, it is manually set to true.

Since stencil is a more specific feature, I feel it hasn't to be enabled by default.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 8, 2024

@lo-th I have reverted the PR via #27885 until the above question is clarified.

@lo-th
Copy link

lo-th commented Mar 8, 2024

here the version with stencil on: https://lo-th.github.io/phy/index2.html#random

Stencil off: https://lo-th.github.io/phy/#random

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 8, 2024

I get 25 - 55 with stencil and 26-55 without on a mac Mini with M2 Pro. But I admit a performance degradation is visible.

@lo-th
Copy link

lo-th commented Mar 8, 2024

26 !? wow mac :)
i have strong 144 - 60 normally and 25 - 25 with stencil on

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 8, 2024

This kind of FPS difference should not be the case. An additional stencil buffer can't be the sole reason for this kind of performance degradation. This is almost worth reporting at the browser vendor. What kind of browser and operating system are you using?

@lo-th
Copy link

lo-th commented Mar 9, 2024

windows 11 / chrome / rtx 4090 / threadripper 32 core :)
is same with firefox ( firefox have lower perf on physx worker )

@mrdoob
Copy link
Owner

mrdoob commented Mar 10, 2024

But that leads me to the question: Why is the stencil context attribute true by default? Couldn't we set it to false for performance reasons? Only when stencil is required by an application, it is manually set to true.

I don't think there was a reason for it.
Setting it to false by default sounds good to me 👌

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 11, 2024

@lo-th I've researched a bit more and it seems the following document explains the performance drop on Windows:

https://groups.google.com/g/webgl-dev-list/c/iwIYWPPJwoQ?pli=1

tl;dr: D3D does not support resolving depth+stencil on the GPU. Hence, WebGL must do this on the CPU (emulation) which is slow. Since the transmission render target is multisampled, it goes through this process on Windows when stencilBuffer is set to true which explains the poor performance.

@Oletus
Copy link
Contributor

Oletus commented Mar 11, 2024

I think there should be some way of disabling the stencil bit when resolving using blitFramebuffer, even if the multisample rendertarget uses a stencil buffer. There are applications which use stencil while rendering, but only care about color buffer after resolving. Maybe a resolveStencil bit in WebGLRenderTarget that would be false by default?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 12, 2024

TBH, I would prefer a solution that doesn't require a new flag. At least not a flag that has to be set by users.

There are applications which use stencil while rendering, but only care about color buffer after resolving

Maybe we can always do this for the transmission render target and nobody will notice 🙈 ?

@Oletus
Copy link
Contributor

Oletus commented Mar 13, 2024

Yep, avoiding a flag is good if we can get away with it! I don't think there will be any problems with resolving just the color buffer in case of the transmission render target. But it's a bit harder to say in general what's the most appropriate for user-created multisample render targets.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 13, 2024

I have implemented the discussed approach in Mugen87@707d454 for testing:

  1. Transmission with stencil (no resolve): https://rawcdn.githack.com/Mugen87/three.js/6534c98c7b603a1b19408763b3c372f17a0dceae/examples/webgl_loader_gltf_transmission.html?stencil=true
  2. Transmission with stencil (resolve):
    https://rawcdn.githack.com/mrdoob/three.js/d3c9871672acc7b38120261513eeccc0dc862640/examples/webgl_loader_gltf_transmission.html
  3. Transmission without stencil: https://rawcdn.githack.com/Mugen87/three.js/6534c98c7b603a1b19408763b3c372f17a0dceae/examples/webgl_loader_gltf_transmission.html?stencil=false

@lo-th Do you still see a performance degradation in the first link on Windows? Ideally test 1. and 3. should have equal performance, test 2. should stutter (it's the version before the revert #27885).

@lo-th
Copy link

lo-th commented Mar 14, 2024

i see no difference on both ?
in my demo i also use mirror on floor

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 14, 2024

Maybe you can apply the patch to your demo and check the performance? Update build files are available here:

Mugen87@6534c98

@lo-th
Copy link

lo-th commented Mar 14, 2024

work fine with this build and stencil=true

@Oletus
Copy link
Contributor

Oletus commented Apr 8, 2024

@Mugen87 Hey, it looks like stencil buffer bit is still set when resolving MSAA render targets, unless it's a transmission render target? Any chance we could universally disable stencil multisample resolve, or implement a flag controlling that?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 8, 2024

Hey, it looks like stencil buffer bit is still set when resolving MSAA render targets, unless it's a transmission render target?

Correct. The following line controls this:

// resolving stencil is slow with a D3D backend. disable it for all transmission render targets (see #27799)
if ( renderTarget.stencilBuffer && renderTargetProperties.__isTransmissionRenderTarget !== true ) mask |= _gl.STENCIL_BUFFER_BIT;

I'm open for a resolveStencilBuffer flag in the RenderTarget class. The internal transmission render target could set it to false so we would not need __isTransmissionRenderTarget anymore.

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

4 participants