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

Remove WebGLMultisampleRenderTarget. #23455

Merged
merged 7 commits into from
Feb 11, 2022
Merged

Remove WebGLMultisampleRenderTarget. #23455

merged 7 commits into from
Feb 11, 2022

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Feb 10, 2022

Related issue: #23444 (comment)

Description

This PR removes WebGLMultisampleRenderTarget and introduces WebGLRenderTarget.samples instead.

WebGLRenderTarget.samples is 0 by default and can only be used with WebGL 2. If set, it triggers the multisample render target code path.

I was able to integrate the properties ignoreDepthForMultisampleCopy, useRenderToTexture and useRenderbuffer into the renderer so only samples is added to WebGLRenderTarget.

I've tested the change with the multisampling examples but not with WebXR yet. The WEBGL_multisampled_render_to_texture code path is only used by Oculus Browser so far.

Updated the build files for testing: https://raw.githack.com/Mugen87/three.js/dev62/examples/index.html

/cc @cabanier @arpu

@Mugen87 Mugen87 marked this pull request as draft February 10, 2022 19:54
@cabanier
Copy link
Contributor

This change is breaking WebXR.
I'm getting a crash in:
const renderTargetProperties = renderer.properties.get( renderTargetProperties );

@mrdoob mrdoob added this to the r138 milestone Feb 10, 2022
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 11, 2022

Tested with a Quest 2 today, too. Examples work as expected.

@vanruesc Do you mind testing this change in your project, too?

@Mugen87 Mugen87 marked this pull request as ready for review February 11, 2022 08:21
@vanruesc
Copy link
Contributor

I've just tested it and can confirm that it works.

I did run into [.WebGL-0x1ec401258700] GL_INVALID_OPERATION: Texture is immutable. under certain circumstances, but I think that was due to a misconfiguration on my side which worked fine with WebGLMultisampleRenderTarget for some reason.

@mrdoob
Copy link
Owner

mrdoob commented Feb 11, 2022

Hmm, if I understand the PR correctly... Reflector will now no longer work in WebGL 1 (unless the user specifies multisample: 1) and it will log a warning in the console right?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 11, 2022

I did run into [.WebGL-0x1ec401258700] GL_INVALID_OPERATION: Texture is immutable. under certain circumstances,

This warning is related to the texStorage() API. It usually appears when a texture is defined with a certain dimension via texStorage2D() and later updated with a larger dimension via texSubImage2D() (which is not valid).

It's the responsibility of the app that a textures dimensions does not change after its initial use.

Reflector will now no longer work in WebGL 1 (unless the user specifies multisample: 1) and it will log a warning in the console right?

Yes. However, the user has to specify a multisample of 0 to deactivate multisampling. And this already happens without the PR.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 11, 2022

@marcofugaro For some reasons the unit tests now fail at GitHub with the message:

message: "No tests matched the filter "!-webonly"."

Do you know why this suddenly happens?

@mrdoob
Copy link
Owner

mrdoob commented Feb 11, 2022

Reflector will now no longer work in WebGL 1 (unless the user specifies multisample: 1) and it will log a warning in the console right?

Yes. However, the user has to specify a multisample of 0 to deactivate multisampling. And this already happens without the PR.

I think we should follow what we do with texture.anisotropy where the user can set it to 16 and the renderer will do as much as the GPU supports.

So if the user sets rendertarget.samples to 4 the renderer will try to use multisample, otherwise it will render without and it'll look aliased.

That way the user doesn't have to check for support and things will continue working.

@marcofugaro
Copy link
Contributor

For some reasons the unit tests now fail at GitHub with the message:

@Mugen87 I did the trick of removing -r failonlyreporter (not ideal I know, should be changed):

image

Looks like tests were broken in #23460, the MathUtils import must have the .js extension.

@marcofugaro marcofugaro mentioned this pull request Feb 11, 2022
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 11, 2022

So if the user sets rendertarget.samples to 4 the renderer will try to use multisample, otherwise it will render without and it'll look aliased.

Okay, I've implemented it like suggested. Meaning the renderer does not report an error if the app tries to use multisampling without WebGL 2. The renderer just ignores the multisampling settings and uses a default render target configuration.

@mrdoob mrdoob merged commit 48b05d3 into mrdoob:dev Feb 11, 2022
@mrdoob
Copy link
Owner

mrdoob commented Feb 11, 2022

Thanks thanks!

@arpu
Copy link

arpu commented Feb 27, 2022

tested on production with r138 and all works fine

0b5vr added a commit to 0b5vr/three-ts-types that referenced this pull request Mar 1, 2022
joshuaellis pushed a commit to three-types/three-ts-types that referenced this pull request Mar 1, 2022
donmccurdy pushed a commit to donmccurdy/three.js that referenced this pull request Mar 10, 2022
* Remove WebGLMultisampleRenderTarget.

* THREE.Legacy.js: Add WebGLMultisampleRenderTarget.

* Exampels: Clean up.

* WebGLRenderer: Use multisampling when possible without reporting errors.

* Update WebGLRenderer.js

Co-authored-by: mrdoob <[email protected]>
qiweicao pushed a commit to qiweicao/three.js that referenced this pull request Apr 9, 2024
* Remove WebGLMultisampleRenderTarget.

* THREE.Legacy.js: Add WebGLMultisampleRenderTarget.

* Exampels: Clean up.

* WebGLRenderer: Use multisampling when possible without reporting errors.

* Update WebGLRenderer.js

Co-authored-by: mrdoob <[email protected]>
nianxy pushed a commit to qiweicao/three.js that referenced this pull request Apr 10, 2024
* Remove WebGLMultisampleRenderTarget.

* THREE.Legacy.js: Add WebGLMultisampleRenderTarget.

* Exampels: Clean up.

* WebGLRenderer: Use multisampling when possible without reporting errors.

* Update WebGLRenderer.js

Co-authored-by: mrdoob <[email protected]>
nianxy pushed a commit to oppenfuture/three.js that referenced this pull request Apr 10, 2024
* WebGLMultipleRenderTargets: Add Options to Constructor (mrdoob#22772)

* WebGLMultipleRenderTargets: Add Options to Constructor

* Update WebGLMultipleRenderTargets Documentation with new options

* Add line break

* WebGLMultipleRenderTargets: Use default parameter.

Co-authored-by: Michael Herzog <[email protected]>

* WebGLRenderTarget: Clone depthTexture in copy(). (mrdoob#23462)

* Remove WebGLMultisampleRenderTarget. (mrdoob#23455)

* Remove WebGLMultisampleRenderTarget.

* THREE.Legacy.js: Add WebGLMultisampleRenderTarget.

* Exampels: Clean up.

* WebGLRenderer: Use multisampling when possible without reporting errors.

* Update WebGLRenderer.js

Co-authored-by: mrdoob <[email protected]>

* WebGLTextures: Fix depth textures with multisampling. (mrdoob#23611)

* WebGLRenderer: Fix setRenderTargetTextures(). (mrdoob#23644)

* fix transmission use multisample rtt error

* delete type definition of WebGLMultisampleRenderTarget

---------

Co-authored-by: Johnathon Selstad <[email protected]>
Co-authored-by: Michael Herzog <[email protected]>
Co-authored-by: mrdoob <[email protected]>
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.

6 participants