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

Add support for EXT_multisampled_render_to_texture extension and use it for WebXR #22550

Merged
merged 1 commit into from
Sep 17, 2021
Merged

Conversation

cabanier
Copy link
Contributor

The current approach via renderbuffers has the limitation that we can't apply foveation.
Foveation helps a lot for GPU limited scenes so we need to have support for both MSAA and foveation.

EXT_multisampled_render_to_texture is used under the hood in regular WebXR in the Oculus browser and this uses that extension so WebXR Layers (and potential other areas that needs MSAA) can use this.

The WebXR manager was also extended to support this.

This contribution is funded by Oculus.

@mrdoob
Copy link
Owner

mrdoob commented Sep 17, 2021

After merging this, do you think we'll be able to remove the current renderbuffers code path any time soon?

@cabanier
Copy link
Contributor Author

After merging this, do you think we'll be able to remove the current renderbuffers code path any time soon?

I suspect that almost all this code can go away once #22079 is implemented.

@mrdoob
Copy link
Owner

mrdoob commented Sep 17, 2021

Excellent!

@mrdoob mrdoob merged commit c789b20 into mrdoob:dev Sep 17, 2021
@mrdoob
Copy link
Owner

mrdoob commented Sep 17, 2021

Thanks!

@cabanier
Copy link
Contributor Author

cabanier commented Sep 20, 2021

@mrdoob I have a PR in progress (#22558).
It is working correctly except in the case of multiple render targets and the MSAA extension. With that combination, the content from the other render target only shows in the left eye. Do you know offhand why that would be?

@mrdoob
Copy link
Owner

mrdoob commented Sep 20, 2021

Hmm, I can't think of the reason at the moment 🤔

@mrdoob
Copy link
Owner

mrdoob commented Sep 20, 2021

Oh, maybe we should implement the stack approach in setRenderTarget() too?

/cc @Mugen87

@cabanier
Copy link
Contributor Author

So, it's working in the webxr case, non-AA aliased and aliased using renderbuffers.
It just doesn't work using the msaa render-to-texture extension.

@cabanier
Copy link
Contributor Author

@mrdoob I figured out why it wasn't working. The MSAA extension will discard the depth texture under certain circumstances.
I worked around this in my change.
It's up for review now. Sorry that it got so big :-\

@mrdoob
Copy link
Owner

mrdoob commented Sep 28, 2021

@mrdoob I figured out why it wasn't working. The MSAA extension will discard the depth texture under certain circumstances.

Is that a three.js bug? Oculus browser bug? Or WebGL spec bug?

@cabanier
Copy link
Contributor Author

@mrdoob I figured out why it wasn't working. The MSAA extension will discard the depth texture under certain circumstances.

Is that a three.js bug? Oculus browser bug? Or WebGL spec bug?

It's by design :-(
I'm trying to get it changed.

If texture is a depth or stencil texture, the contents of the multisample
buffer is discarded rather than resolved - equivalent to the application
calling InvalidateFramebuffer for this attachment.

https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_multisampled_render_to_texture2.txt

@cabanier
Copy link
Contributor Author

@mrdoob The PR fixes it by using renderbuffers which don't suffer from this issue.
On browser that require the depth texture to be populated, I'm reverting to regular multisampling.

@cabanier
Copy link
Contributor Author

cabanier commented Sep 28, 2021

So, it's working in the webxr case, non-AA aliased and aliased using renderbuffers. It just doesn't work using the msaa render-to-texture extension.

@mrdoob @Mugen87 Should I revert the PR that introduced the extension or patch it so it works?

mrdoob pushed a commit that referenced this pull request Oct 4, 2021
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.

2 participants