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

CopyShader opacity does not work as expected #19531

Closed
3 of 13 tasks
generesque opened this issue Jun 3, 2020 · 6 comments · Fixed by #23671 or #26179
Closed
3 of 13 tasks

CopyShader opacity does not work as expected #19531

generesque opened this issue Jun 3, 2020 · 6 comments · Fixed by #23671 or #26179

Comments

@generesque
Copy link

generesque commented Jun 3, 2020

Description of the problem

In examples/js/shaders/CopyShader.js, there is an issue with the fragment shader:

               "void main() {",

		"	vec4 texel = texture2D( tDiffuse, vUv );",
		"	gl_FragColor = opacity * texel;",

		"}"

Because texel is multiplied by opacity directly, all elements of the color are scaled by the opacity value. This means on a black background, the opacity is effectively applied twice. On any other background, the resulting color will end up darker than expect.

Proposed Solution:

Multiply only the alpha component by opacity:

		"void main() {",

		"	vec4 texel = texture2D( tDiffuse, vUv );",
		"	texel.a = opacity * texel.a;",
		"	gl_FragColor = texel;",

		"}"
Three.js version
  • Dev
  • r117
  • ...
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • macOS
  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)
@WestLangley
Copy link
Collaborator

Yes, likely.

One needs to check the three.js examples where this is used -- used both directly and indirectly.

Check BlendShader.js, too.

@WestLangley
Copy link
Collaborator

I have second thoughts on this...

I trust the original author knew what he was doing back in 2011.

CopyShader was designed to copy render target textures, which by default, have premultiplied alpha.

If you want to further scale the opacity, you need to scale all four components.

I would be inclined to revert #23671, and look elsewhere if there are issues.

@WestLangley WestLangley reopened this Jun 25, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 26, 2022

I would be inclined to revert #23671, and look elsewhere if there are issues.

Yes, it seems better to revert the PR. That means however shader materials using CopyShader should set their premultipliedAlpha property to true. Doing this e.g. in TexturePass fixes the issue mentioned in #23671 (comment).

this.material = new ShaderMaterial( {
uniforms: this.uniforms,
vertexShader: shader.vertexShader,
fragmentShader: shader.fragmentShader,
depthTest: false,
depthWrite: false
} );

I can file a PR with updating the respective example code.

@WestLangley
Copy link
Collaborator

I am not sure that is correct. Maybe no-blending.

  1. Every post-processing pass needs to have the blending function set properly. I would not use the transparent flag -- just set the blending function directly.

  2. I expect some of these effects were written assuming no alpha channel. They need to be checked to makes sure the alpha channel is handled properly.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 27, 2022

The problem is that webgl_postprocessing_backgrounds blends a textured background with whatever is behind it. Not setting premultipliedAlpha to true with the previous version of CopyShader produces a too dark image.

What is the downside of always setting premultipliedAlpha to true when CopyShader is used? Like in the previous version of SSAARenderPass.

@WestLangley
Copy link
Collaborator

You can try. There have been many piecemeal attempts at getting post-processing working correctly with opacity < 1.

So far, it's always been one more thing to try to fix...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment