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

Reintroduce MeshPhysicalMaterial #8529

Merged
merged 3 commits into from
Apr 5, 2016
Merged

Conversation

WestLangley
Copy link
Collaborator

As suggested the discussion in #8505, this PR adds MeshPhysicalMaterial by extending MeshStandardMaterial.

Also as proposed by @bhouston in #8505, the new MeshPhysicalMaterial adds the reflectivity parameter, which models the specular reflectance of the material for non-metals.

reflectivity takes values in [ 0, 1 ], and has a default value of 0.5, which maps to an F0 of 0.04.

/ping @bhouston

#define Material_BlinnShininessExponent( material ) GGXRoughnessToBlinnExponent( material.specularRoughness )

// ref: http://www.frostbite.com/wp-content/uploads/2014/11/course_notes_moving_frostbite_to_pbr_v2.pdf
float computeSpecularOcclusion( const in float dotNV, const in float ambientOcclusion, const in float roughness ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can move this to someplace common to remove the duplication?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. Some refactoring/cleanup is needed. I'll gladly take your suggestions on that. Can we do it in the next PR?

@bhouston
Copy link
Contributor

bhouston commented Apr 5, 2016

Thank you @WestLangley for this! Sweet!

I love the fact that we can add more material parameters but I am a little concerned that we have added 300 lines of code to be able to do that, and most of that code is duplicated code.- now we have to maintain two code paths that are nearly identical. It would be cool after this PR is merged to figure out some way to further share glsl code between MeshStandardMaterial and MeshPhysicalMaterial.

@@ -103,6 +103,36 @@ THREE.ShaderLib = {

},

'physical': {

uniforms: THREE.UniformsUtils.merge( [
Copy link
Contributor

Choose a reason for hiding this comment

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

could potentially merge in the parameters from 'standard'? Reduce deuplication?

@WestLangley
Copy link
Collaborator Author

It would be cool after this PR is merged to figure out some way to further share glsl code

Agreed. I was thinking that we could hack in the advanced MeshPhysicalMaterial features fairly quickly, and then refactor when we were done, rather than trying to refactor first -- unless it is clear to you already what will be common and what will not. Either approach is fine with me.

@bhouston
Copy link
Contributor

bhouston commented Apr 5, 2016

I wonder if we could create a "meshbsdf_vert.glsl", and a "meshbsdf_frag.glsl" that could be shared between Phong, Standard and Physical. I'd go even further and create a MeshBRDFMaterial that could serve as the base class between all three, and it would have the huge number of shared parameters:

https://github.com/mrdoob/three.js/blob/master/src/materials/MeshStandardMaterial.js
https://github.com/mrdoob/three.js/blob/master/src/materials/MeshPhongMaterial.js

@@ -1554,6 +1554,7 @@ THREE.WebGLRenderer = function ( parameters ) {
if ( material instanceof THREE.MeshPhongMaterial ||
material instanceof THREE.MeshLambertMaterial ||
material instanceof THREE.MeshStandardMaterial ||
material instanceof THREE.MeshPhysicalMaterial ||
Copy link
Owner

Choose a reason for hiding this comment

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

These checks aren't needed because THREE.MeshPhysicalMaterial extends THREE.MeshStandardMaterial so the check is already true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct. Will fix.

@mrdoob
Copy link
Owner

mrdoob commented Apr 5, 2016

It would be cool after this PR is merged to figure out some way to further share glsl code

Agreed. I was thinking that we could hack in the advanced MeshPhysicalMaterial features fairly quickly, and then refactor when we were done, rather than trying to refactor first -- unless it is clear to you already what will be common and what will not. Either approach is fine with me.

I think the duplicating approach is a bit confusing...

How about we add features on the glsl code, and maybe rename that code to PHYSICAL.
Then on the javascript side we just make the MeshPhysicalMaterial extension as this PR already does.

So both materials use the same glsl code, but MeshStandardMaterial exposes less of it.

@WestLangley
Copy link
Collaborator Author

I think the duplicating approach is a bit confusing...
So both materials use the same glsl code, but MeshStandardMaterial exposes less of it.

Yes, we could add the physical.glsl files here, and then remove the standard.glsl files in the next PR. Then standard would use the physical shader, and we would use if defined STANDARD, etc, as needed.

@mrdoob
Copy link
Owner

mrdoob commented Apr 5, 2016

Yes, we could add the physical.glsl files here, and then remove the standard.glsl files in the next PR. Then standard would use the physical shader, and we would use if defined STANDARD, etc, as needed.

Sounds good!

@mrdoob mrdoob merged commit 5a6df63 into mrdoob:dev Apr 5, 2016
@mrdoob
Copy link
Owner

mrdoob commented Apr 5, 2016

Thanks!

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