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

[3.x] Fix alpha scissor shadow casting support #58959

Merged
merged 1 commit into from
Mar 11, 2022

Conversation

Ansraer
Copy link
Contributor

@Ansraer Ansraer commented Mar 10, 2022

3.x version of #58954, should fix #58924.

@clayjohn
Copy link
Member

should fix #58924.

Did you test the MRP in #58924?

@Ansraer
Copy link
Contributor Author

Ansraer commented Mar 10, 2022

Yes, appears to work the way it should:

Screenshot

image

Just noticed that my one-line change managed to break the mono build. No clue how that happened...

@Ansraer Ansraer force-pushed the 3.x-alpha-scissor branch from 512330c to a88a050 Compare March 10, 2022 11:30
@Ansraer
Copy link
Contributor Author

Ansraer commented Mar 10, 2022

Huh, just forcing it to run again fixed it.

@Calinou
Copy link
Member

Calinou commented Mar 10, 2022

Yes, appears to work the way it should:

PS: You need to add a blank line after <details> (or <summary>) for Markdown to work inside the collapsed block. I edited your comment accordingly, but remember to do this in the future 🙂
The same applies to GitHub attachment video previews, by the way. They require a blank line before and after the link.

@clayjohn
Copy link
Member

The logic is escaping me, so I am going to write some notes about how I understand what is going on:

First has_alpha is created with two parts has_base_alpha and has_blend_alpha. has_blend_alpha is true if a blend mode other than mix is used (in the MRP, blend mode mix is used) so has_blend_alpha should be false. has_base_alpha is true if the shader uses the screen texture, the depth texture or if it writes to ALPHA without using the alpha scissor. The MRP does not use screen texture or depth texture and uses alpha scissor so has_base_alpha should be false. Therefore, has_alpha should be false.

https://github.com/godotengine/godot/blob/a88a0501339a3d0741a1f421b778396e84200775/drivers/gles3/rasterizer_scene_gles3.cpp#L2281-L2283

Then we get to your change. You have added a check to whether alpha scissor is used. This way, even if an object has alpha, it will be forced into the opaque pass if alpha scissor is used.
https://github.com/godotengine/godot/blob/a88a0501339a3d0741a1f421b778396e84200775/drivers/gles3/rasterizer_scene_gles3.cpp#L2326

But, following the logic above, when alpha scissor is used, has_alpha should only be true if using depth texture, screen texture, or a non-mix blend mode. The MRP doesn't use any of those, so has_alpha should not be true. so your PR shouldn't fix this bug, but it does!

Okay, on reflection, I don't think this is the right approach for two reasons:

  1. This will force objects that read from depth texture and screen texture into the opaque pipeline if they use alpha scissor which will break those objects (screen texture is captured after the opaque pass and you can't read from and write to depth at the same time).
  2. We need to figure out why has_alpha is true despite the fact that all the conditions should be false. My guess is that a condition is being set incorrectly somewhere leading to has_alpha being true when it shouldn't.

@Ansraer
Copy link
Contributor Author

Ansraer commented Mar 10, 2022

Fair enough, I will look into it again tomorrow.

@Ansraer Ansraer force-pushed the 3.x-alpha-scissor branch from a88a050 to 2fb998b Compare March 11, 2022 15:18
@Ansraer
Copy link
Contributor Author

Ansraer commented Mar 11, 2022

Had some spare time to look into this again. You were right, there was a significantly cleaner solution to this than to just assume that everything with alpha scissors was either fully transparent or opaque.

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.

Great work!

@akien-mga akien-mga merged commit 7a3a386 into godotengine:3.x Mar 11, 2022
@akien-mga
Copy link
Member

Thanks!

@Ansraer Ansraer deleted the 3.x-alpha-scissor branch March 11, 2022 17:19
@akien-mga
Copy link
Member

Cherry-picked for 3.4.4.

@akien-mga akien-mga changed the title [3.x] Fix alpha scissor support [3.x] Fix alpha scissor shadow casting support Aug 4, 2022
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.

4 participants