Skip to content
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

CopyPass: Assume premultipliedAlpha is set to true. #24277

Closed
wants to merge 3 commits into from

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jun 27, 2022

Fixed #19531.

Description

Reverts #23671 and ensures copy materials always set premultipliedAlpha to true. The PR also introduces CopyPass for an easier usage on app level.

Motivation of this change is outlined here: #19531 (comment)

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 27, 2022

@Mugen87 Mugen87 requested a review from WestLangley June 27, 2022 14:29
@@ -105,7 +105,8 @@ class OutlinePass extends Pass {
blending: NoBlending,
depthTest: false,
depthWrite: false,
transparent: true
transparent: true,
premultipliedAlpha: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Setting NoBlending and premultipliedAlpha: true and transparent: true at the same time?

Copy link
Collaborator

@WestLangley WestLangley Jun 27, 2022

Choose a reason for hiding this comment

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

@Mugen87 I appreciate your desire to work on this. I just think it is going to take a lot of thought to get opacity properly-supported in EffectComposer.

The transparent flag is for user convenience, in lieu of having to set the blending. I am not sure it is wise to be setting both. ( Is the transparent flag needed for the transparent rendering group? ).

Is premultipliedAlpha even supported with NoBlending?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Setting NoBlending and premultipliedAlpha: true and transparent: true at the same time?

My point is: Since the shader implementation assumes premultiplied alpha, I would suggest to always set premultipliedAlpha to true for clarity.

Whether or not all combinations of transparent and blending make sense is something I would tackle in a different issue/PR.

Copy link
Collaborator

@WestLangley WestLangley Jun 27, 2022

Choose a reason for hiding this comment

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

Since the shader implementation assumes premultiplied alpha

The drawing buffer has premultiplied alpha, but the output of the shader may or may not have it.

You only need to set material.premultipliedAlpha if it is required to get the correct answer. Setting it when it is not needed just causes more confusion.

.premultipliedAlpha is a flag, just like transparent, to guide the renderer in setting the correct blending function. It only applies to the built-in blending functions.

.premultipliedAlpha means the output of the shader has RGB multiplied by alpha. In that case, it triggers the renderer to select a blending function different from the typical one.

.premultipliedAlpha does not apply when NoBlending is set.

To understand these concepts, I'd start with understanding the setBlending() function in WebGLState.js.

Copy link
Collaborator Author

@Mugen87 Mugen87 Jun 28, 2022

Choose a reason for hiding this comment

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

I'm aware how premultipliedAlpha is used in WebGLState. I'm just the opinion that less errors occur if premultipliedAlpha is always set to true for this particular case. Especially since nothing breaks even when no alpha blending is used.

In other words: We should not expect that users enable premultipliedAlpha if they blend the result of a copy pass. webgl_postprocessing_backgrounds shows that even examples don't do this right.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 1, 2023

Closing in favor of #26179.

@Mugen87 Mugen87 closed this Jun 1, 2023
@Mugen87 Mugen87 mentioned this pull request Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CopyShader opacity does not work as expected
3 participants