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: Set stencil to false by default. #27900

Merged
merged 1 commit into from
Mar 12, 2024
Merged

Renderer: Set stencil to false by default. #27900

merged 1 commit into from
Mar 12, 2024

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Mar 11, 2024

Related issue: #27799

Description

As suggested in #27799 (comment), the stencil parameter of WebGLRenderer and the new Renderer.stencil property are now false by default. That means applications have to set the value to true now if they rely on stencil related logic.

The motivation behind this change is to only request resources if they are required. Since stencil is only used in special circumstances, the new default value seems more appropriate.

Note: Not enabling stencil for the default drawing buffer does not exclude depth textures or render targets of having stencil types/formats or attachments.

@cabanier I've seen WebXRManager also evaluates the stencil property for its layer related logic. Do you see any issues with this change?

Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
669.7 kB (166.2 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
@cabanier
Copy link
Contributor

@cabanier I've seen WebXRManager also evaluates the stencil property for its layer related logic. Do you see any issues with this change?

No, this change looks fine. No WebXR browser implementations use the stencil buffer so it's ok to disable by default.

@Mugen87 Mugen87 marked this pull request as ready for review March 11, 2024 16:41
@Mugen87 Mugen87 added this to the r163 milestone Mar 11, 2024
@mrdoob mrdoob merged commit aca3b16 into mrdoob:dev Mar 12, 2024
12 checks passed
@mrdoob
Copy link
Owner

mrdoob commented Mar 12, 2024

@RenaudRohlinger
Copy link
Collaborator

RenaudRohlinger commented Mar 12, 2024

Should we also consider eliminating the clear stencil operation? Given that the stencil buffer is now disabled by default, its clear operation appears unnecessary in both WebGLRenderer and WebGPURenderer.

Additionally, should we default stencil = false in both getDepthBuffer and RenderContext as well?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 12, 2024

Should we also consider eliminating the clear stencil operation? Given that the stencil buffer is now disabled by default, its clear operation appears unnecessary in both WebGLRenderer and WebGPURenderer.

That sounds good. We can do this by evaluating the stencil context attribute in WebGLRenderer and the respective stencil property in WebGPURenderer and then perform the clear if required.

Additionally, should we default stencil = false in both getDepthBuffer and RenderContext as well?

Sounds good to me as well. We should only keep in might that UnsignedInt248Type might have more depth precision than UnsignedIntType. If we see issues with depth precision on app level, we might want to use a different configuration for depth textures.

@RenaudRohlinger Would you file a PR with your suggested changes?

@RenaudRohlinger
Copy link
Collaborator

Ok will do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants