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

BlendModes: Added blend* prefix #29897

Merged
merged 6 commits into from
Nov 19, 2024
Merged

BlendModes: Added blend* prefix #29897

merged 6 commits into from
Nov 19, 2024

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Nov 15, 2024

Copy link

github-actions bot commented Nov 15, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.13
79
339.13
79
+0 B
+0 B
WebGPU 478.14
132.62
478.67
132.72
+528 B
+99 B
WebGPU Nodes 477.61
132.5
478.13
132.6
+528 B
+96 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 464.59
111.96
464.59
111.96
+0 B
+0 B
WebGPU 546.83
148.16
546.83
148.16
+0 B
+0 B
WebGPU Nodes 502.71
137.88
502.71
137.88
+0 B
+0 B

@sunag
Copy link
Collaborator Author

sunag commented Nov 15, 2024

Would it be better to rename blendNormal to blendColor?
blendNormals should be intended for blend geometry normals.

@Makio64
Copy link
Contributor

Makio64 commented Nov 15, 2024

Would it be better to rename blendNormal to blendColor? blendNormals should be intended for blend geometry normals.

Sorry for my lack of knowlegdes but you mean there is a way to blend the geometries normals without blending the color (output) ? Do you have a usecase in mind you can share to me to understand why peoples will do it ?

@sunag
Copy link
Collaborator Author

sunag commented Nov 15, 2024

Sorry for my lack of knowlegdes but you mean there is a way to blend the geometries normals without blending the color (output) ? Do you have a usecase in mind you can share to me to understand why peoples will do it ?

A common case is mixing two normal maps.

@WestLangley
Copy link
Collaborator

Would it be better to rename blendNormal to blendColor?

Yes, rename it. The question remains as to what the name should be...

The formula looks similar to the Porter Duff over operator, but not exactly.

@Mugen87 Is it an SSR hack of some sort, and therefore specific to that particular algorithm?

//

Be aware that the output of a beauty pass has alpha premultiplied by default. Be careful when using these formulas.

These blend modes only make sense if you know the context. I think the file should have specific references for the formulas.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 15, 2024

@Mugen87 Is it an SSR hack of some sort, and therefore specific to that particular algorithm?

The previous SSRPass used a separate render pass via copy shader to blend the SSR over the beauty via normal blending. Via blendColor() we achieve the same result as before but without a separate pass.

Right now, the blend function is not specific for SSRNode. It should work for similar use cases which relied on normal blending as well.

The premultiply alpha topic might be relevant in the future. If a use case comes up, we probably need a third parameter premultipliedAlpha with an additional code path.

@WestLangley
Copy link
Collaborator

@Mugen87

BlendModes.js contains some blending functions -- from somewhere -- maybe @sunag knows where they are from. I am curious to know...

It should work for similar use cases which relied on normal blending as well.

The function you added to the file in #29879 does not match the three.js normal blending formula. Also, the proposed name blendColor() does not have a well-defined meaning.

So, currently, I would be inclined to revert #29879, and inline the function in the SSR shader.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 16, 2024

The function you added to the file in #29879 does not match the three.js normal blending formula.

Do you mind elaborating the differences and why we still get the same visual result?

@sunag
Copy link
Collaborator Author

sunag commented Nov 16, 2024

I don't remember exactly all links, because I was looking for blend modes that used mix instead of conditionals like this:
https://github.com/Experience-Monks/glsl-blend-overlay/blob/master/index.glsl

then validated them in the image editor and got the expected result.

It looks the "same" comparing to the reference below, except that our version returns a vec4() with the base opacity and has a opacity as parameter, I don't think the order of addition should change the result here.

https://github.com/jamieowen/glsl-blend/blob/master/normal.glsl

@sunag sunag added this to the r171 milestone Nov 16, 2024
@sunag sunag marked this pull request as ready for review November 16, 2024 20:37
@WestLangley
Copy link
Collaborator

Do you mind elaborating the differences and why we still get the same visual result?

Please refer to the Porter Duff over blending operator explained in this Wikipedia article.

  1. The original blending formula assumes the source, destination, and output colors are non-premultiplied. Here is how those formulas are written in the case where the source is the shader output and the destination is the drawing buffer -- both non-premultiplied.
dst.rgb = src.rgb * src.a + dst.rgb * dst.a * ( 1 - src.a )
dst.a   = src.a   * 1     + dst.a           * ( 1 - src.a )
dst.rgb = dst.rgb / dst.a
  1. Later in that article, you will find the formula to use when the source, destination, and output colors are premultiplied. This formula is akin to the three.js NormalBlending formula when material.premultipliedAlpha is true.
dst.rgb = src.rgb * 1 + dst.rgb * ( 1 - src.a )
dst.a   = src.a   * 1 + dst.a   * ( 1 - src.a )
  1. Not mentioned in the article is the formula used in three.js for NormalBlending when the source (output of the shader) has non-premultiplied alpha and the destination has premultiplied alpha.
dst.rgb = src.rgb * src.a + dst.rgb * ( 1 - src.a )
dst.a   = src.a   * 1     + dst.a   * ( 1 - src.a )

So, the proper formula to use depends on the context.

You got the "same answer" because ssr() ignores the alpha channel of the beauty pass, and assumes alpha is 1. Plus, your test case was likely an opaque one.

//

You have promoted a shader-specific blending formula to a more general status, but the formula is only correct in certain cases. The context is important. Also, the proposed name blendColor() does not have a well-defined meaning. This is why I suggested in-lining the formula in the shader, and not promoting it.

@WestLangley
Copy link
Collaborator

In the renderer's constructor, we set premultipliedAlpha to true. This is a flag to the compositor that the contents of the drawing buffer have premultiplied alpha.

When working with WebGPURederer and TSL, it is important to remember that the input to postprocessing (the output of the beauty pass) will have premultiplied alpha by default. And so should the final output of postprocessing, because that is what the compositor is expecting.

@sunag
Copy link
Collaborator Author

sunag commented Nov 19, 2024

I think the problem is in how these formulas are handling opacity, for general use it is better to use the formula that @WestLangley mentioned (1.), because the SSR blending return a vec4() with base.a and it would be ignoring the intermediate opacity that blend.a could add (It looks like this is a small optimization of instructions for specific SSR usage). All other blending modes are ignoring opacity at the moment, but it is very likely that we will need an opacity setting option in the future that works in all blending modes.

@sunag sunag merged commit 405ad7b into mrdoob:dev Nov 19, 2024
12 checks passed
@sunag sunag deleted the dev-blends branch November 19, 2024 02:56
@Mugen87 Mugen87 mentioned this pull request Nov 20, 2024
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.

4 participants