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

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

BlendModes: Added blend* prefix #29897

wants to merge 5 commits into from

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.37
79.06
339.37
79.06
+0 B
+0 B
WebGPU 478.22
132.63
478.68
132.71
+464 B
+77 B
WebGPU Nodes 477.68
132.51
478.15
132.61
+464 B
+97 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 464.82
112.02
464.82
112.02
+0 B
+0 B
WebGPU 546.95
148.2
546.95
148.2
+0 B
+0 B
WebGPU Nodes 502.83
137.91
502.83
137.91
+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
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