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] Prevent use of Opaque Pre-Pass's threshold on top of Alpha Scissor #45372

Merged
merged 1 commit into from
Feb 11, 2022

Conversation

Firepal
Copy link

@Firepal Firepal commented Jan 22, 2021

In SpatialMaterials and ShaderMaterials, both Alpha Scissor and Opaque Pre-Pass can discard fragments (== create fully-transparent pixels), but Opaque Pre-Pass has a hardcoded threshold of 0.1 that the end-developer cannot modify.

This PR makes SpatialMaterials use the alpha scissor threshold instead of Opaque Pre-Pass's own threshold whenever Alpha Scissor is in use. This prevents the strange behavior where any fragment with an ALPHA value under 0.1 would be discarded, no matter what Alpha Scissor value you set.

Using blue noise for ALPHA_SCISSOR and a radial gradient:

image
image

Scene in image: PrepassTesting.zip

Related: #45288

Do tell me if this breaks the intent of Opaque Pre-Pass.

@Firepal Firepal requested a review from reduz as a code owner January 22, 2021 21:03
@Calinou Calinou added this to the 3.2 milestone Jan 22, 2021
@Calinou Calinou added the bug label Jan 22, 2021
@Firepal Firepal changed the title Prevent use of Opaque Pre-Pass's threshold on top of Alpha Scissor [3.2] Prevent use of Opaque Pre-Pass's threshold on top of Alpha Scissor Jan 23, 2021
@Firepal Firepal requested a review from a team as a code owner March 12, 2021 12:26
@clayjohn
Copy link
Member

clayjohn commented Mar 12, 2021

Hi, sorry I missed this. I like the proposed change, it makes sense to have the alpha scissor override the depth-prepass threshold.

That being said, the reason that the current behaviour hasn't changed, despite being awkward, is that small tweaks to how the depth prepass work have big impacts on performance, and visuals. It is very easy to accidentally break everyone's scenes when we introduce changes.

So before a change like this is considered, we need to be absolutely sure that it is going to completely break real scenes.

Further discussion here where I propose exactly the change you have made here :P

@Firepal
Copy link
Author

Firepal commented Mar 12, 2021

@clayjohn I've been using this change in a custom Godot build for a few weeks now and haven't found issues as of yet, and I couldn't find any collisions with the "depth-prepass + blending" behavior, but I do understand there could be edge cases that I missed (or am completely unaware of), so I share the same worry. 😅

@clayjohn
Copy link
Member

Do you mind testing the MRP in #36669 with your custom build?

@Firepal
Copy link
Author

Firepal commented Mar 12, 2021

Was that issue fixed? Because 3.2.4 RC4 has that problem, but my custom build doesn't.

prepass.mp4

Base automatically changed from 3.2 to 3.x March 16, 2021 11:11
@aaronfranke aaronfranke modified the milestones: 3.2, 3.3 Mar 16, 2021
@akien-mga akien-mga changed the title [3.2] Prevent use of Opaque Pre-Pass's threshold on top of Alpha Scissor [3.x] Prevent use of Opaque Pre-Pass's threshold on top of Alpha Scissor Mar 26, 2021
@akien-mga akien-mga modified the milestones: 3.3, 3.4 Mar 26, 2021
@Chaosus Chaosus modified the milestones: 3.4, 3.5 Nov 8, 2021
@akien-mga
Copy link
Member

What's the current status on this PR? Needs a rebase, and if it still works as intended it should be good to merge.

@akien-mga akien-mga merged commit d0b446c into godotengine:3.x Feb 11, 2022
@akien-mga
Copy link
Member

Thanks!

@Calinou
Copy link
Member

Calinou commented Feb 12, 2022

PS: If anyone has time, please check the behavior in master to make sure it's consistent with the new behavior in 3.x.

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.

6 participants