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

Renderer: Stencil Buffer disabled in RenderContext by default #27908

Merged
merged 8 commits into from
Mar 13, 2024

Conversation

RenaudRohlinger
Copy link
Collaborator

@RenaudRohlinger RenaudRohlinger commented Mar 12, 2024

Related: #27900

Description

Following #27900, this PR disable the Stencil Buffer by default in RenderContext.

This contribution is funded by Utsubo

@RenaudRohlinger RenaudRohlinger requested a review from Mugen87 March 12, 2024 16:04
Copy link

github-actions bot commented Mar 12, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
669.7 kB (166.1 kB) 669.7 kB (166.1 kB) +0 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
449.9 kB (108.8 kB) 449.9 kB (108.8 kB) +0 B

@gkjohnson
Copy link
Collaborator

gkjohnson commented Mar 12, 2024

I don't know if I agree that the default of the clear flag should be changed. If I have the stencil buffer enabled why should calling "clear" not clear the stencil buffer, as well?

The flag for enabling the stencil buffer by default is separate from when it get cleared by default when it exists.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 12, 2024

How about not changing the default parameter in the clear() methods but check instead if stencil/stencilBuffer(for render targets) has been set to true or not? If so, the stencil clear is performed.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 12, 2024

webgl_postprocessing_advanced and webgl_postprocessing_masking fail now. Both use the stencil buffer.

This PR should not introduce a breaking change.

@RenaudRohlinger
Copy link
Collaborator Author

RenaudRohlinger commented Mar 12, 2024

Ok, what about this one?

the WebGPURenderer part is pretty straight forward since we have this.stencil exposed to the renderer but for the WebGLRenderer it looks like this:

if ( _currentRenderTarget === null ) {

	stencil = this.autoClearStencil;

} else if ( _currentRenderTarget.stencilBuffer === false ) {

	stencil = false;

}

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 12, 2024

Ok, what about this one?

I think this looks good!

@Mugen87 Mugen87 added this to the r163 milestone Mar 12, 2024
@gkjohnson
Copy link
Collaborator

gkjohnson commented Mar 13, 2024

Just to be clear - are we really worried about performance implications of setting the "clear stencil" bit when there's no stencil buffer created? This seems very much like a micro optimization that adds unnecessary code for something the driver would have to do anyway.

@RenaudRohlinger
Copy link
Collaborator Author

I agree that adjusting the "clear stencil" bit without an existing stencil buffer likely won't impact performance, as it only modifies settings for the gl.clear() command without any substantial processing.
But to me, it just makes more sense to not bother with it if we don’t have a stencil buffer in the first place. Why clear something that’s not there? Considering how different GPUs might react, keeping things simple and logical feels like the better move.

Copy link
Collaborator

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why clear something that’s not there? Considering how different GPUs might react, keeping things simple and logical feels like the better move.

I'll defer to other opinions here but my opinion is that defensively adding code to do things we don't know are needed isn't a great policy. It adds more code which means more to understand (and in this case the purpose or intent isn't obvious) and more points for potential bugs to arise, however simple the logic may seem. I think there are good reasons to not just add things that seem simple without an apparent need.

Regarding driver issues - I think you do have to trust the drivers to do something reasonable until it's shown they don't in some case. Otherwise what's to stop you from adding defensive code everywhere?

examples/jsm/renderers/common/Renderer.js Outdated Show resolved Hide resolved
@RenaudRohlinger RenaudRohlinger changed the title Renderer: Remove Default Stencil Buffer Clear Operation Renderer: Stencil Buffer disabled in RenderContext by default Mar 13, 2024
@RenaudRohlinger
Copy link
Collaborator Author

Alright you convinced me @gkjohnson. I will just continue the previous work and disable by default the stencil buffer in the WebGPU part too on this PR.

@Mugen87 Mugen87 merged commit f763181 into mrdoob:dev Mar 13, 2024
11 checks passed
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.

3 participants