GLTFLoader: Explicitly clone ShaderMaterial properties.

[donmccurdy]

Fixes #donmccurdy/three-gltf-viewer#42 (thanks @emackey!).

The spec/gloss ShaderMaterial is pretty fragile. 😕 Maybe we will want to revisit the idea of having MeshSGStandardMaterial at some point?

[donmccurdy]

/cc @takahirox

[takahirox]

Just curious to know if ShaderMaterial can be cloned correctly, we don't need explicitly clone ShaderMaterial properties?
I'd like to know the root issue.

[donmccurdy]

The cause of the linked bug is that material.map was not copied when cloning a shader material.

ShaderMaterial does the right thing, copying everything it knows about and invoking Material.copy(). But the Material class doesn't know about all of the MeshStandardMaterial properties we've added to the ShaderMaterial to support spec/gloss. So we have to explicitly clone all the things ShaderMaterial doesn't know about, when in an ideal world everything that's not unique to spec/gloss would be inherited instead.

[takahirox]

Hm, so having MeshSGStandardMaterial would be another option?

[takahirox]

IMO, this change is good so far but I'd like to add comments into the code why we need Explicitly clone ShaderMaterial properties.

And we'll also should try to fix ShaderMaterial.copy() issue and then revert once we've done.

[donmccurdy]

Hm, so having MeshSGStandardMaterial would be another option?

Yeah. The current status of that is probably @mrdoob's comment here, that we would wait to see if people use the workflow. I don't really expect people to create spec/gloss materials manually, but importing models that use the workflow has a bit of traction. In this case, the Minecraft exporter creates spec/gloss materials (why it does that, I don't know...).

So IMO the SG workflow is not something most users need to worry about when creating materials programmatically, but ideally it should work reliably when importing models. 😕

And we'll also should try to fix ShaderMaterial.copy() issue and then revert once we've done.

Not sure I understand — ShaderMaterial.copy() is working as intended, as far as I can tell. It just doesn't know about all these properties we've added ourselves, and therefore can't clone them.

[takahirox]

The problem I'm worrying is ShaderMaterial.copy() of SG material still doesn't work even if we merge this change.

If users want to copy of SG material in their user code, they need to be aware of that they need to explicitly clone properties instead of calling .copy(). This's very confusing.

So I wondered if we can enable ShaderMaterial.copy() to copy properties that users/we add. (But adding special properties to ShaderMaterial would be out of Three.js/ShaderMaterial manner?)

In my mind, I have three options.

1. Support MeshSGStandardMaterial in Three.js core. It works fine but we need to reconsider MeshPhongMaterial.
2. Enable ShaderMaterial.copy() to copy properties added by users/us. But this'd be out of Three.js manner.
3. Declare MeshSGStandardMaterial in GLTFLoader.

I prefer 3. so far if it won't have any problems. Probably inheriting ShaderMaterial and overrides .copy() (and .clone()?) would be the easiest way. (Inheriting MeshStandardMaterial would be another option?)

[donmccurdy]

Even if we do (2) or (3), it will still not be possible to clone a THREE.ShaderMaterial created from a glTF model. In addition to copying properties, we need for the mesh.onBeforeRender callback of the material's mesh to refresh the material's uniforms, and ShaderMaterial.copy() can't do that.

This PR just corrects things so that at least the model renders correctly in the first place — cloning still does not work.

[takahirox]

OK, this change would be a good workaround if we add the comment why we need "Explicitly clone ShaderMaterial properties" into the code.

And let's keep discussing how to solve the root issue.

[donmccurdy]

OK, this change would be a good workaround if we add the comment why we need "Explicitly clone ShaderMaterial properties" into the code.

Sounds good — done.

[mrdoob]

Thanks guys!