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

WebGLRenderer: Ensure correct clear after transmission. #28445

Merged
merged 1 commit into from
May 20, 2024

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented May 20, 2024

Fixed #28420.

Description

This PR fixes a regression introduced in #28118.

After rendering the transmission, the depth (and color) buffer might not be writable which is required for the clear triggered via background.render(). Before #28118 the clear happened at the very beginning in render() so the buffer states were correct.

The issue mentioned in #28420 (comment) is unrelated to this specific regression. When using preserveDrawingBuffer: true and setting autoClear = false, textured backgrounds are not compatible since they don't perform depth testing. That means they always overwrite what is present in the (preserved) color attachment which is unwanted if you head for a trails-like effects.

/cc @mrxz

Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
679 kB (168.3 kB) 679.1 kB (168.3 kB) +84 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
457 kB (110.3 kB) 457.1 kB (110.3 kB) +87 B

@Mugen87 Mugen87 added this to the r165 milestone May 20, 2024
@Mugen87 Mugen87 merged commit 6be05e4 into mrdoob:dev May 20, 2024
12 checks passed
@mrxz
Copy link
Contributor

mrxz commented May 20, 2024

@Mugen87 I see, completely overlooked that possibility. Wouldn't that also mean that the shadow map rendering could theoretically leave the depth or colour write disabled? Haven't looked into it in detail, but it seems that a customDepthMaterial with colorWrite: false could case the same issue.

Wondering if it might be better to let WebGLBackground set the state it needs itself.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 20, 2024

Wouldn't that also mean that the shadow map rendering could theoretically leave the depth or colour write disabled?

I don't see in WebGLShadowMap that buffers are restored so it seems that could be the case.

Wondering if it might be better to let WebGLBackground set the state it needs itself.

Interesting. Maybe WebGLBackground.render() would be a good place for that.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 20, 2024

Or maybe even in WebGLRenderer.clear(). In any event, depth and color buffers should be writable at the end of the frame as before to make subsequent (third-party) code isn't affected.

@mrxz
Copy link
Contributor

mrxz commented May 20, 2024

Or maybe even in WebGLRenderer.clear()

That might be a better option, actually. In general I'd assume that calling clear() performs a clear even if the current state would have depth or color writes disabled. Based on #18973 it seems that the Reflector was initially written with this assumption as well, but now manually enables depth writes before calling clear(). Which makes me realize that nested render calls could happen with any state, and thus can't really call clear without manually setting depthTest, depthWrite and colorWrite. From that POV it would be nice if clear() handled this internally, as now Reflector and Refractor might still have some uncovered edge cases where they'd fail.

At the same time, there might very well be code working under the assumption that clear() 'respects' the current state. So making changes to clear() isn't without risks.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 21, 2024

At the same time, there might very well be code working under the assumption that clear() 'respects' the current state. So making changes to clear() isn't without risks.

That was the main reasons why I hesitated with changing WebGLRenderer.clear(). I don't want to change semantics because of potential side effects. Let's start with WebGLBackground.render() until we feel more confident with updating WebGLRenderer.clear().

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.

Weird r164 WebGL renderer regression with GLB model
2 participants