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

WebGLMultipleRenderTargets: Add multisampling support. #24001

Merged
merged 27 commits into from
May 25, 2022

Conversation

RenaudRohlinger
Copy link
Collaborator

@RenaudRohlinger RenaudRohlinger commented May 3, 2022

Fixed #23300

Description

Added multisampling support for WebGLMultipleRenderTargets.

Example (build is included in order to run the example):


This contribution is funded by Utsubo

@CodyJasonBennett
Copy link
Contributor

CodyJasonBennett commented May 3, 2022

Comment on lines 179 to 180
renderTarget.depthTexture = new THREE.DepthTexture( 512, 512 );
renderTarget.depthTexture.type = THREE.FloatType;
Copy link
Contributor

@CodyJasonBennett CodyJasonBennett May 3, 2022

Choose a reason for hiding this comment

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

I don't see this used in the example anywhere. Just testing depth/stencil attachments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When using MRT in threejs that is the only way to get a proper depth buffer, otherwise you will get weird behaviors when two geometries overlap 🤷

@CodyJasonBennett
Copy link
Contributor

CodyJasonBennett commented May 3, 2022

Can also reproduce the black screen issue on iOS, but if you toggle multisampling off/on again, you can see that the textures aren't correctly re-populated via blit.

This isn't present in my previous WebGL 2 example (#23300 (comment)), so we must be going wrong somewhere.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 4, 2022

I wonder why the E2E test fails. The screenshot in the PR looks as expected...

@Mugen87
Copy link
Collaborator

Mugen87 commented May 4, 2022

Ideally, this PR is also tested on a Oculus Quest 2 since it affects its multisampling code path.

@RenaudRohlinger
Copy link
Collaborator Author

RenaudRohlinger commented May 5, 2022

I wonder why the E2E test fails. The screenshot in the PR looks as expected...

Ah, maybe it's because I pushed two different versions of the screenshot? "ERROR! Diff wrong in 0.521 of pixels in file: webgl2_multiple_rendertargets_multisampled"

EDIT: Seems to be fixed

@CodyJasonBennett
Copy link
Contributor

Ran through the WebXR examples (ball shooter, hands, layers) on my Quest 2 without issue.

Issues in iOS Safari are still evident. Tried digging locally and everything looks correct as far as the changes. Notably, textures appear to retain color passed to gl.clearBufferfv before calling gl.blitFramebuffer.

@RenaudRohlinger
Copy link
Collaborator Author

We tried with WebXR and I can confirm that everything seems to work fine.

Issues in iOS Safari are still evident. Tried digging locally and everything looks correct as far as the changes. Notably, textures appear to retain color passed to gl.clearBufferfv before calling gl.blitFramebuffer.

@CodyJasonBennett Are you sure that it is correctly working on your example and just a fortunate configuration? I wonder if you try to add a loop or a slightly more complex setup.

I would suspect a Metal WebGL issue.

@CodyJasonBennett
Copy link
Contributor

CodyJasonBennett commented May 5, 2022

I've tried toggling mipmaps and disabling everything that didn't deal with color attachments to compare with the blit method here https://gist.github.com/CodyJasonBennett/edbc4372d3c9a921ebc3e46cb5899ba8. Both this and the simplified demo appear to work.

I'm not sure how to verify whether the multi-sampled render buffers are populated after render either, but I found it peculiar that they seemingly have no color output (blit appears to be copying everything correctly).

@Mugen87
Copy link
Collaborator

Mugen87 commented May 5, 2022

Tested the PR a bit more on a Pixel 4a and macOS. Looking good so far 👍 .

I have currently no iOS device at hand so I can't investigate the mentioned black screen.

@Mugen87 Mugen87 changed the title [WebGLMultipleRenderTargets] Supports multisampling WebGLMultipleRenderTargets: Add multisampling support. May 5, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented May 6, 2022

I wonder what would be the real fix in WebGL?

If it's possible to reproduce the issue with a plain, minimalistic WebGL example (so without three.js), one could report the issue at the WebKit bugtracker.

BTW: An example with three.js would maybe work, too. Sometimes it is just easier for the browser devs to have a stripped-down test with no dependencies.

@RenaudRohlinger
Copy link
Collaborator Author

@Mugen87 I see, I will try to make a basic reproduction another time and submit it to WebKit bugtracker. In my opinion, this PR is now ready to merge. CI is ok, it does not change the previous behavior for MSAA anymore and works on all recent browsers 👍

@Mugen87
Copy link
Collaborator

Mugen87 commented May 6, 2022

Great! Can you then please remove the build files from the PR?

@RenaudRohlinger
Copy link
Collaborator Author

Done!

Create `DepthTexture` without parameters.
@ligaofeng0901
Copy link
Contributor

cool! The demo looks good

@ligaofeng0901
Copy link
Contributor

ligaofeng0901 commented May 22, 2022

Can this pr be merged in r141? Or there is something else to do?

@RenaudRohlinger
Copy link
Collaborator Author

@ligaofeng0901 In my opinion everything is ready to be merged and this PR does not introduce any change except enhancement. In our company, we've been using it in production for 2 weeks and had no issues reported 👍

@mrdoob mrdoob merged commit 714a21b into mrdoob:dev May 25, 2022
@mrdoob
Copy link
Owner

mrdoob commented May 25, 2022

Thanks!

@marcofugaro
Copy link
Contributor

Is it me or the MRT + multisample example is way laggier now? Even with samples set to 1.

https://raw.githack.com/mrdoob/three.js/764cf0fa5919e891bc259253e0464dc8033d5fcd/examples/webgl2_multiple_rendertargets.html

@RenaudRohlinger
Copy link
Collaborator Author

I used stencil buffer instead of a float depth texture to have a more precise quality. Using back float depth texture might be the solution 🤔

@RenaudRohlinger
Copy link
Collaborator Author

@marcofugaro Maybe the previous version that was using temporary FBO dedicated for the MRT Multisample was better? The new one was introduced to prevent the usage of temporary FBO but might be heavier indeed.3ba011d#diff-6bdac4da85be606f09e2ab24b41c761b23ced0d9acaa720e2f2200d938a714e9R1745-R1759

@CodyJasonBennett

@RenaudRohlinger
Copy link
Collaborator Author

@jcyuan You should try THREE.FloatType for the depthTexture. Also I'm not sure stencil will work, better disable stencilBuffer if possible.

@jcyuan
Copy link

jcyuan commented Aug 11, 2022

@jcyuan You should try THREE.FloatType for the depthTexture. Also I'm not sure stencil will work, better disable stencilBuffer if possible.

solved! it was a mistake by re-use FBOs, i created 2 FBOs for 4 RTs.
really sorry, and thanks so much for your help!

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* setUsage does not update attributes after the buffer is created

* check is not necessary in the buffer creation

* clean code

* prevent unnecessary double buffer update

* fix lint

* [WebGLMultipleRenderTarget] Support Multisampling

[WebGLMultipleRenderTarget] Support Multisampling

clean bufferattributes code

clean bufferattributes code

* removed unused code

* added build for tests

* pupeeter screenshot

* better example

* resets the active FBO back to the multisampled FBO after blitting.

* lint

* use mask instead of COLOR_BUFFER_BIT

* Update webgl2_multiple_rendertargets_multisampled.html

* wip fix safari

* removed use of temporary buffers and fix safari and ios example

* fix lint

* fix pupeeter example?

* prevent unecessary fbo creation for non-mrt

* fix

* fix puppeeter rendering black screenshot

* remove build

* Update webgl2_multiple_rendertargets_multisampled.html

Create `DepthTexture` without parameters.

* remove unecessary framebuffer render and texture instructions

* example: usage of stencil buffer instead of depth texture

Co-authored-by: Michael Herzog <[email protected]>
@prophetw
Copy link

OMG So amazing! Any document about this implement? Confused Why ? @RenaudRohlinger

@RenaudRohlinger
Copy link
Collaborator Author

@prophetw Just like a WebGLRenderTarget you have to use the samples property:

renderTarget = new THREE.WebGLMultipleRenderTargets(
	window.innerWidth * window.devicePixelRatio,
	window.innerHeight * window.devicePixelRatio,
	2,
	{
	  samples: 4
	}
);

// or renderTarget.samples = 4

Here the official example:
https://threejs.org/examples/?q=mrt#webgl2_multiple_rendertargets

@arminheller
Copy link

arminheller commented Jul 26, 2023 via email

@prophetw
Copy link

I investigated MSAA MRT FBO, and got a result that WebGL2 may only support one Color_ATTACHMENT copy(blitFramebuffer).
But after see this commit, it makes me feel what!!!!! Curious about the WebGL blitFramebuffer copy part why write like that.

@prophetw
Copy link

And i found a issue
MSAA performance drop with depth-stencil blit
https://groups.google.com/g/webgl-dev-list/c/iwIYWPPJwoQ

@CodyJasonBennett
Copy link
Contributor

CodyJasonBennett commented Jul 27, 2023

See #26143. This only affects D3Dx via ANGLE which is software emulated here.

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.

Multisampling over Multiple Render Target
9 participants