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 viewport with transparent bg changed to solid black if enable fxaa or debanding. #54585

Merged
merged 1 commit into from
May 16, 2022
Merged

Fix viewport with transparent bg changed to solid black if enable fxaa or debanding. #54585

merged 1 commit into from
May 16, 2022

Conversation

Kinwailo
Copy link

@Kinwailo Kinwailo commented Nov 4, 2021

If enable fxaa or debanding in viewport with transparent bg, the transparent bg will change to solid black color. It is because the shader always output alpha as 1.0.

Bugsquad edit: This closes #45208.

@Kinwailo Kinwailo requested a review from a team as a code owner November 4, 2021 07:17
@Calinou Calinou added this to the 3.5 milestone Nov 4, 2021
@Calinou
Copy link
Member

Calinou commented Nov 4, 2021

Good catch 🙂

Does this fully address #45208, or are there still parts of the tonemapping shader that don't play well with Transparent Bg?

@Kinwailo
Copy link
Author

Kinwailo commented Nov 5, 2021

Good catch slightly_smiling_face

Does this fully address #45208, or are there still parts of the tonemapping shader that don't play well with Transparent Bg?

Post processing is bypass if transparent is enabled in driver code, and the shader of glow / dof produce "black edge". I have changed the code and shader, should fix #45208.

@Calinou
Copy link
Member

Calinou commented Feb 6, 2022

I tested this PR rebased against the latest 3.x branch as of writing. It indeed fixes the issue in the original MRP 🙂

Before

2022-02-06_11 57 40

After (with this PR)

2022-02-06_12 27 29

When Transparent Bg is disabled, the appearance is identical:

Before

2022-02-06_12 27 38

After (with this PR)

2022-02-06_12 27 52

drivers/gles3/shaders/tonemap.glsl Outdated Show resolved Hide resolved
drivers/gles2/shaders/tonemap.glsl Outdated Show resolved Hide resolved
@bfloch
Copy link
Contributor

bfloch commented Feb 28, 2022

I was able to verify this with a project that was suffering from the issue in 3.4.3. Works great, no complaints.

@bfloch
Copy link
Contributor

bfloch commented Mar 1, 2022

EDIT: The issue and the transparency not working was since my root node that hosted the viewport was 3D not 2D. So I don't think this is a real blocker. Original message below.

A negative side-effect I just noticed: In transparent viewports that are setup via a ViewportTexture in a TextureRect and not ViewportController the (supposedly transparent) background looks overly bright.
Somehow I noticed that transparency does not seem to work in either 3.4.3 or this PR this way, but that may be a separate issue or something wrong on my end.

The screenshot shows the PR at the top:
godot_transparency_effect

@Calinou
Copy link
Member

Calinou commented May 3, 2022

@Kinwailo Looks good to me, please squash the commits together so we can merge this. See PR workflow for instructions 🙂

@clayjohn
Copy link
Member

clayjohn commented May 4, 2022

This looks pretty good and is very promising.

The only issue I noticed while testing is that FXAA doesn't weight the transparent samples by their alpha, so you end up with black edges when using FXAA.

This shouldn't be too hard to fix, it may just be a matter of premultiplying the sampled colour by its transparency so that transparent pixels don't contribute their color.

FXAA on
Screenshot from 2022-05-04 13-18-14

FXAA off
Screenshot from 2022-05-04 13-18-18

@Kinwailo
Copy link
Author

Kinwailo commented May 6, 2022

fixed fxaa with alpha.

@Kinwailo
Copy link
Author

Kinwailo commented May 6, 2022

fixed a mistake.

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.

Just the one change needed. Other than that, this is looking good to go!

drivers/gles2/shaders/tonemap.glsl Outdated Show resolved Hide resolved
@Kinwailo
Copy link
Author

Just the one change needed. Other than that, this is looking good to go!

should change the shader for gles3 too?

@clayjohn
Copy link
Member

Just the one change needed. Other than that, this is looking good to go!

should change the shader for gles3 too?

Yes please

@Kinwailo
Copy link
Author

Just the one change needed. Other than that, this is looking good to go!

should change the shader for gles3 too?

Yes please

changed

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! Thank you!

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Thanks! Congratulations for your first merged pull request 🎉

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.

5 participants