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

Enable geometry fade/transparency in the Mobile renderer #91672

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eswartz
Copy link
Contributor

@eswartz eswartz commented May 7, 2024

Resolves (partially) #76774 (for Mobile).

This is mostly a copy-paste of similar changes in the Forward+ renderer, as hinted in #76774 (comment) and #87231 (review), along with a tweak to the shader and pipeline model.

The shader pipeline tweak involves distinguishing geometry instance-enabled transparency from material-enabled transparency. A material cannot support transparency on a per-instance level; a separate pipeline must be used when rendering. Originally in c571e4a this was solved along with a non-trivial refactoring to the shader and pipeline model in Forward+, which in the past few years has evolved a few more times to account for the multitude of combinations allowed there.

I opted to follow a simpler but similar sentiment to SceneShaderForwardClustered::PipelineColorPassFlags in a simpler SceneShaderForwardMobile::PipelinePasses enum, which only accounts for don't-care or opaque pipelines vs. color transparency-enabled ones. This multiplies the number of PipelineCacheRDs by two, but otherwise accomplishes the goals without adding unnecessary changes or shooting myself or others in the foot (I hope).

BTW I also reorganized and removed some redundant checks in RenderForwardClustered::_geometry_instance_add_surface_with_material (since I was comparing this side-by-side with the RenderForwardMobile version, which seems better-organized). All the checks result in the same behavior anyway.

Test project:
fade-test.zip

Master This PR
Mobile, masterl Mobile, this PR
Mobile w/lightmap, master Mobile w/lightmap, this PR
Forward, master Forward, this PR
76774-forward-lm-master Forward w/lightmap, this PR

Compatibility still shows warnings as expected:

screenshot-20240507-130238

Open issues:

  • I'm not sure what the direction is for Mobile with regard to keeping its structure similar to Forward+. I found most of the code between the two sides to be similar except for the several shader/pipeline/variant changes from @JFonS since the original geometry alpha support in 2021. My assumption is, this is intentional, since Mobile doesn't support as many combinations that warrant such a refactor.

If there are strong opinions, I can try to adapt some of the Forward+ ideas in favor of code similarly, though I would need pointers to know how to test all the combinations. (Is there some grand unified shader pipeline test project somewhere?)

This is mostly a copy-paste of similar changes in the Forward+ renderer,
as hinted in godotengine#76774 (comment) and godotengine#87231 (review),
along with a tweak to the shader and pipeline model.

The shader pipeline tweak involves distinguishing geometry instance-enabled
transparency from material-enabled transparency. A material cannot support
transparency on a per-instance level; a separate pipeline must be used when rendering.

Originally in c571e4a this was a non-trivial refactoring in Forward+
(which in the past few years has evolved a few more times to account for the multitude of combinations
allowed there).

I opted to follow a similar sentiment to `SceneShaderForwardClustered::PipelineColorPassFlags`
in a simpler `SceneShaderForwardMobile::PipelinePasses` enum, which only accounts
for don't-care or opaque pipelines vs. color transparency-enabled ones.

This multiplies the number of `PipelineCacheRD`s by two, but otherwise accomplishes
the goals without adding unnecessary changes or shooting myself or others in the foot (I hope).

BTW I also reorganized and removed some redundant checks in
`RenderForwardClustered::_geometry_instance_add_surface_with_material`
(since I was comparing this side-by-side with the `RenderForwardMobile` version, which seems
better-organized). There is no functional change since `has_alpha` is the only variable used
and includes all the checks anyway.
@clayjohn
Copy link
Member

To address your open questions, there is no need to port over the shader variants from the Forward+ renderer for this. As they are needed for features which are only present in the Forward+ renderer.

That being said, there is no reason for us to double our pipeline count, we already have a pipeline with transparency enabled, so we should be using that instead of creating a new permutation. Adding unnecessary pipelines leads to pipeline compilation stutters and once #90400 is merged, it will double the load time for shaders.

So this PR should be adapted to just use the existing transparent pipeline instead of create a new permutation

@eswartz
Copy link
Contributor Author

eswartz commented May 18, 2024

@clayjohn Thanks for taking a look. But I think you might have misunderstood some of my points. I didn't want to over-explain the reasoning in the summary, but to provide some more context:

there is no reason for us to double our pipeline count, we already have a pipeline with transparency enabled, so we should be using that instead of creating a new permutation

Yes, I believe this PR's implementation only creates new pipelines if there is geometry alpha applied while using an opaque material. The same shader is used in both and isn't recompiled.

For this feature to work, the scene shader needs to account for an opaque material/shader and a non-opaque RD::PipelineColorBlendState so a given material can be used both with opaque or transparent geometry. Mobile didn't know how to manage this mapping because it only "saw" the material alpha before. Forward+ had to be evolved along the same lines in the original feature.

So this isn't "doubling" the pipeline count except in the notion of pipeline's array dimensions (which, AFAICT in both shaders, is already mostly full of uninitialized PipelineCacheRD elements).

there is no need to port over the shader variants from the Forward+ renderer for this

I agree. I was probably a bit too oblique -- I was referring more to code structure than function.

The original Forward+-only implementation of this geometry alpha feature in c571e4a also added the notion of selecting a pipeline based on material and instance flags.

I tried to follow in spirit for Mobile. But that pipeline mapping logic was originally bundled with a significant refactoring (ShaderType -> two enums) and since then has evolved even more (now ~five), for obvious reasons.

I can tell that the Forward+ + Mobile renderers/scene shaders came from a common source and are still mostly similar. But this PR doesn't keep that similarity when it could.

Specifically, for Mobile, I added PipelineColorPassFlags but used it as a new pipelines dimension rather than e.g. splitting pipelines into color_pipelines and doing other various changes in Forward+ involving shader groups, pipeline versions, and specializations.

Though I can see how one could move closer with appropriate simplifications. But it seems it would add unnecessary risk in this PR from a rando like me.

So my question is -- did I make an appropriately simple choice? Or is it too simple? Do other contributors have opinions about the way I implemented this? Or know of similar efforts doing it the "hard" way? Or am I overthinking this? :)

and once #90400 is merged

Thanks for reminding me about this one. From looking at the changes made in common classes it and this PR, it seems it will introduce ShaderSpecializations to Mobile as well, but doesn't seem to otherwise conflict with the kinds of changes in this PR.

That said, while I believe with this PR there's still only one shader build per material, but I'm unsure how to confirm the number of shader compilations from the GUI (is there a monitor/profiler or debug verbose flag I can look out for)?

@eswartz eswartz closed this Jun 5, 2024
@Calinou Calinou removed this from the 4.3 milestone Jun 5, 2024
@Zireael07
Copy link
Contributor

Why close?

@eswartz
Copy link
Contributor Author

eswartz commented Jun 6, 2024

Oops! Sorry about that.

@eswartz eswartz reopened this Jun 6, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Jun 6, 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.

None yet

5 participants