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

Show warning for GeometryInstance3D transparency in Mobile/Compatibility #87231

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jan 15, 2024

This feature is currently only supported when using Forward+.

@Calinou Calinou requested review from a team as code owners January 15, 2024 19:34
@Calinou Calinou added this to the 4.3 milestone Jan 15, 2024
This feature is currently only supported when using Forward+.
@Calinou Calinou force-pushed the geometryinstance3d-transparency-warn-mobile-compatibility branch from d038e3f to a9893e2 Compare January 15, 2024 19:51
@eswartz
Copy link
Contributor

eswartz commented May 3, 2024

I'd like to see this in 4.3 please :) I ran into this yet again the other day (in the guise of fade distance).

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.

I think it would be quite easy to add this to both renderers. The code can be mostly copy-pasted from the Forward+ renderer, but let's add this for now to avoid confusing people

@akien-mga akien-mga merged commit 471ddf4 into godotengine:master May 4, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Calinou Calinou deleted the geometryinstance3d-transparency-warn-mobile-compatibility branch May 7, 2024 00:06
eswartz added a commit to eswartz/godot that referenced this pull request May 7, 2024
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.
eswartz added a commit to eswartz/godot that referenced this pull request May 28, 2024
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.
eswartz added a commit to eswartz/godot that referenced this pull request Jul 11, 2024
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.
eswartz added a commit to eswartz/godot that referenced this pull request Jul 24, 2024
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.
eswartz added a commit to eswartz/godot that referenced this pull request Jul 29, 2024
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.
eswartz added a commit to eswartz/godot that referenced this pull request Jul 29, 2024
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.
eswartz added a commit to eswartz/godot that referenced this pull request Aug 13, 2024
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.
eswartz added a commit to eswartz/godot that referenced this pull request Aug 24, 2024
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.
eswartz added a commit to eswartz/godot that referenced this pull request Aug 25, 2024
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.
eswartz added a commit to eswartz/godot that referenced this pull request Aug 25, 2024
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.
eswartz added a commit to eswartz/godot that referenced this pull request Aug 28, 2024
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.
eswartz added a commit to eswartz/godot that referenced this pull request Sep 19, 2024
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.
eswartz added a commit to eswartz/godot that referenced this pull request Oct 27, 2024
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.
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