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: clearcoat should not use underlying normal map #12867

Closed
4 of 13 tasks
RenateOnVizoo3d opened this issue Dec 12, 2017 · 23 comments
Closed
4 of 13 tasks

Comments

@RenateOnVizoo3d
Copy link

RenateOnVizoo3d commented Dec 12, 2017

I have recognized that the PhyscialMaterialShader is taking the perturbed normal of the material into consideration.
Seeing how the effect is implemented elsewhere (e.g. in Blender using the Principled BSDF shader node), I would expect the clearcoat to behave like a "flat additional coating" on top of the material and not being perturbed by the underlying normal map. Thus as a change request, I would suggest to conserve the "original" normal which should be fed into the calculation of the clearcoat reflection.

In order to achieve this getLightProbeIndirectRadiance needs to distinguish between specular and clearcoat reflection mapping; see Pseudocode:

vec3 getLightProbeIndirectRadiance( const in GeometricContext geometry, const in float blinnShininessExponent, const in int maxMIPLevel ) {

      vec3 reflectVec;
      if ( specular_reflections ) {
            reflectVec = reflect( -geometry.viewDir, geometry.normal );
       } 
      else {      // clearcoat reflections
      reflectVec = reflect( -geometry.viewDir, geometry.clearCoatNormal );
      }
[...]
}

Alternativley passing the respective normals via the the function parameters (or adding the .originalNormal to the GeometricContext structure) could help too,

Cheers,
Renate

img attachment: glossy leather with clearcoat on top

Three.js version
  • Dev
  • r88
  • ...
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • macOS

capture

  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)
@WestLangley
Copy link
Collaborator

Can you hack a live example as a proof of concept?

@RenateOnVizoo3d
Copy link
Author

RenateOnVizoo3d commented Dec 18, 2017

In the package attached you'll find the modified three.js file as well as some notes, what I did to achieve the effect. For SURE, there are better ways to do this.
As I am new to this: pls let me know if other files/ implementation is expected.

[-> removed zip file and replaced with online demo on Jan 18]

@WestLangley
Copy link
Collaborator

If you could include a live example in the zip file that would be helpful...

If we were to implement this, I expect we would allow the clearCoat to have a separate normal map, that way, clearCoat surface scratches can be modeled. It is also a more general solution.

@RenateOnVizoo3d
Copy link
Author

RenateOnVizoo3d commented Dec 19, 2017

Here is our test environment. Please unzip and run index.html (ideally in Firefox as we have not tested it elsewhere). Then browse (top left corner) for /materials/Green Leather/Green Leather.xtex
Let me know if this works.

[-> removed zip file and replaced with online demo on Jan 18]

Totally agree. An (optional) separate clearcoat normal would be the most meaningful solution. Thus clearcoat would be specified by intensity, roughness and normal.

@WestLangley
Copy link
Collaborator

I think your demo is going to have to show a side-by-side comparison of two spheres to be able to see the subtlety you are trying to implement.

Thus clearcoat would be specified by intensity, roughness and normalMap.

... and normalScale.

@RenateOnVizoo3d
Copy link
Author

RenateOnVizoo3d commented Jan 10, 2018

Please see the original version of the webGl_testenvironment attached.
[-> removed zip file and replaced with online demo on Jan 18]

You should see the difference compared to the implementation in my last post when playing with normal, clearcoat / clearcoat roughness and environment intensity. here (webGl_testenvironment - originalClearCoatBehaviour.zip) the clearcoat gets perturbed by the normals and here (webGl_testenvironment.zip) the clearcoat stays as a flat reflection layer.

See more explanation on the clearcoat behaviour here:
https://docs.blender.org/manual/ru/dev/render/cycles/nodes/types/shaders/principled.html

sidebysidecomparison

@WestLangley
Copy link
Collaborator

Your demos throw console errors for me. Sorry, I am not able to invest the time to debug this.

I am still interested in your suggestion, however.

@mrdoob
Copy link
Owner

mrdoob commented Jan 10, 2018

/cc @bhouston

@bhouston
Copy link
Contributor

Unreal Engine 4 does this:

https://docs.unrealengine.com/latest/INT/Engine/Rendering/Materials/HowTo/ClearCoatDualNormal/

I've wanted this as well.

Basically the main way to do this is to add a second normal map and have it used for either the top layer or the bottom layer. I would prefer to have the second normal map be used for the surface because then we can call it clearCoatNormalMap and it is part of the clear coat top layer.

If it isn't specified, then we would use the underlying normalMap for both? Or we could allow for a scalar parameter, clearCoatNormalWeight, that would scale between the clearCoatNormalMap and the underlying normalMap - normally it would be 0 and thus favor the underlying normalMap/bumpMap, but if you set it to 1 it would use the clearCoatNormalMap completely or it would be perfectly smooth if the clearCoatNormalMap isn't set.

Does that logic work?

@WestLangley
Copy link
Collaborator

It is probably sufficient to keep it simple and just to add two properties:

material.clearCoatNormalMap
material.clearCoatNormalScale

If clearCoatNormalMap is null, the clear coat uses no normal map.

@bhouston
Copy link
Contributor

It is probably sufficient to keep it simple and just to add two properties:

Sure, I am fine with your suggestion. Just to be clear, this is for the top layer normal and not the bottom layer as it is with UE4? I think that is the more logical choice. clearCoatNormalMAp == null leads to a smooth surface for clearcoat but normal/bump affect the underlying layer.

@WestLangley
Copy link
Collaborator

@bhouston Correct.

@RenateOnVizoo3d
Copy link
Author

@WestLangley First, apologies for the example throwing errors. This should not have happened.

@bhouston I just wanted to confirm from our side that a clearCoatNormalMap == null should lead to a completely smooth, unperturbed clear coat layer. As if the materials has an additional transparent painting.
To keep it simple I would use the second normal map only for the clearcoat (top) layer. The bottom layer should not be affected.

For a comprehensive implementation a
material.clearCoatNormalMap
material.clearCoatNormalScale
would be needed to control the normals of the clearcoat. Moreover the intensity and roughness of this layer would be tweaked by:
material.clearCoat [0 = off, 1 = on]
material.clearCoatRoughness [0 = smooth, 1 = rough]

@RenateOnVizoo3d
Copy link
Author

@WestLangley

Your demos throw console errors for me. Sorry, I am not able to invest the time to debug this.

Did you try in Mozilla Firefox? The demo is not running in any other browser yet

@WestLangley
Copy link
Collaborator

Did you try in Mozilla Firefox? The demo is not running in any other browser yet

No. Maybe you will fix your demo.

@mrdoob mrdoob added this to the rXX milestone Jan 18, 2018
@RenateOnVizoo3d
Copy link
Author

The 2 demos are online now and should work in all Browsers.

Please open the links below and load the "Green leather" material which you find when unzipping the attached (select .xTex file, see screenshot attached)
Green Leather.zip

Please see clearcoat hack here:
https://www.vizoo3d.com/tmp_Renate/webGl_testenvironmentCLEARCOATIMPLEMENTATION/3js/

And compare with the original / current clearcoat behavior here:
https://www.vizoo3d.com/tmp_Renate/webGl_testenvironmentORIGINALCLEARCOATBEHAVIOUR/3js/

How to load the material Green Leather.zip:
image

@WestLangley
Copy link
Collaborator

Thanks. I see your demo is working now.

@robertoranon
Copy link
Contributor

Hi! I'm also interested in a similar modification, and I am willing to implement it if the author of the first post is not interested anymore. The two demos now result in a 404, so I cannot see what has been done so far. However, I am not sure I understand why only getLightProbeIndirectRadiance() should be modified. Also direct light should take into account the double normal, right?

@RenateOnVizoo3d
Copy link
Author

Hi @robertoranon,

I am still very keen on having this feature available in three.js.
However, I am not a contributor to three.js and therefore would not do the implementation myself. I was hoping that this feature gets scheduled for one of the upcoming releases.
You have already noticed that my "hack" is not 100% complete. I was only trying to proof that the visual outcome is better than without the double normal... but obviously did not make myself familiar enough with contributing to three.js.

I hope very much to see some implementation and I am happy to help if I can. Would be great to keep me posted on that.

Let me also see if I can put our examples online again.

Cheers

@RenateOnVizoo3d
Copy link
Author

Hi again,

I have put
https://www.vizoo3d.com/tmp_Renate/webGl_testenvironmentCLEARCOATIMPLEMENTATION/3js/
online again.

See instructions and download of the GreenLeather Material in my posting from 18 Jan.

@mrdoob
Copy link
Owner

mrdoob commented Jul 31, 2018

@WestLangley

It is probably sufficient to keep it simple and just to add two properties:

material.clearCoatNormalMap
material.clearCoatNormalScale

If clearCoatNormalMap is null, the clear coat uses no normal map.

How about we start by just making sure the clearCoat layer doesn't have the geometry.normal perturbed? We can add parametrization later.

@WestLangley
Copy link
Collaborator

How about we start by just making sure the clearCoat layer doesn't have the geometry.normal perturbed?

Agreed. That should be the objective.

Thing is, it seems to me the code needs to be significantly refactored to do that...

@donmccurdy
Copy link
Collaborator

Fixed by #17079. Thank you @arobertson0!

@mrdoob mrdoob removed this from the rXXX milestone Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants