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: Simplify updateMultisampleRenderTarget(). #28141

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Apr 15, 2024

Related issue: -

Description

The idea of the PR is to put the invalidation calls of gl.invalidateFramebuffer() in updateMultisampleRenderTarget() at one place.

It also makes sure to reuse array instances for the invalidation to avoid unnecessary object allocation.

@Mugen87 Mugen87 added this to the r164 milestone Apr 15, 2024
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 15, 2024

@cabanier Does this new approach look good to you?

Copy link

github-actions bot commented Apr 15, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
674.7 kB (167.1 kB) 674.7 kB (167.1 kB) -6 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
454.1 kB (109.6 kB) 454.1 kB (109.6 kB) +5 B

@cabanier
Copy link
Contributor

@cabanier Does this new approach look good to you?

Yes, it looks reasonable. This code is only exercised when "multisample render to texture" is not available so it should be tested on non-quest devices.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 16, 2024

For testing, I've set supportsInvalidateFramebuffer to true and interestingly the glitch mentioned in the other issue is now gone with this PR. I suspect this happens because the invalidation occurs now once for draw and read buffer. Previously depth was invalidated first and then depth+color for the read buffer again. Maybe this order was responsible for the visual issue in #28132 (comment).

I'll merge to finalize the performance related improvements for r164. For r165, I'll try to remove supportsInvalidateFramebuffer for testing so gl.invalidateFramebuffer() is always used. I want to do checks with different devices in order to find out if the glitches still appear.

@Mugen87 Mugen87 merged commit e395358 into mrdoob:dev Apr 16, 2024
12 checks passed
@cabanier
Copy link
Contributor

I remember having a similar issue on Safari before but now it seems to be on Chrome. Would it make sense to file a bug?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 16, 2024

Yes, that would be good!

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