Skip to content

UnrealBloomPass: Use premultipliedAlpha. #26185

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

UnrealBloomPass: Use premultipliedAlpha. #26185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f128239
UnrealBloomPass: Use `premultipliedAlpha`.
Mugen87 Jun 3, 2023
3fc2509
Examples: Update screenshots.
Mugen87 Jun 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions examples/jsm/postprocessing/UnrealBloomPass.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ class UnrealBloomPass extends Pass {
vertexShader: copyShader.vertexShader,
fragmentShader: copyShader.fragmentShader,
blending: AdditiveBlending,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so. The opacity scales all color components, not just alpha.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, and given the change here sill leaves inconsistencies in the code

On what inconsistencies are you referring to? Is there something else to fix?

Copy link
Collaborator Author

@Mugen87 Mugen87 Jun 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@Mugen87 Mugen87 Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

premultipliedAlpha: true,
depthTest: false,
depthWrite: false,
transparent: true
Expand Down
Binary file modified examples/screenshots/webgl_postprocessing_unreal_bloom.jpg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified examples/screenshots/webgl_postprocessing_unreal_bloom_selective.jpg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 2 additions & 2 deletions examples/webgl_postprocessing_unreal_bloom.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@

const params = {
threshold: 0,
strength: 1.5,
strength: 5,
radius: 0,
exposure: 1
};
Expand Down Expand Up @@ -131,7 +131,7 @@

} );

bloomFolder.add( params, 'strength', 0.0, 3.0 ).onChange( function ( value ) {
bloomFolder.add( params, 'strength', 0.0, 10.0 ).onChange( function ( value ) {

bloomPass.strength = Number( value );

Expand Down
4 changes: 2 additions & 2 deletions examples/webgl_postprocessing_unreal_bloom_selective.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@

const params = {
threshold: 0,
strength: 3,
radius: 0.5,
strength: 5,
radius: 1,
exposure: 1
};

Expand Down