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

ShaderChunk: Modulate transmission.a with transmissionFactor #22473

Merged
merged 1 commit into from
Aug 31, 2021

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Aug 31, 2021

Related issue: #22425

Description

Before After
Screen Shot 2021-08-31 at 7 49 48 PM Screen Shot 2021-08-31 at 7 50 01 PM

/cc @elalish

@mrdoob mrdoob added this to the r133 milestone Aug 31, 2021
@@ -27,6 +27,6 @@ export default /* glsl */`
attenuationTint, attenuationDistance );

totalDiffuse = mix( totalDiffuse, transmission.rgb, transmissionFactor );
transmissionAlpha = transmission.a;
transmissionAlpha = mix( transmissionAlpha, transmission.a, transmissionFactor );
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea what's happening here, but I love the result!

@mrdoob mrdoob merged commit 4bf361f into dev Aug 31, 2021
@mrdoob mrdoob deleted the transmission-alpha branch August 31, 2021 19:05
@mrdoob
Copy link
Owner Author

mrdoob commented Aug 31, 2021

By the way, this is how it would look like without the + 0.1 hack:

Screen Shot 2021-08-31 at 8 11 39 PM

With the hack:

Screen Shot 2021-08-31 at 8 14 30 PM

@elalish what do you think?

@elalish
Copy link
Contributor

elalish commented Aug 31, 2021

The hack looks like it's definitely doing the right thing. The color seems pretty faint; are you sure it should be so low at 0.1? What happens adversely when it's raised?

@mrdoob
Copy link
Owner Author

mrdoob commented Aug 31, 2021

May try to modulate with the reflection too... some other day 😇

@WestLangley
Copy link
Collaborator

I would revert these hacks. It makes testing of new features difficult.

This approach is not going to work, because the CSS background needs to be attenuated by the material color.

I expect the only way to possibly do this is to pass a screenshot of the CSS background as a uniform to the shader.

@elalish
Copy link
Contributor

elalish commented Sep 1, 2021

Does it have an effect when the canvas is not set to use alpha? The idea was that it would be constrained only to changing the transparent canvas situation.

You're absolutely right that there's no physical way to do this, but I think it's still important to make a best effort: #22425 (comment)

@WestLangley
Copy link
Collaborator

You're absolutely right that there's no physical way to do this

Actually, I was not concerned about being physically correct. I was concerned about being remotely correct. :-)

@mrdoob
Copy link
Owner Author

mrdoob commented Sep 2, 2021

Unfortunately it's not possible to take a screenshot of the css background. So we are trying to get something that looks decent.

But I think this code should only affect cases where alpha: true is passed to WebGLRenderer. Are you saying this is affecting the alpha: false case too?

@WestLangley
Copy link
Collaborator

Sorry, I can not comprehend this code, so I do not know what the hacks are doing.

export default /* glsl */`
#ifdef OPAQUE
diffuseColor.a = 1.0;
#endif

// https://github.com/mrdoob/three.js/pull/22425
#ifdef USE_TRANSMISSION
diffuseColor.a *= transmissionAlpha + 0.1;
#endif

gl_FragColor = vec4( outgoingLight, diffuseColor.a );
`;

I think the spheres in the test case all have opaque true. What is the blending function being used ? No blending? Normal blending? As far as I know, we have not agreed on what the alpha type means for three.js.

And diffuseColor.a can be 1.1? What does the compositor do with a value greater than 1? Is it clamped?

I really do not want to have to be concerned about these issues while I am trying to fix the remaining deficiencies in the shaders.

@mrdoob
Copy link
Owner Author

mrdoob commented Sep 2, 2021

You can ignore this

#ifdef OPAQUE
diffuseColor.a = 1.0;
#endif

I'm thinking about reverting the material.format change and setting texture.format = THREE.RGBFormat in GLTFLoader instead.

This is my understanding:

// OPAQUE
material.map.format = THREE.RGBFormat;
material.transparent = false;

// MASK
material.map.format = THREE.RGBAFormat;
// TODO the alpha needs to be used for the mask only
material.alphaTest = alphaCutoff;
material.transparent = false;

// BLEND
material.map.format = THREE.RGBAFormat;
material.transparent = true;

@mrdoob
Copy link
Owner Author

mrdoob commented Sep 2, 2021

@WestLangley

I think the spheres in the test case all have opaque true. What is the blending function being used ? No blending? Normal blending? As far as I know, we have not agreed on what the alpha type means for three.js.

As far as I understand... OPAQUE, MASK and BLEND only apply to material.map.

And diffuseColor.a can be 1.1? What does the compositor do with a value greater than 1? Is it clamped?

As far as I know, values greater than 1 get clamped.

@WestLangley
Copy link
Collaborator

// OPAQUE
material.map.format = THREE.RGBFormat;
material.transparent = false;

I would tend to agree. Of course, that means no alpha test using map.a.

// MASK
material.map.format = THREE.RGBAFormat;
// TODO the alpha needs to be used for the mask only
material.alphaTest = alphaCutoff;
material.transparent = false;

Maybe to avoid the transparent restriction it may be sufficient to modify the three.js alpha test code so alpha > alphaTest is set to 1.

// BLEND
material.map.format = THREE.RGBAFormat;
material.transparent = true;

Yes, I think so.

Maybe we need is a material.alphaMask flag? -- or do we just leave this as a GLTFLoader thing?

@WestLangley
Copy link
Collaborator

As far as I understand... OPAQUE, MASK and BLEND only apply to material.map.

Based on above, they also control the blending function in three.js

@mrdoob
Copy link
Owner Author

mrdoob commented Sep 2, 2021

Maybe to avoid the transparent restriction it may be sufficient to modify the three.js alpha test code so alpha > alphaTest is set to 1.

Do you mean that if alphaTest > 0 we can force diffuse.a to 1.0 after the discard?

@WestLangley
Copy link
Collaborator

WestLangley commented Sep 2, 2021

Yes. It is my understanding that is what the spec calls for -- when using MASK, set alpha to 1.

When using alpha test with normal blending, then perhaps what we are doing now is OK -- leaving non-discarded alpha as-is.

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.

3 participants