UnrealBloomPass: Use premultipliedAlpha.
#26185
Conversation
| @@ -130,6 +130,7 @@ class UnrealBloomPass extends Pass { | |||
| vertexShader: copyShader.vertexShader, | |||
| fragmentShader: copyShader.fragmentShader, | |||
| blending: AdditiveBlending, | |||
There was a problem hiding this comment.
A so-called materialCopy that does additive blending is not copying. I think that is poorly-named.
Do you still think premultipliedAlpha is correct? I find trying to track the logic of the code in this file a bit daunting...
If you are not sure, I wonder if changing the behavior for users is worth it?
//
On a related note, I think the existing example has too much bloom effect, and looks better with this one-line change. IMO, there would be no need to change the parameters to try to match the previous look.
There was a problem hiding this comment.
A so-called materialCopy that does additive blending is not copying. I think that is poorly-named.
Agreed. E.g. materialBlend sounds more appropriate.
Do you still think premultipliedAlpha is correct?
Yes. The pass additively blends over the input. Considering how CopyPass is implemented, premultipliedAlpha should be set to true.
If you are not sure, I wonder if changing the behavior for users is worth it?
Let me think about this. That is a valid point since even without the change the effect looks "right".
On a related note, I think the existing example has too much bloom effect, and looks better with this one-line change. IMO, there would be no need to change the parameters to try to match the previous look.
Yeah, I think I got a little bit excited when UnrealBloomPass finally produces a correctly tone-mapped result in the right color space so I couldn't have enough bloom 😅 . I'm fine with turning it down a bit.
There was a problem hiding this comment.
Considering how CopyPass is implemented, premultipliedAlpha should be set to true.
We should set .premultipliedAlpha to true only if the shader outputs premultiplied RGB values. So, the question is: does it?
There was a problem hiding this comment.
Yes, I think so. The opacity scales all color components, not just alpha.
There was a problem hiding this comment.
This effect was not written to properly accommodate the premultiplied output from RenderPass.
Given this PR causes breakage for users, and given the change here sill leaves inconsistencies in the code, I'd suggest doing nothing for now.
There was a problem hiding this comment.
, and given the change here sill leaves inconsistencies in the code
On what inconsistencies are you referring to? Is there something else to fix?
There was a problem hiding this comment.
But I agree the breakage isn't worth the overall change. If someone reports a bug related to blending that can be fixed by setting the premultipliedAlpha to true, we can reconsider the PR.
There was a problem hiding this comment.
When implementing the fiddle for #24703 (comment), I've noticed that the bloom is too strong given standard values like 1 for bloom strength. It seems enabling premultipliedAlpha as suggested in the PR produces a more consistent results.
Related issue: #26179
Description
UnrealBloomPasshas to set thepremultipliedAlphaflag totruefor itsCopyShaderpass similar toSSAARenderPasssince both useAdditiveBlending(so a built-in blending function wherepremultipliedAlphaapplies).This PR mitigates the bloom effect so it is required to increase the bloom strength to achieve a comparable result like before. So this change has to be noted in the migration guide.