-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
[3.x] Add blend_premul_alpha support to scene shaders. #36747
base: 3.x
Are you sure you want to change the base?
Conversation
Ah, interesting, compiler settings for the checks are stricter than what was used compiling locally. The blend modes enum is shared for the detail blend mode (which is not impacted by this). Also, seems the pull request took all my changes in that branch, so I need to rebase this, I guess? Still figuring my way around git. |
8eb193d
to
69c1805
Compare
The 5 commits must be squashed together before this can be merged. See PR workflow for instructions 🙂 |
89fd68e
to
b35220b
Compare
Ok, I tried to do that, but now I somehow have 2 mystery changelists that ended up in this branch that I didn't make. Time to re-rebase. |
268db98
to
dc47944
Compare
This PR comes in handy for me because I'll be able to apply it to billboarded sprites, so thank you for that. But I also wanted to point out that this will create issues when used with the default spatial shader. For example:
Multiplying rgb by alpha at https://github.com/godotengine/godot/blob/3.2/drivers/gles3/shaders/scene.glsl#L2181 is an approximate fix for that issue, but still leaves some differences in shading. Also, I'm not sure that 'fix' is compatible with textures with premultiplied alpha, since the rgb components would end up being multiplied twice by alpha. |
Hmm, this seems to work for me. This is the shader code I ended up with:
|
I got it a bit wrong, the premultiplied mesh doesn't stay opaque, but there's a difference in shading. Here's a test scene where the materials on each sphere only differ by blend mode:
On second thoughts, this is the right fix. The differences in shading would go away once it's only applied to shaders using pre-multiplied blending - doh. |
But I think the albedo component from textures should probably be excluded from being multiplied for the reason mentioned. Perhaps double-check with someone else before putting in extra work - the lighting model doesn't adapt to other blend modes either, so maybe it's ok like this. |
The whole point of premul alpha is that you have to premultiply the alpha so you can control the visual behavior (ex: have some sort of hybrid between additive blending and alpha blending). You're suggesting that I multiply the alpha, which would make it just a normal alpha blend. |
Agreed, but the lighting model in the default shader outputs with regular alpha. So you'll get your texture with pre-multiplied alpha rendering correctly, but with potentially weird lighting, unless you go with the unshaded flag. That might be fine for now, since working around it introduces complexity. It's probably best if someone from the engine team reviews the current PR, I'm just another user. |
Ah, I see what you're saying, now. I was using it for things like lasers, unshaded. Wonder if there's a case where you'd want this lit in practice. In any case, this is better than nothing. We can iterate as needed. |
Something like this is commonly used for rendering fire + smoke in one pass. This of course causes issues if the material is not unshaded. This would allow us to have a material that has an alpha-blended component that is lit, and an emissive component. A better name for that would maybe be blend_mix_add. |
This was automatically switched to 3.x, so I don't need to do anything, right? |
Yeah it's all good, though you might want to rebase on latest @godotengine/rendering WDYT about the feature? It should be suitable for 3.4 if deemed useful. |
Sounds good to me. Not only it's useful for various special effects in particles, it can also be used to avoid issues with alpha edges on 3D sprites. This should also be forward-ported to |
I kind of feel like it's a bug or incomplete feature that blend_premul_alpha exists for 2D, but not 3D, but I went ahead and created a proposal here: godotengine/godot-proposals#3431 |
You probably also want to add in Do you have a test project for 3.x ? Seems to compile and the code above seems to work but I don't know exactly what to test for. |
71cb8d3
to
c58391c
Compare
bb7dc55
to
97821b6
Compare
Ok, finally got around to updating this. I've added the functionality to non-shader spatial materials as well, as suggested by @ghost. Included a test project here:
Premul alpha is pretty much how all professional CG/VFX compositing is done, so I think it's a pretty important feature to have. Lets you combine solid and glowy things in one pass. |
scene/resources/material.cpp
Outdated
@@ -1036,6 +1039,9 @@ void SpatialMaterial::_update_shader() { | |||
case BLEND_MODE_MUL: { | |||
code += "\tvec3 detail = mix(ALBEDO.rgb,ALBEDO.rgb*detail_tex.rgb,detail_tex.a);\n"; | |||
} break; | |||
case BLEND_MODE_PMALPHA: { | |||
// Not used for detail blend modes, but strict compiler rules cause errors if this case is not handled. |
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.
If this situation shouldn't be possible, maybe we can print a warning for this case, so it will get noticed if it ever goes wrong.
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.
Its not that it isn't possible, its just that the compiler insists that every case be represented in a switch statement. So this line of code tells the reader that we intentionally choose not to insert any code in the premul alpha case
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.
Technically, you could use premultiplied alpha for as a detail map blend mode, but it probably doesn't make much sense and wouldn't be used often.
97821b6
to
68c8521
Compare
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.
Am I right in thinking that this mode requires the user to either use textures with pre-multiplied alpha or pre-multiply manually in the shader? If so, we need to accompany this with a documentation change to clearly say that. I hesitate with features that require either a deep background knowledge and that don't just work when enabled. If shader changes are required I am concerned that we will get a flood of users asking why the feature doesn't just work when enabled.
On that note, docs need to be regenerated and filled in to pass CI how to do so is described here https://docs.godotengine.org/en/stable/community/contributing/updating_the_class_reference.html
glBlendFuncSeparate(GL_ONE, GL_ONE_MINUS_SRC_ALPHA, GL_ONE, GL_ONE_MINUS_SRC_ALPHA); | ||
} else { | ||
glBlendFunc(GL_ONE, GL_ONE_MINUS_SRC_ALPHA); |
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.
These are functionally equivalent. In either case source is multiplied by GL_ONE and destination is multiplied by GL_ONE_MINUS_SRC_ALPHA
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.
That's what I thought, but I wasn't 100% sure, so I did it like this to be safe.
scene/resources/material.cpp
Outdated
@@ -1036,6 +1039,9 @@ void SpatialMaterial::_update_shader() { | |||
case BLEND_MODE_MUL: { | |||
code += "\tvec3 detail = mix(ALBEDO.rgb,ALBEDO.rgb*detail_tex.rgb,detail_tex.a);\n"; | |||
} break; | |||
case BLEND_MODE_PMALPHA: { | |||
// Not used for detail blend modes, but strict compiler rules cause errors if this case is not handled. |
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.
Its not that it isn't possible, its just that the compiler insists that every case be represented in a switch statement. So this line of code tells the reader that we intentionally choose not to insert any code in the premul alpha case
Correct.
This is already a feature of Godot, but it's for 2D only. I'm simply bringing it to 3D for consistency (and it's a very useful feature for FX). It's just another blend mode, so it should more or less "just work". Not terribly complicated. If people aren't using it with premul alpha textures, it'll look like additive blend mode or mix blend mode (or somewhere in between). I'll add documentation as well. Also, I need this in Godot 4 for explosions, so I'll probably do another PR for that... I was going to say soon, but whenever that happens. :D |
68c8521
to
a58bdbb
Compare
Updated PR to add documentation. I was going to go ahead and add support for premul alpha in the detail blending, but then realized that it would consume a bit in the MaterialKey, so I opted not to do that as there are only 5 bits left, and I doubt anybody would actually use this, so better to save them for more practical features. |
Here's the Godot Engine 4 version: #68548 |
#85609 was merged and I think @lawnjelly we can close as the pr doesn't seem to be enough interest, but it's salvageable. |
I do suspect if decisions have (finally 🙂 ) been made and #85609 was merged in 4.x, then that means an easy port or modification of this PR is more easily justifiable, so it might be good to leave this option open imo, unless someone else on rendering has different opinion. Usually small backports are quite easy to justify. |
This will probably be difficult to backport since the file structure for rendering stuff completely changed. I imagine it'd be easier to just tweak this CL. Can't remember what all was different between it and the 4.x version offhand. |
This was previously only supported by 2D shaders, but can be pretty useful in 3D as well. For example, I'm using it for lasers where I don't want to use pure additive blending, because they lose visibility with bright backgrounds, but a normal alpha blend doesn't look good, either, so I make them mostly additive with some slight alpha to ensure visibility on brighter areas.
I'm making this pull request directly into 3.2 because the gles3 renderer was removed and I can't make the pull request in master and have it merged back to 3.2. (Also, can't compile master due to some internal compiler error, so I'll have to figure that out and make a different pull request for master to support vulkan after I figure that out, if this goes through).
Bugsquad edit: This addresses godotengine/godot-proposals#3431 for
3.x
.