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

WebGLTextures: Fix multisampled rendering for non-Quest VR browsers. #24689

Merged
merged 3 commits into from
Sep 25, 2022

Conversation

cabanier
Copy link
Contributor

@cabanier cabanier commented Sep 23, 2022

When the WEBGL_multisampled_render_to_texture extension is not available, three.js falls back to using renderbuffers to get support for multisampled rendering.
However, there was a change that forces the renderbuffer to sRGB. This result in a GL error when it tries to copy the MSAA renderbuffer into the WebXR texture.

The fix now correctly flags the getInternalFormat call to not choose sRGBA.

This contribution is funded by Meta

@cabanier cabanier marked this pull request as ready for review September 23, 2022 16:54
@Mugen87 Mugen87 added this to the r145 milestone Sep 24, 2022
@Mugen87 Mugen87 changed the title Fix multisampled rendering for non-Quest VR browsers WebGLTextures: Fix multisampled rendering for non-Quest VR browsers. Sep 24, 2022
@cabanier
Copy link
Contributor Author

The title is a bit confusing. Three is currently broken for:

  • non-quest browser that support layers (I don't know if any exist)
  • quest browsers were the WebXR Layers feature was disabled.
  • browsers that run experiences that use the webxr layers polyfill

This error was found by developers that are trying to use the polyfill so their layers code works on other browser.

@Mugen87 Mugen87 merged commit cf88d02 into mrdoob:dev Sep 25, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 25, 2022

I've tested the PR in more detail on dev and encounter the following problem on dev (tested with Chrome on macOS): When you change the render target's encoding in webgl2_multisampled_renderbuffers to THREE.sRGBEncoding, the right side of the example is not rendered anymore and the following is logged.

GL_INVALID_OPERATION: Attempt to blit from a multisampled framebuffer and the bounds or format of the color buffer don't match with the draw framebuffer.

The render target creation is just changed from

const renderTarget = new THREE.WebGLRenderTarget( size.width, size.height, { samples: 4 } );

to

const renderTarget = new THREE.WebGLRenderTarget( size.width, size.height, { samples: 4, encoding: THREE.sRGBEncoding } );

This is a regression since having multisampled sRGB render targets should work as before.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 25, 2022

Maybe we need a separate code path (or additional checks) to differentiate between WebXR and non-WebXR?

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 26, 2022

I've reverted the PR for now since r145 will be released this Wednesday. Planing this change for r146 so it's more time to test and verify.

@cabanier
Copy link
Contributor Author

Maybe we need a separate code path (or additional checks) to differentiate between WebXR and non-WebXR?

I'm looking at this code for depth buffers and it seems there is a lot of confusion in this area.
I agree that we might need different code paths.

@cabanier
Copy link
Contributor Author

@Mugen87 I special cased WebXR and created #24698

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