-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
[Merged by Bors] - Standard Material Blend Modes #6644
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
Changes from 17 commits
7141282
bbb3420
e2a0c46
e1b54dd
63b7844
4efa604
0562f46
92ca5bd
b34341f
fc33691
295d8d2
4187b15
c8f914c
95aad7e
2c9e22a
7d9989e
a3810fd
d538109
fe052e7
b2196b1
e035576
071a4c6
1ee663a
54591c8
28bfb45
5a14a7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -384,8 +384,14 @@ pub fn queue_material_meshes<M: Material>( | |
| MeshPipelineKey::from_primitive_topology(mesh.primitive_topology) | ||
| | view_key; | ||
| let alpha_mode = material.properties.alpha_mode; | ||
| if let AlphaMode::Blend = alpha_mode { | ||
| mesh_key |= MeshPipelineKey::TRANSPARENT_MAIN_PASS; | ||
| if let AlphaMode::Blend | AlphaMode::Premultiplied | AlphaMode::Add = | ||
| alpha_mode | ||
|
Comment on lines
+418
to
+419
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This syntax is... interesting. I guess we are e certain it works? As in, this will match if alpha mode is one of blend, premultiplied, or add.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, just double checked it, and it does work as expected I didn't think much of it as I was making this change, since I just reused the existing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree it’s more concise. I think the mental hiccup for me is because the flags can be combined using the | or operator. But I don’t mind either way. This is fine. |
||
| { | ||
| // Blend, Premultiplied and Add all share the same pipeline key | ||
| // They're made distinct in the PBR shader, via `premultiply_alpha()` | ||
| mesh_key |= MeshPipelineKey::BLEND_PREMULTIPLIED_ALPHA; | ||
| } else if let AlphaMode::Multiply = alpha_mode { | ||
| mesh_key |= MeshPipelineKey::BLEND_MULTIPLY; | ||
| } | ||
|
|
||
| let pipeline_id = pipelines.specialize( | ||
|
|
@@ -424,7 +430,10 @@ pub fn queue_material_meshes<M: Material>( | |
| distance, | ||
| }); | ||
| } | ||
| AlphaMode::Blend => { | ||
| AlphaMode::Blend | ||
| | AlphaMode::Premultiplied | ||
| | AlphaMode::Add | ||
| | AlphaMode::Multiply => { | ||
| transparent_phase.add(Transparent3d { | ||
| entity: *visible_entity, | ||
| draw_function: draw_transparent_pbr, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -515,10 +515,13 @@ bitflags::bitflags! { | |||||
| /// MSAA uses the highest 3 bits for the MSAA log2(sample count) to support up to 128x MSAA. | ||||||
| pub struct MeshPipelineKey: u32 { | ||||||
| const NONE = 0; | ||||||
| const TRANSPARENT_MAIN_PASS = (1 << 0); | ||||||
| const HDR = (1 << 1); | ||||||
| const TONEMAP_IN_SHADER = (1 << 2); | ||||||
| const DEBAND_DITHER = (1 << 3); | ||||||
| const BLEND_RESERVED_BITS = 0b11; // ← Bitmask reserving two bits for the blend state | ||||||
| const BLEND_OPAQUE = 0; // ← Values are just sequential within the mask, and can range from 0 to 3 | ||||||
| const BLEND_PREMULTIPLIED_ALPHA = 1; // | ||||||
| const BLEND_MULTIPLY = 2; // ← We still have room for one more value without adding more bits | ||||||
coreh marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| const HDR = (1 << 2); | ||||||
| const TONEMAP_IN_SHADER = (1 << 3); | ||||||
| const DEBAND_DITHER = (1 << 4); | ||||||
| const MSAA_RESERVED_BITS = Self::MSAA_MASK_BITS << Self::MSAA_SHIFT_BITS; | ||||||
| const PRIMITIVE_TOPOLOGY_RESERVED_BITS = Self::PRIMITIVE_TOPOLOGY_MASK_BITS << Self::PRIMITIVE_TOPOLOGY_SHIFT_BITS; | ||||||
| } | ||||||
|
|
@@ -620,12 +623,30 @@ impl SpecializedMeshPipeline for MeshPipeline { | |||||
| let vertex_buffer_layout = layout.get_layout(&vertex_attributes)?; | ||||||
|
|
||||||
| let (label, blend, depth_write_enabled); | ||||||
| if key.contains(MeshPipelineKey::TRANSPARENT_MAIN_PASS) { | ||||||
| label = "transparent_mesh_pipeline".into(); | ||||||
| blend = Some(BlendState::ALPHA_BLENDING); | ||||||
| let pass = key.intersection(MeshPipelineKey::BLEND_RESERVED_BITS); | ||||||
| if pass == MeshPipelineKey::BLEND_PREMULTIPLIED_ALPHA { | ||||||
| label = "premultiplied_alpha_mesh_pipeline".into(); | ||||||
| blend = Some(BlendState::PREMULTIPLIED_ALPHA_BLENDING); | ||||||
| shader_defs.push("PREMULTIPLY_ALPHA".to_string()); | ||||||
| shader_defs.push("BLEND_PREMULTIPLIED_ALPHA".to_string()); | ||||||
| // For the transparent pass, fragments that are closer will be alpha blended | ||||||
| // but their depth is not written to the depth buffer | ||||||
| depth_write_enabled = false; | ||||||
| } else if pass == MeshPipelineKey::BLEND_MULTIPLY { | ||||||
| label = "multiply_mesh_pipeline".into(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can change this, but it will be slightly inconsistent with the other two, that do match 1:1 with the key name, e.g.
Let me know what you prefer
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
EDIT: No, that’s something else.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stick with multiply. |
||||||
| blend = Some(BlendState { | ||||||
| color: BlendComponent { | ||||||
| src_factor: BlendFactor::Dst, | ||||||
| dst_factor: BlendFactor::OneMinusSrcAlpha, | ||||||
coreh marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| operation: BlendOperation::Add, | ||||||
| }, | ||||||
| alpha: BlendComponent::OVER, | ||||||
| }); | ||||||
| shader_defs.push("PREMULTIPLY_ALPHA".to_string()); | ||||||
| shader_defs.push("BLEND_MULTIPLY".to_string()); | ||||||
| // For the multiply pass, fragments that are closer will be alpha blended | ||||||
| // but their depth is not written to the depth buffer | ||||||
| depth_write_enabled = false; | ||||||
| } else { | ||||||
| label = "opaque_mesh_pipeline".into(); | ||||||
| blend = Some(BlendState::REPLACE); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,10 +7,11 @@ | |||||
|
|
||||||
| fn alpha_discard(material: StandardMaterial, output_color: vec4<f32>) -> vec4<f32>{ | ||||||
| var color = output_color; | ||||||
| if ((material.flags & STANDARD_MATERIAL_FLAGS_ALPHA_MODE_OPAQUE) != 0u) { | ||||||
| let alpha_mode = (material.flags & STANDARD_MATERIAL_FLAGS_ALPHA_MODE_RESERVED_BITS); | ||||||
| if (alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_OPAQUE) { | ||||||
| // NOTE: If rendering as opaque, alpha should be ignored so set to 1.0 | ||||||
| color.a = 1.0; | ||||||
| } else if ((material.flags & STANDARD_MATERIAL_FLAGS_ALPHA_MODE_MASK) != 0u) { | ||||||
| } else if (alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_MASK) { | ||||||
| if (color.a >= material.alpha_cutoff) { | ||||||
| // NOTE: If rendering as masked alpha and >= the cutoff, render as fully opaque | ||||||
| color.a = 1.0; | ||||||
|
|
@@ -268,3 +269,66 @@ fn dither(color: vec4<f32>, pos: vec2<f32>) -> vec4<f32> { | |||||
| } | ||||||
| #endif | ||||||
|
|
||||||
| #ifdef PREMULTIPLY_ALPHA | ||||||
| fn premultiply_alpha(standard_material_flags: u32, color: vec4<f32>) -> vec4<f32> { | ||||||
coreh marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| // `Blend`, `Premultiplied` and `Alpha` all share the same `BlendState`. Depending | ||||||
| // on the alpha mode, we premultiply the color channels by the alpha channel value, | ||||||
| // (and also optionally replace the alpha value with 0.0) so that the result produces | ||||||
| // the desired blend mode when sent to the blending operation. | ||||||
| #ifdef BLEND_PREMULTIPLIED_ALPHA | ||||||
| // For `BlendState::PREMULTIPLIED_ALPHA_BLENDING` the blend function is: | ||||||
| // | ||||||
| // result = 1 * src_color + (1 - src_alpha) * dst_color | ||||||
| let alpha_mode = standard_material_flags & STANDARD_MATERIAL_FLAGS_ALPHA_MODE_RESERVED_BITS; | ||||||
| if (alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_BLEND) { | ||||||
coreh marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| // Here, we premultiply `src_color` by `src_alpha` (ahead of time, here in the shader) | ||||||
| // | ||||||
| // src_color *= src_alpha | ||||||
| // | ||||||
| // We end up with: | ||||||
| // | ||||||
| // result = 1 * (src_alpha * src_color) + (1 - src_alpha) * dst_color | ||||||
| // result = src_alpha * src_color + (1 - src_alpha) * dst_color | ||||||
| // | ||||||
| // Which is the blend operation for regular alpha blending `BlendState::ALPHA_BLENDING` | ||||||
| return vec4<f32>(color.rgb * color.a, color.a); | ||||||
coreh marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we risk losing precision here, compared to returning unmultiplied and using I don't really see a reason to change this path though, we could just not push
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the nvidia paper https://developer.nvidia.com/content/alpha-blending-pre-or-not-pre from comments above gives a good reason for the change: it behaves much better with mipmapping.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's been a while since I wrote this PR; Consolidating additive, premultiplied and regular alpha blending into a single pass was based off some early feedback. I don't recall the exact reasoning, but looking at the code, and if I understand it correctly, the benefits I see are:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That was entirely coincidental, at least on my part! 😆 I'm trying to find who originally suggested consolidating, maybe they had that in mind as well?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Being able to use the same pipeline for more things is good as it allows us to batch multiple draw commands into one. |
||||||
| } if (alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_ADD) { | ||||||
|
||||||
| } if (alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_ADD) { | |
| } else if (alpha_mode == STANDARD_MATERIAL_FLAGS_ALPHA_MODE_ADD) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. The lack of something like rustfmt for wgsl really makes me let a bunch of weird formatting errors slip by 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by fixed RGB values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah that came out confusing. Changed it to “For otherwise constant RGB values...”