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

Overhead in fragment discard for opaque_prepass #77600

Closed
BastiaanOlij opened this issue May 29, 2023 · 3 comments · Fixed by #79865
Closed

Overhead in fragment discard for opaque_prepass #77600

BastiaanOlij opened this issue May 29, 2023 · 3 comments · Fixed by #79865

Comments

@BastiaanOlij
Copy link
Contributor

Godot version

4.x

System information

Any

Issue description

Myself and @clayjohn looked into this before but were busy focusing on other things at the time so didn't investigate this further.

In the scene shader of the compatibility renderer, the forward and mobile implementation we have a variation of this check:

#ifdef USE_OPAQUE_PREPASS
	if (alpha < scene_data.opaque_prepass_threshold) {
		discard;
	}
#endif // USE_OPAQUE_PREPASS

As best as I can tell USE_OPAQUE_PREPASS is set by default for our StandardMaterial so this code is always included however for normal fully opaque materials we wouldn't want this code to be included especially when we're optimising for depth prepass or shadow rendering and want the fragment code completely filtered out. The scenario we were investigating had this introducing a needless texture read because it couldn't filter out this check. While it was inconclusive how much impact this had, theory states that including this when not needed has a detrimental impact.

I'm not entirely sure what the check is supposed to accomplish but it seems to only be applicable when rendering transparent objects during depth pre-pass or shadow rendering.

Steps to reproduce

Create any scene that includes a StandardMaterial and uses an albedo texture. Examine the shader code sent to the depth pre-pass and shadow passes.

Minimal reproduction project

n/a

@lawnjelly
Copy link
Member

lawnjelly commented May 29, 2023

Don't know about desktop, but afaik this will prevent early Z on mobile, as discard disallows lots of opaque optimizations. (ARM docs state this, and I suspect it's the same for a lot of mobile)
https://developer.arm.com/documentation/102224/0200/Early-Z

@clayjohn
Copy link
Member

In 3.x the USE_OPAQUE_PREPASS define was only added for the depth pass and only when the depth mode was set to alpha prepass.

For 4.0 we should re-instate that check and then also validate that the material has transparent enabled so it never gets included in opaque builds

@BastiaanOlij
Copy link
Contributor Author

@lawnjelly indeed, that is why I'm raising this. Worse yet, from what we could tell there was a scenario where even for a completely opaque texture, it ended up reading the assigned texture just so it could apply the discard check even if nothing ever got discarded. In most cases it should be able to completely remove the fragment shader which allows mobile GPUs to skip part of the raster all together and just do it's early Z check.

@clayjohn I could have misunderstood but from what I picked up from @QbieShay remark is that it makes sense to have this for shadows as well. Transparent materials with a texture were part of the texture is opaque, should have the opaque part of the material cast shadows.

That said, I need to investigate a little bit further but seeing we already identify if a material is transparent by checking if the shader writes to ALPHA, we could make sure this check only happens if this is the case but do this for any pass. This will mean more depth pass modes and a separate pass for opaque and transparent materials both in prepass and shadow passes.
The value of opaque_prepass_threshold will steer the logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants