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

MeshPhysicalMaterial: added transparency #17114

Merged
merged 4 commits into from
Aug 23, 2019

Conversation

WestLangley
Copy link
Collaborator

@WestLangley WestLangley commented Jul 28, 2019

Inspired by the efforts of @DanielSturk in #16996, this WIP implements a different approach to model thin transparency.

Transparency is supported by adding a single line of code to the shader:

diffuseColor.a *= saturate( 1. - transparency + linearToRelativeLuminance( reflectedLight.directSpecular + reflectedLight.indirectSpecular ) );

The opacity of the specular highlights are increased using a heuristic. It seems to have reasonable behavior under a wide variety of test cases. It is similar to the approach used by Babylon.

I have hacked it in as a stub.

Quite frankly, I am opposed to these kinds of hacks, and if it were not for the fact that it appears plausible, I would not support it. My concern is the model adds quantities of different units:

1. - transparency + linearToRelativeLuminance( reflectedLight.directSpecular + reflectedLight.indirectSpecular

transparency is unit-less, and final term has units of luminous radiance.

This could be fixed by adding a scaling constant to change the units of the final term, but I do not (yet) know how to set the value of that constant. Here, the constant is effectively 1.

Screen Shot 2019-08-20 at 8 02 24 PM

EDIT: live link removed

Notes:

  1. In the live example, an alpha map is used, which is attenuated by material.opacity. Typically opacity in these use cases would be set to 1. material.transparency is used to control the glass-like appearance.

  2. Also in the live example, the left sphere has material.premultipliedAlpha set to false; in the right sphere it is set to true. It does not seem to matter, which is a good thing, I think.

@mrdoob mrdoob added this to the r108 milestone Jul 31, 2019
@mrdoob
Copy link
Owner

mrdoob commented Jul 31, 2019

  1. Also in the live example, the left sphere has material.premultipliedAlpha set to false; in the right sphere it is set to true. It does not seem to matter, which is a good thing, I think.

So what is material.premultipliedAlpha for then? 🤔

@bhouston
Copy link
Contributor

bhouston commented Aug 1, 2019

So what is material.premultipliedAlpha for then? 🤔

material.premultipliedAlpha still results in more correct results when using opacity. But we were using it somewhat inappropriately for a transparency like effect.

@bhouston
Copy link
Contributor

I like this PR, I would like to see it merged.

@WestLangley
Copy link
Collaborator Author

I like this PR, I would like to see it merged.

OK, I'll clean this PR up...

@WestLangley WestLangley force-pushed the dev_pbr_transparency branch from 1bacebb to e09031c Compare August 21, 2019 09:22
@WestLangley
Copy link
Collaborator Author

@arobertson0 I'm trying to keep up with your refactoring... :-)

Does this look right to you?

@arobertson0
Copy link
Contributor

Looks perfect! Also love how it just fits simply on one line.

@WestLangley
Copy link
Collaborator Author

WestLangley commented Aug 22, 2019

@sunag Does this pattern:

#ifdef PHYSICAL
	#define REFLECTIVITY
	#define CLEARCOAT
	#define TRANSPARENCY
#endif

. . .

#ifdef TRANSPARENCY
	uniform float transparency;
#endif

#ifdef REFLECTIVITY
	uniform float reflectivity;
#endif

make things easier for you with Node?

@sunag
Copy link
Collaborator

sunag commented Aug 22, 2019

@WestLangley Good point. NodeMaterial does not use standard uniform with default, it would be better to move all this uniforms to related chunks to avoid unnecessary overhead.

#ifdef TRANSPARENCY
	uniform float transparency;
#endif
#ifdef REFLECTIVITY
	uniform float reflectivity;
#endif
#ifdef CLEARCOAT
	uniform float clearcoat;
	uniform float clearcoatRoughness;
#endif
#ifdef USE_SHEEN
	uniform vec3 sheen;
#endif

@sunag
Copy link
Collaborator

sunag commented Aug 22, 2019

@WestLangley Good point. NodeMaterial does not use standard uniform with default, it would be better to move all this uniforms to related chunks to avoid unnecessary overhead.

No, no, this was put in ShaderLib. Sry, it is perfect the way it is.

@WestLangley WestLangley changed the title WIP MeshPhysicalMaterial: added transparency MeshPhysicalMaterial: added transparency Aug 22, 2019
@WestLangley
Copy link
Collaborator Author

@mrdoob This is good-to-go.

I can make changes to the example in a subsequent PR if you want.

@mrdoob mrdoob merged commit d39dabb into mrdoob:dev Aug 23, 2019
@mrdoob
Copy link
Owner

mrdoob commented Aug 23, 2019

Thanks!

@WestLangley WestLangley deleted the dev_pbr_transparency branch August 23, 2019 03:26
@mrdoob
Copy link
Owner

mrdoob commented Aug 23, 2019

Are both spheres supposed to look the same?

@WestLangley
Copy link
Collaborator Author

I think so. Previously, @bhouston added material.premultipliedAlpha support to help get this look.

If there is no clamping of RGB shader output to [0, 1], then they should definitely look the same.

I am not sure what the use cases are in which this clamping occurs. It has to do with the render target color bit depth.

@sunag
Copy link
Collaborator

sunag commented Aug 26, 2019

premultipliedAlpha add additive blend. I implemented transparency @WestLangley formula in StandardNode, it was a little easier to see the difference.
https://rawgit.com/sunag/three.js/dev-r8/examples/webgl_materials_nodes.html?e=transparency

@EliasHasle
Copy link
Contributor

The opacity of the specular highlights are increased using a heuristic.

I take it this is necessary because physical light ranges much wider than what can be represented decently in 8 bits. But what will happen if you render this to a float, which (if I am not wrong) can represent a wider range well?

@bhouston
Copy link
Contributor

I take it this is necessary because physical light ranges much wider than what can be represented decently in 8 bits. But what will happen if you render this to a float, which (if I am not wrong) can represent a wider range well?

Internally we represent everything as floating point anyways and then we apply a tone map prior to rendering thus we do generally achieve a true HDR pipeline. I believe the increased opacity heuristic is unrelated to numeric range limitations, and more about visual correctness.

@EliasHasle
Copy link
Contributor

@bhouston The tonemapping is applied only to rgb, so alpha can ruin the end result anyway. Consider a case such as when you are out in the sun and try to look through a basement window. Not only does your eye turn down the light sensitivity to a level where everything behind the window looks nearly black, but if the angle is right you will have a sharp specular reflection of the sun in the window glass, which masks everything else. If you apply the window alpha to that reflected sunlight after clamping the sunlight down to 1, the result will be greyish. If I am not mistaken.

@bhouston
Copy link
Contributor

bhouston commented Aug 28, 2019 via email

@gkjohnson
Copy link
Collaborator

So just to be clear it is still more correct to set premultipliedAlpha = true even when using the transparency field, right?

@WestLangley
Copy link
Collaborator Author

So just to be clear it is still more correct to set premultipliedAlpha = true even when using the transparency field, right?

We have implemented a heuristic because (1) it was the best we could do, and (2) it appears physically plausible. Since it is not a physical model, I do not think "more correct" is the correct term to use.

I think it would be beneficial to do a test with an LDR pipeline and an HDR pipeline (preferably tone mapped) and see if you can find use cases where material.premultipliedAlpha produces improved visual output.

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.

7 participants