Copy .onBeforeCompile in Material.copy()

[takahirox]

This PR add missing .onBeforeCompile copying in Materia.copy.
With this change, Materials hacked with .onBeforeCompile() can be copied/cloned.

[donmccurdy]

Per #14009 (comment), I'm not sure about this change:

...note that all Object3D instances extend EventDispatcher, and can have arbitrary event callbacks attached to them — these are not cloned. I'm suggesting that hooks like onBeforeFoo are effectively an event callback and not a "property", and that it doesn't make sense for it to be cloned. I would expect for cloning to give identical results to serializing and de-serializing an object. For another example, cloning DOM nodes does not copy event listeners.

Using these hooks means that an object has state that cannot be cleanly observed, serialized, or modified. That is what I mean when I say it is a "bad API". Not that hooks shouldn't be used, they're useful escape hatches for custom needs, but common situations (like modifying builtin materials) should eventually be supported through a non-imperative API. NodeMaterial is more promising for this purpose.

I'm willing to be convinced otherwise... I feel less strongly about that change lately. And in particular I want to be clear that I'm not saying this should be blocked until NodeMaterial is finished. Setting NodeMaterial aside, does it seem confusing for copy/clone vs serialization to have different results?

And in particular, what specific issues would this fix? Support cloning for glTF spec/gloss materials (which #14099 also does) and eventually the KHR_techniques_webgl extension? Is there any reason not to merge #14099?

[takahirox]

does it seem confusing for copy/clone vs serialization to have different results?

Maybe yes.

I think what we should first do is to make clear the .onBeforeCompile() specification. About material modified with .onBeforeCompile, which one users can expect

1. full functional (render, copy, clone, serialization, and so on) in the same API as built-in materials'
2. has a limitation, it can be rendered but the others could be done wrongly. They should be done in user side if users want correct result.

(The above is the background of question #11475 (comment))

My understanding is we go with 2. because the purpose of .onBeforeCompile() is an easy modification API with less core change. If it's right, I don't think we need this PR. But better to add a note about the limitation in the doc instead.

/cc @mrdoob

[pailhead]

What i've noticed about onBeforeCompile is that it seems to be used mostly as onBefore|Three|Parse. Since it happens before three.js actually produces valid GLSL. At this time you don't have access to valid GLSL (never?).

My understanding is we go with 2. because the purpose of .onBeforeCompile() is an easy modification API with less core change. If it's right, I don't think we need this PR. But better to add a note about the limitation in the doc instead.

I'd very much vote for this.

[pailhead]

Actually, i think this might still be relevant (deleted it once 😄 )

#13192

I think you might be able to create materials in such a way to run into this issue.

[mrdoob]

I vote 2 too.

[takahirox]

Can you help me to add a note to the doc? Please correct my writing. Does "extra work" sound vague?

.onBeforeCompile() won't be copied, cloned, and serialized with .copy(), .clone(), and toJSON(). This means built-in material modified with .onBeforeCompile() will be copied, cloned, or serialized as original built-in material. If you want to do them correctly, you need extra work in user side.

[Mugen87]

Docs updated via #17672