-
-
Notifications
You must be signed in to change notification settings - Fork 21.2k
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
Add premult alpha blending to 3D (spatial) shaders #85609
Changes from all commits
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 | ||
---|---|---|---|---|
|
@@ -688,6 +688,9 @@ void BaseMaterial3D::_update_shader() { | |||
case BLEND_MODE_MUL: | ||||
code += "blend_mul"; | ||||
break; | ||||
case BLEND_MODE_PREMULT_ALPHA: | ||||
code += "blend_premul_alpha"; | ||||
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 merged a bit too fast, this one is still wrong. 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. So it seems like we had this Should we fix it in Spatial shaders and accept that we have an inconsistency in 2D 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 noticed that too, but I'd rather not start touching things elsewhere for now 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 mean this one is a bug, it's writing So we have two options:
And yes to be clear we shouldn't change 2D shaders, that would break compat. 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 didn't remember either, and frankly I wonder if I did it on purpose or not. I'm not sure if we want to keep consistent with 2d and generally inconsistent, or if we should have correct 3d and weird 2D. Is there any precedent we can reference to take the decision? |
||||
break; | ||||
case BLEND_MODE_MAX: | ||||
break; // Internal value, skip. | ||||
} | ||||
|
@@ -1819,6 +1822,11 @@ void fragment() {)"; | |||
vec3 detail = mix(ALBEDO.rgb, ALBEDO.rgb * detail_tex.rgb, detail_tex.a); | ||||
)"; | ||||
} break; | ||||
case BLEND_MODE_PREMULT_ALPHA: { | ||||
// This is unlikely to ever be used for detail textures, and in order for it to function in the editor, another bit must be used in MaterialKey, | ||||
// but there are only 5 bits left, so I'm going to leave this disabled unless it's actually requested. | ||||
//code += "\tvec3 detail = (1.0-detail_tex.a)*ALBEDO.rgb+detail_tex.rgb;\n"; | ||||
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. @QbieShay I don't understand why this is commented out. Before the nuance is lost to the sands of time can you expand a little more? 5 bits left where? godot/scene/resources/material.h Line 326 in f91db3d
The bits as far as I can tell are already spent allocating 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. Ah, that was probably taken from my original PR which didn't have the bits taken. I was hesitant to waste a bit for this, but if the bit is already taken, this should be uncommented. |
||||
} break; | ||||
case BLEND_MODE_MAX: | ||||
break; // Internal value, skip. | ||||
} | ||||
|
@@ -3040,7 +3048,7 @@ void BaseMaterial3D::_bind_methods() { | |||
ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "alpha_hash_scale", PROPERTY_HINT_RANGE, "0,2,0.01"), "set_alpha_hash_scale", "get_alpha_hash_scale"); | ||||
ADD_PROPERTY(PropertyInfo(Variant::INT, "alpha_antialiasing_mode", PROPERTY_HINT_ENUM, "Disabled,Alpha Edge Blend,Alpha Edge Clip"), "set_alpha_antialiasing", "get_alpha_antialiasing"); | ||||
ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "alpha_antialiasing_edge", PROPERTY_HINT_RANGE, "0,1,0.01"), "set_alpha_antialiasing_edge", "get_alpha_antialiasing_edge"); | ||||
ADD_PROPERTY(PropertyInfo(Variant::INT, "blend_mode", PROPERTY_HINT_ENUM, "Mix,Add,Subtract,Multiply"), "set_blend_mode", "get_blend_mode"); | ||||
ADD_PROPERTY(PropertyInfo(Variant::INT, "blend_mode", PROPERTY_HINT_ENUM, "Mix,Add,Subtract,Multiply,Premultiplied Alpha"), "set_blend_mode", "get_blend_mode"); | ||||
ADD_PROPERTY(PropertyInfo(Variant::INT, "cull_mode", PROPERTY_HINT_ENUM, "Back,Front,Disabled"), "set_cull_mode", "get_cull_mode"); | ||||
ADD_PROPERTY(PropertyInfo(Variant::INT, "depth_draw_mode", PROPERTY_HINT_ENUM, "Opaque Only,Always,Never"), "set_depth_draw_mode", "get_depth_draw_mode"); | ||||
ADD_PROPERTYI(PropertyInfo(Variant::BOOL, "no_depth_test"), "set_flag", "get_flag", FLAG_DISABLE_DEPTH_TEST); | ||||
|
@@ -3269,6 +3277,7 @@ void BaseMaterial3D::_bind_methods() { | |||
BIND_ENUM_CONSTANT(BLEND_MODE_ADD); | ||||
BIND_ENUM_CONSTANT(BLEND_MODE_SUB); | ||||
BIND_ENUM_CONSTANT(BLEND_MODE_MUL); | ||||
BIND_ENUM_CONSTANT(BLEND_MODE_PREMULT_ALPHA); | ||||
|
||||
BIND_ENUM_CONSTANT(ALPHA_ANTIALIASING_OFF); | ||||
BIND_ENUM_CONSTANT(ALPHA_ANTIALIASING_ALPHA_TO_COVERAGE); | ||||
|
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.
Should be
[code]PREMULT_ALPHA_FACTOR[/code]
.