Add MeshStandardMaterialSG #11038

takahirox · 2017-03-21T07:22:13Z

This PR

adds MeshStandardMaterialSG which can handle glTF Specular-Glossiness extension.
lets GLTF2Loader support glTF Specular-Glossiness extension.

You can see MeshStandardMaterialSG example in In webgl_loader_gltf2 example
by selecting Specular-Glossiness extension with BoomBox model.

Only the difference between MeshStandardMaterial and MeshStandardMaterialSG
is metalness/roughness or specular/glossiness.
So I made MeshStandardMaterialCommon and let them inherit it.

/cc @donmccurdy @WestLangley

takahirox · 2017-03-21T08:04:55Z

Well, let me close once.
I think I can rename specular2 with specular.
Let me try...

takahirox · 2017-03-21T08:41:22Z

Updated and reopened

WestLangley · 2017-03-21T16:09:54Z

Nice job.

I personally do not care for your refactoring, however. You have taken an implementation detail, and pushed it up to the API and created another layer of obfuscation -- especially for users -- by creating the new MeshStandardMaterialCommon, which is sort of like a virtual base class.

As I suggested here, leave MeshStandardMaterial as-is, and extend it -- similarly to your implementation of MeshToonMaterial.

Maybe something like this:

function MeshStandardMaterialSG( parameters ) {

	MeshStandardMaterial.call( this );

	this.defines[ 'STANDARD_SG' ] = '';

	this.type = 'MeshStandardMaterialSG';

	this.glossiness = 0.5;
	this.specular = new Color( 0x111111 );

	this.glossinessMap = null;

	this.specularMap = null;

	delete this.metalness;
	delete this.roughness;
	delete this.metalnessMap;
	delete this.roughnessMap;

	this.setValues( parameters );

}

Yes, I understand that you can argue otherwise from a software-design standpoint. However, I expect that 99% of users will use MeshStandardMaterial, and the SG version only by sophisticated users who prefer the specular-glossiness workflow.

takahirox · 2017-03-21T16:24:55Z

OK, that makes sense.
(I had misunderstood "as-is" and "remove" you mentioned in the thread.)
I'll update.

WestLangley · 2017-03-21T16:32:48Z

src/renderers/shaders/ShaderChunk/uv_pars_fragment.glsl

@@ -1,4 +1,4 @@
-#if defined( USE_MAP ) || defined( USE_BUMPMAP ) || defined( USE_NORMALMAP ) || defined( USE_SPECULARMAP ) || defined( USE_ALPHAMAP ) || defined( USE_EMISSIVEMAP ) || defined( USE_ROUGHNESSMAP ) || defined( USE_METALNESSMAP )
+#if defined( USE_MAP ) || defined( USE_BUMPMAP ) || defined( USE_NORMALMAP ) || defined( USE_SPECULARMAP ) || defined( USE_ALPHAMAP ) || defined( USE_EMISSIVEMAP ) || defined( USE_ROUGHNESSMAP ) || defined( USE_METALNESSMAP ) || defined( USE_GLOSSINESSMAP) || defined( USE_SPECULAR2MAP )


SPECULAR2MAP ?
defined( USE_GLOSSINESSMAP ) // white space

It is unfortunate that specularMap is used for multiple purposes. We will have to fix that. It should be RGB and represent the specular reflectance of the material.

If I'm right, this is a related comment? #9339 (comment)

WestLangley · 2017-03-21T16:33:30Z

src/renderers/shaders/ShaderLib.js

@@ -82,12 +82,15 @@ var ShaderLib = {
 			UniformsLib.displacementmap,
 			UniformsLib.roughnessmap,
 			UniformsLib.metalnessmap,
+			UniformsLib.glossinessmap,
 			UniformsLib.fog,
 			UniformsLib.lights,
 			{
 				emissive: { value: new Color( 0x000000 ) },
 				roughness: { value: 0.5 },
 				metalness: { value: 0 },


Yes, should be

metalness: { value: 0.5 }

That isn't what I edited.
I already noticed that and sent another PR.

#11039

Yes, I saw that PR earlier. :-)

Ah, ok. I understand what you meant.

…et MeshStandardMaterialSG inherit MeshStandardMaterial

takahirox · 2017-03-22T00:54:54Z

Updated

mrdoob · 2017-03-26T18:20:05Z

As discussed in the SFHTML5 meetup (😁), I think it'll be better if, for now, GLTF2Loader just produces a ShaderMaterial for the specular/glossiness case.

takahirox · 2017-03-26T18:34:59Z

Alright, I wanna try that before I leave the USA!

WestLangley · 2017-03-26T18:50:06Z

As discussed in the SFHTML5 meetup (😁), I think it'll be better if, for now, GLTF2Loader just produces a ShaderMaterial for the specular/glossiness case.

That suits me fine.

I assume we then do not require the addition of MeshStandardMaterialSG ?

mrdoob · 2017-03-26T18:52:48Z

I assume we then do not require the addition of MeshStandardMaterialSG ?

Yeah, that's the idea. I personally didn't like the name and I couldn't think of a better name. At this point this is mainly for the GLTF2Loader anyway.

takahirox · 2017-03-30T05:20:24Z

Well, I couldn't make a time.
I'll implement after I land in Japan ;D

bhouston · 2017-04-02T02:59:46Z

The MeshPhong model is specular/glossy. If we merge this, we should remove MeshPhong if this is accepted. We really shouldn't have two materials that are basically the same thing.

WestLangley · 2017-04-02T04:34:37Z

we should remove MeshPhong if this is accepted.

In light of what @bhouston said, I agree. I did not realize that was an option.

Removing Phong would make our shader code simpler, plus Phong, as implemented, does not model metals well, and the way it handles environment map reflections is hacky.

We could keep Lambert, which is non-specular and implements Gouraud shading.

…l, and let MeshStandardMaterialSG inherit MeshStandardMaterial" This reverts commit 743524c.

This reverts commit bc88561.

This reverts commit 27999fb.

…rial

takahirox · 2017-04-23T05:27:48Z

I've implemented Specular-Glossiness extension support with ShaderMaterial.

On the current implementation, GLTF2Loader setups its uniforms and defines.
Users need to update its uniforms value(ex: material.uniforms.map.value) and its defines,
not its properties(ex: material.map), if they wanna update material values by themselves.
(I think this's ShaderMaterial standard manner.)

I'll try onBeforeRender() later to allow users to update its properties.

The code would be like this,

object.onBeforeRender = function( renderer, scene, camera, geometry, material, group ) {

    material.uniforms.map.value = material.map;

}

([de]serialization would't work well with it tho)

…erial

takahirox · 2017-04-23T11:13:09Z

Done

mrdoob · 2017-04-23T15:54:16Z

Sweeeet!

mrdoob · 2017-04-23T15:55:11Z

@donmccurdy looks good?

WestLangley · 2017-04-24T00:21:32Z

examples/js/loaders/GLTF2Loader.js

+							uniforms.envMap.value = material.envMap;
+							uniforms.normalMap.value = material.normalMap;
+							uniforms.specularMap.value = material.specularMap;
+							uniforms.glossinessMap.value = material.glossinessMap;


Do you need to add these? Maybe others?

uniforms.normalScale uniforms.displacementMap uniforms.displacementScale uniforms.displacementBias uniforms.envMapIntensity uniforms.opacity uniforms.offsetRepeat

Oops, right.
I think we should also have them and others
for the compatibility with StandardMaterial as much as possible.
I'll update...

Yes, this is a pain, but it is illustrative to show how it is done...

It is also why I prefer the oddly-named MeshStandardMaterialSG as a built-in material over this ShaderMaterial approach.

…with MeshStandardMaterial

takahirox · 2017-04-25T14:09:45Z

Updated

WestLangley · 2017-04-25T19:16:33Z

I know I have been overruled, but MeshSGStandardMaterial is sounding even better to me. : - )

donmccurdy · 2017-04-25T20:05:15Z

Nice work @takahirox!

The implementation of this approach looks good to me, and I don't have any comment on MeshSGStandardMaterial vs ShaderMaterial here.

If possible in a clean way, I might prefer to isolate the spec-gloss code in an extension object. Something like:

class GLTFSpecGlossExtersion {
  extendMaterialParams (params, ...) {
     params.foo = bar;
     // ...
  }

  createShaderMaterial (params) {
    material = new THREE.ShaderMaterial( /* ... */ );
    // ...
    material.onBeforeRender = function () { /* ... */ };
    return material;
  }
}

This helps readability IMHO, avoiding 500+ line methods, and the division is clean enough because the specification for extensions is self-contained already. But I'm OK with the code as-is, if we prefer to have all material code in one larger method.

mrdoob · 2017-04-25T22:53:05Z

I know I have been overruled, but MeshSGStandardMaterial is sounding even better to me. : - )

Lets wait to see if people actually use this workflow.

takahirox · 2017-04-26T05:58:05Z

Updated!

donmccurdy · 2017-04-26T14:21:51Z

Great thanks — looks good to me!

mrdoob · 2017-04-27T07:01:42Z

Very nice!

mrdoob · 2017-04-27T07:01:49Z

Thanks!

WestLangley · 2017-07-20T21:42:39Z

examples/js/loaders/GLTF2Loader.js

@@ -2475,6 +2464,47 @@ THREE.GLTF2Loader = ( function () {

 					}

+					// for Specular-Glossiness
+					if ( child.material && child.material.type === 'ShaderMaterial' ) {


@takahirox Can you change this so material.type is not used for this purpose? I do not think that pattern is used elsewhere...

In this case, it should be something like

if ( child.material && child.material.isGLTFSpecularGlossinessMaterial ) {

takahirox added 4 commits March 20, 2017 21:36

Add MeshStandardMaterialSG

dc4a2db

Add glTF PBR Specular-Glossiness extension support

4a0b4a4

Make MeshStandardMaterialCommon

225dabd

Fix diffuse shader code of Specular-Glossiness

8360b53

takahirox closed this Mar 21, 2017

Rename specular2 with specular of MeshStandardMaterialSG

8053b6f

takahirox reopened this Mar 21, 2017

takahirox added 2 commits March 21, 2017 01:38

Rename specular2 with specular in GLTF2Loader

e158265

Glossiness support in serialization/deserialization

27999fb

WestLangley reviewed Mar 21, 2017

View reviewed changes

takahirox added 3 commits March 21, 2017 14:07

Remove USE_SPECULAR2MAP from shader

bc88561

Remove MeshStandardMaterialCommon, Revert MeshStandardMaterial, and l…

743524c

…et MeshStandardMaterialSG inherit MeshStandardMaterial

Merge branch 'dev' into SpecularGlossiness

f82763e

takahirox added 4 commits April 9, 2017 15:50

Merge branch 'dev' into SpecularGlossiness

c451782

Revert "Remove MeshStandardMaterialCommon, Revert MeshStandardMateria…

61e2ddb

…l, and let MeshStandardMaterialSG inherit MeshStandardMaterial" This reverts commit 743524c.

Revert "Remove USE_SPECULAR2MAP from shader"

60c42ec

This reverts commit bc88561.

Revert "Glossiness support in serialization/deserialization"

10d76dc

This reverts commit 27999fb.

Merge branch 'dev' into SpecularGlossiness

6f3aa27

takahirox changed the title ~~Add MeshStandardMaterialSG~~ (WIP) Add MeshStandardMaterialSG Apr 21, 2017

takahirox added 3 commits April 22, 2017 14:40

Merge branch 'dev' into SpecularGlossiness

2f430ff

Specular-Glossiness extension of glTF 2.0 PBR supoort with ShaderMate…

585c50b

…rial

Add Specular-Glossiness extension support in GLTF2Loader example

327a67f

Update uniforms in onBeforeRender() for Specular-Glossiness ShaderMat…

2245b6f

…erial

takahirox changed the title ~~(WIP) Add MeshStandardMaterialSG~~ Add MeshStandardMaterialSG Apr 23, 2017

WestLangley reviewed Apr 24, 2017

View reviewed changes

Add properties to Specular-Glossiness material for the compatibility …

13c8864

…with MeshStandardMaterial

Clean up the Specular Glossiness extension code

f99c157

mrdoob merged commit 4c6bd58 into mrdoob:dev Apr 27, 2017

takahirox deleted the SpecularGlossiness branch April 27, 2017 07:45

donmccurdy mentioned this pull request Jul 11, 2017

Support for MTL map_Ns (shininess map) #9339

Closed

12 tasks

WestLangley reviewed Jul 20, 2017

View reviewed changes

donmccurdy mentioned this pull request Jul 22, 2017

[gltf] Fixes for interpolation, normal map handedness, and cleanup. #11825

Merged

donmccurdy mentioned this pull request Sep 25, 2017

GLTFLoader: Explicitly clone ShaderMaterial properties. #12274

Merged

donmccurdy mentioned this pull request Dec 1, 2018

GLTFExporter: Support spec/gloss PBR materials #15351

Closed

polarathene mentioned this pull request Mar 21, 2020

MeshStandardMaterialSG - Only available to GLTF loader? #18938

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MeshStandardMaterialSG #11038

Add MeshStandardMaterialSG #11038

takahirox commented Mar 21, 2017 •

edited

Loading

takahirox commented Mar 21, 2017

takahirox commented Mar 21, 2017

WestLangley commented Mar 21, 2017

takahirox commented Mar 21, 2017 •

edited

Loading

WestLangley Mar 21, 2017

takahirox Mar 22, 2017

WestLangley Mar 21, 2017

takahirox Mar 21, 2017

WestLangley Mar 21, 2017

takahirox Mar 22, 2017

takahirox commented Mar 22, 2017

mrdoob commented Mar 26, 2017

takahirox commented Mar 26, 2017

WestLangley commented Mar 26, 2017

mrdoob commented Mar 26, 2017

takahirox commented Mar 30, 2017

bhouston commented Apr 2, 2017

WestLangley commented Apr 2, 2017

takahirox commented Apr 23, 2017 •

edited

Loading

takahirox commented Apr 23, 2017

mrdoob commented Apr 23, 2017

mrdoob commented Apr 23, 2017

WestLangley Apr 24, 2017

takahirox Apr 24, 2017 •

edited

Loading

WestLangley Apr 24, 2017

takahirox commented Apr 25, 2017

WestLangley commented Apr 25, 2017

donmccurdy commented Apr 25, 2017 •

edited

Loading

mrdoob commented Apr 25, 2017

takahirox commented Apr 26, 2017

donmccurdy commented Apr 26, 2017

mrdoob commented Apr 27, 2017

mrdoob commented Apr 27, 2017

WestLangley Jul 20, 2017 •

edited

Loading

		@@ -1,4 +1,4 @@
		#if defined( USE_MAP ) \|\| defined( USE_BUMPMAP ) \|\| defined( USE_NORMALMAP ) \|\| defined( USE_SPECULARMAP ) \|\| defined( USE_ALPHAMAP ) \|\| defined( USE_EMISSIVEMAP ) \|\| defined( USE_ROUGHNESSMAP ) \|\| defined( USE_METALNESSMAP )
		#if defined( USE_MAP ) \|\| defined( USE_BUMPMAP ) \|\| defined( USE_NORMALMAP ) \|\| defined( USE_SPECULARMAP ) \|\| defined( USE_ALPHAMAP ) \|\| defined( USE_EMISSIVEMAP ) \|\| defined( USE_ROUGHNESSMAP ) \|\| defined( USE_METALNESSMAP ) \|\| defined( USE_GLOSSINESSMAP) \|\| defined( USE_SPECULAR2MAP )

Add MeshStandardMaterialSG #11038

Add MeshStandardMaterialSG #11038

Conversation

takahirox commented Mar 21, 2017 • edited Loading

takahirox commented Mar 21, 2017

takahirox commented Mar 21, 2017

WestLangley commented Mar 21, 2017

takahirox commented Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahirox commented Mar 22, 2017

mrdoob commented Mar 26, 2017

takahirox commented Mar 26, 2017

WestLangley commented Mar 26, 2017

mrdoob commented Mar 26, 2017

takahirox commented Mar 30, 2017

bhouston commented Apr 2, 2017

WestLangley commented Apr 2, 2017

takahirox commented Apr 23, 2017 • edited Loading

takahirox commented Apr 23, 2017

mrdoob commented Apr 23, 2017

mrdoob commented Apr 23, 2017

Choose a reason for hiding this comment

takahirox Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahirox commented Apr 25, 2017

WestLangley commented Apr 25, 2017

donmccurdy commented Apr 25, 2017 • edited Loading

mrdoob commented Apr 25, 2017

takahirox commented Apr 26, 2017

donmccurdy commented Apr 26, 2017

mrdoob commented Apr 27, 2017

mrdoob commented Apr 27, 2017

WestLangley Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

takahirox commented Mar 21, 2017 •

edited

Loading

takahirox commented Mar 21, 2017 •

edited

Loading

takahirox commented Apr 23, 2017 •

edited

Loading

takahirox Apr 24, 2017 •

edited

Loading

donmccurdy commented Apr 25, 2017 •

edited

Loading

WestLangley Jul 20, 2017 •

edited

Loading