-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
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
WebGPURenderer: Support MSAA with Postprocessing #28784
WebGPURenderer: Support MSAA with Postprocessing #28784
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
src/nodes/display/PassNode.js
Outdated
for ( let i = 0; i < this.renderTarget.textures.length; i ++ ) { | ||
|
||
const type = this.renderTarget.textures[ i ].type; | ||
this.renderTarget.textures[ i ].isMultisampleRenderTargetTexture = this.renderTarget.samples > 1 && type !== HalfFloatType && type !== FloatType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of type !== HalfFloatType && type !== FloatType
?
Does that mean multisampled (half) float textures are not allowed?
I cleaned up and refactored the samples and antialiasing logic. Previously, it was under Since WebGPU only supports sample counts of 1 and 4, I refactored the Here is most of the refactor: Following this refactor, I discovered that in the case of multisampled textures in WebGPU, we were always actually using two textures: one extra that resolves the multisampled one. Based on the specs, it seems that WebGPU supports direct rendering of multisampled 2D and depth 2D textures, as it seems that the Therefore, I added multisample texture support to the WebGPUBackend and updated the WGSLNodeBuilder to support both types: Small notes:
Aside from this, while testing most of the examples, I noticed that |
It seems the check is still present though. The logic that configures
|
I don't think it makes sense to do that in the near future since you need the |
Sidenote: Post processing in HDR requires half float so I recommend to split the change and add support for |
Thanks for the additional information!
Maybe @sunag has an idea how to hide this bit. Ideally, we evaluate whether a depth texture is multisampled or not inside the renderer and not in components using the renderer. Apart from that, the PR looks already good! |
this.renderTarget.samples = this.options.samples === undefined ? renderer.samples : this.options.samples; | ||
|
||
// Disable MSAA for WebGL backend for now | ||
if ( renderer.backend.isWebGLBackend === true ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this exception still required? My understand is you have added this bit since rendering multisampled to a color renderable texture does not yet work in the WebGL backend. But can at least the "normal" MSAA be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately yes it's still needed. I noticed that using PassNode with samples > 0
doesn't seem to render anything in WebGL. I believe this might be due to the need for an additional frameBuffer somewhere in the render pipeline, which I couldn't easily resolve. That's why I added this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add this path in WebGLBackend.beginRender()
for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't recommend so because basic THREE.RenderTarget
setups works just fine with samples > 0
. For example webgpu_multisampled_renderbuffers.html
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this PR then! It's possible do it in another PR, currently already a step forward in that sense. So the future goal is for us to move this and isMultisampleRenderTargetTexture
into the renderer as @Mugen87 said, when this is ready examples like webgpu_backdrop_water
should work too.
Description
The WebGPU did not support MSAA with a post-processing pipeline. This PR fixes the issue and adds a way to specify options, such as
samples
, toPassNode
.This contribution is funded by Utsubo