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

Fix transparent viewport backgrounds with custom clear color #79876

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

LRFLEW
Copy link
Contributor

@LRFLEW LRFLEW commented Jul 25, 2023

This fixes #79778. This code change zeroes out the alpha component of the clear color. This basically just ports a change made in #61109 from the Forward+ renderer to the Mobile renderer, as well as an equivalent change to the Compatibility renderer.

@LRFLEW LRFLEW requested a review from a team as a code owner July 25, 2023 09:24
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

If you want you can do the same fix for OpenGL (either in this PR or a followup). Clear color is set here:

if (!keep_color) {
glClearBufferfv(GL_COLOR, 0, clear_color.components);
}

@clayjohn clayjohn added this to the 4.2 milestone Jul 25, 2023
@LRFLEW LRFLEW changed the title Mobile: fix transparent viewport backgrounds Fix transparent viewport backgrounds Jul 25, 2023
@LRFLEW
Copy link
Contributor Author

LRFLEW commented Jul 25, 2023

I went ahead and made the change to the PR for the Compatibility renderer. I tested it, and it is working, but with a caveat: The viewport's transparency doesn't seem to update until you reload the scene. I'm not sure why, but I'll look into it.

@LRFLEW LRFLEW force-pushed the mobalpha branch 3 times, most recently from 3febf88 to dad6ed1 Compare July 25, 2023 22:21
@LRFLEW
Copy link
Contributor Author

LRFLEW commented Jul 25, 2023

For the Compatibility renderer, I changed if (render_data.transparent_bg) to if (rt->is_transparent), which resolved the issue of the background transparency not updating until the scene was reloaded. I don't really understand why the two are different, but the change worked. EDIT: Is this a bug? It doesn't make sense to me that these would be different, and I'm starting to think this may be a latent bug that I brought out. I can try looking into it a bit. EDIT 2: Seems like what's happening is that RenderSceneBuffersGLES3::configure is what sets rb->is_transparent, but it isn't called from RendererViewport::viewport_set_transparent_background, so it doesn't update if the viewport's background transparency is updated. It doesn't seem like a trivial fix, though, as calling configure from there requires specifying a lot of arguments that aren't necessarily available, and I'm wondering if we really need rb->is_transparent at all if we have rt->is_transparent.

I also made another change to force consistency between the renderers. Right now, setting an alpha on the custom clear color is ignored by the Forward+ and Mobile renderers. However, the alpha of the clear color was used when clearing the viewport in the Compatibility renderer, even with the viewport set to use an opaque background. I decided to force the alpha of the clear color to be 1 when the render target is not transparent to make the Compatibility renderer's behavior match the other renderers.

@LRFLEW
Copy link
Contributor Author

LRFLEW commented Jul 27, 2023

From what I can tell, RenderSceneBuffersGLES3::is_transparent is redundant, since it's just duplicating GLES3::RenderTarget::is_transparent, and trying to keep them in sync looks to be a problem. I added a commit (not an amended commit since it might want to be approved / declined separately, and wanted to keep an easy way to undo it) that removes RenderSceneBuffersGLES3::is_transparent entirely, which turned out to be fairly simple. This directly addresses the issue I had when making this work with the Compatibility renderer, as well as addressing any other parts of the Compatibility renderer that depended on render_data.transparent_bg being accurate.

@clayjohn
Copy link
Member

From what I can tell, RenderSceneBuffersGLES3::is_transparent is redundant, since it's just duplicating GLES3::RenderTarget::is_transparent, and trying to keep them in sync looks to be a problem. I added a commit (not an amended commit since it might want to be approved / declined separately, and wanted to keep an easy way to undo it) that removes RenderSceneBuffersGLES3::is_transparent entirely, which turned out to be fairly simple. This directly addresses the issue I had when making this work with the Compatibility renderer, as well as addressing any other parts of the Compatibility renderer that depended on render_data.transparent_bg being accurate.

Thanks for looking into that more deeply. Indeed it makes sense to remove is_transparent from render buffers. That was an artifact from before we merged RT and RB.

Go ahead and squash the commits together, then we can merge this!

@YuriSizov
Copy link
Contributor

Also needs a rebase now :)

@LRFLEW LRFLEW changed the title Fix transparent viewport backgrounds Fix transparent viewport backgrounds with custom clear color Jul 30, 2023
@YuriSizov YuriSizov merged commit 8b12849 into godotengine:master Jul 31, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Viewport background transparency not working with custom clear color in Mobile and Compatibility renderers
4 participants