[Enhancement] onBeforeCompile may break material's .clone() method

[pailhead]

Description of the problem

onBeforeCompile seems to be ignored when cloning materials:

https://codepen.io/anon/pen/yjPLrJ?editors=1010

Expected result is equal amount of yellow and red cubes.

Proposing to either:

1. consider Material.onBeforeCompile when cloning
2. document that there should be no cloning happening after setting this callback
3. document that .clone() is non deterministic, it's going to be a different clone depending on the presence of onBeforeCompile

Three.js version

• Dev
• r92
• ...

Browser

• All of them
• Chrome
• Firefox
• Internet Explorer

OS

• All of them
• Windows
• macOS
• Linux
• Android
• iOS

Hardware Requirements (graphics card, VR Device, ...)

[donmccurdy]

^Same applies to materials relying on mesh.onBeforeRender for updates, per #11573. When users have added the .onBeforeFoo hook themselves it's not so bad — they can probably realize pretty intuitively that the callback has to be copied over manually. But it's less obvious when the hook is added by a loader, e.g. GLTFLoader needs this to properly render spec/gloss PBR materials, and the resulting models don't "just work" when you clone them.

I'm not sure what solution makes sense here... Copying callbacks when cloning an object feels unintuitive, e.g. the DOM doesn't do this. More flexible ways to customize materials would certainly reduce the need for these hooks — maybe node-based materials, allowing custom nodes, are the (long-term) solution?

[pailhead]

I think mesh.onBeforeRender is slightly different, you could call it off the parent in some order. onBeforeCompile also hashes the materials, making this a lot more tricky. ( i might be wrong )
#13192

[pailhead]

I was going to say, it sounded like an issue of something that probably shouldnt be in onBeforeRender in the first place. Upon investigation, it looks exactly like it.

three.js/examples/js/loaders/GLTFLoader.js

Line 775 in 04e86d0

refreshUniforms: function ( renderer, scene, camera, geometry, material, group ) {

I will continue this conversation in the other issue.

[pailhead]

Given the example of gltf loader limitations, i would advise against trying to implement complex effects with onBeforeCompile.

[donmccurdy]

As of 2015 there was the idea that NodeMaterial might eventually be in src/*... I guess I'm wondering whether we should try to keep improving hooks for replacement into shaders and onBeforeCompile, or whether we should be spending time on that instead. For example, implement glTF's spec/gloss materials as THREE.NodeStandardSGMaterial.

[WestLangley]

I guess I'm wondering whether we should try to keep improving hooks for replacement into shaders and onBeforeCompile, or whether we should be spending time on that instead. For example, implement glTF's spec/gloss materials as THREE.NodeStandardSGMaterial.

I agree, focusing on NodeMaterial would be a much better use of our time -- and we could follow that up with a graphical API to go with it.

[pailhead]

Also, can you please stay on the topic of the issue? What is to be done in this particular case when I want to clone something that has onBeforeCompile and should the docs give me any additional information.

this thing from 2015 we’re still waiting for will fix it

Should it cut it?

[pailhead]

I guess I'm wondering whether we should try to keep improving hooks for replacement into shaders and onBeforeCompile, or whether we should be spending time on that instead.

What are you wondering, can you share some thoughts?

[donmccurdy]

For this particular case, I don't think .onBeforeCompile should be copied when cloning an object. I won't object to documentation explaining that .onBeforeCompile won't be copied, but I also don't think it will help many people — if you know that .onBeforeCompile has been added to your material, you can probably figure out pretty quickly that it doesn't persist after cloning. It's the users who don't know what .onBeforeCompile is who are going to struggle, and I don't see how we solve that. Hooks make a good escape hatch, but a poor API.

More generally (and, yes, arguably off-topic 😅) if we knew that having a node-based material system in src/* really was an eventual goal, then I think we would approach this issue (and several others, like #12608) differently. Not just waiting for a thing from 2015 — there are plenty of active contributors to three.js, and if we know how NodeMaterial fits into the roadmap we can spend our time helping sunag to build it.

[pailhead]

Could you elaborate a bit on what hooks mean to you and why are they poor api?

Scenekit has something like his.

[pailhead]

@donmccurdy

I'm not sure what solution makes sense here... Copying callbacks when cloning an object feels unintuitive, e.g. the DOM doesn't do this.

I'm not sure if i agree with this.

class WriteToStencilMesh extends Mesh {
  constructor( geometry, material, stencil_id ) {
    super( geometry, material )
    this.onBeforeRender = renderer =>{
      const gl = renderer.getContex()
      gl.enable(gl.STENCIL_TEST)
      gl.stencilFunc(gl.ALWAYS, 1, 0xff)
      gl.stencilOp(gl.KEEP, gl.KEEP, gl.REPLACE)
    }
    this.onAfterRender = renderer =>{
      gl.disable(gl.STENCIL_TEST)
    }
  }
}

It is not totally senseless to not carry this over somehow when cloning. Otherwise i'd be confused by this:

myWriteToStencilMesh = new WriteToStencilMesh(geom,material, 5) 

myWriteToStencilMeshClone = myWriteToStencilMesh.clone()

It's not really a clone of WriteToStencilMesh, if it quacks like a Mesh.

Hooks make a good escape hatch, but a poor API.

---

const m = new ShaderMaterial({
  uniforms:{
    uFoo: { value: 0 }
  }
}

1. i know that webgl shaders have to be compiled, so whatever the glsl in here, will compile (first render)
2. i know that the uniforms need to be refreshed, in order to set the proper webgl state (render)
3. I know that three.js takes care of both 1 and 2 so i dont have to, and it does it in render() (blackbox)

---

const m = new StandardMaterial({ roughness: 0 })

1. same, it compiles but i don't care
2. same. It's not a uniform, but the renderer eventually stores it as a uniform and refreshes it at the same time as it does the ShaderMaterial (or same stage in render)
3. same

---

const m  = new StandardMaterial()
renderer.render(scene,cam)

m.map = texture
m.needsUpdate = true //compile thing
renderer.render(scene,cam)

I'm not sure if it matters at what time specifically the compilation happens nor how much do i care. Even though i do care about some compilation (or recompilation) i express it only through this dirty flag, and have some knowledge that the next render() is going to take care of this.

---

I'd say most users stop at you have to set needsUpdate to true, on line XX
Some users are aware that a recompilation needs to happen and why you need to set it to true.
A few users are aware of the intricacies involved and what actually happens on the webgl level.

As is, all of these users are in the same basket.

1. They have to instantiate their materials, however abstract or low level they are.
2. They have to render in order to compile them
3. They have to set needsUpdate to true, under certain conditions

All of their uniforms are treated the same by the WebGLRenderer. It will set them up, refresh them etc. ShaderMaterial users just declare the uniforms, and mutate them, StandardMaterial users create those same uniforms under the hood.

All of the users expect any intance of the Material to be able to be cloned, and it does not impact the flow from above. Still gets recompiled and compiled by the renderer as needed, hidden from the user, still gets refreshed, also hidden from the user. The work happens in javascript, in between webgl calls. Three hides this.

then we got

const m = new StandardMaterial()
m.onBeforeCompile = lord_save_us

My mind melts from what's going on in here. The same type of data that i use to as a uniform above, or as roughness now has to jump through hoops to be updated.

class M extends StandardMaterial {
  constructor(params){
    super(params)
    this.onBeforeCompile = shader =>{
       shader.uniforms.uFoo = { value: 0 }
       this.___myUniforms = { uFoo: shader.uniforms.uFoo } // <-- probably not a good idea
       //maybe
       //shader.uniforms.uFoo = this.userData.myUniforms.uFoo
       shader.vertexShader = shader.vertexShader... // the whole replace thing
    }
    //maybe this?
    this.userData.myUniforms = { uFoo: { value: 0 } }
  }
  set foo ( val ) { 
    //if(this.___myUniforms.uFoo) this.___myUniforms.uFoo.value = val
    this.userData.myUniforms.uFoo.value = val
  }
}

Without the onBeforeCompile my foo is meaningless. Even if i'm able to set some uniform, i need the blackbox to wire it. Why can't we have the same convenience of the first two examples?

With ShaderMaterial i declare a uniform, three handles how it's hooked and refreshed.
With StandardMaterial i declare a property, three turns it into a uniform, and handles how it's hooked and refreshed.

With onBeforeCompile I also want to declare some uniform, but three suddenly gets very opinionated, where i declare it, when i declare it, whether i'm allowed to keep it etc.
It feels it changes the interface to the material. While needsUpdate flag is the only touch point with compilation, here i have to provide logic, and manipulate a string directly on top of that for that moment in time. I care a bit less about what uses a bool and when, compared to when a function gets called. Is this way of thinking reasonable?

How worse is the hook api that you have in mind?

const myUniforms = {}

const shm = new ShaderMaterial({
  uniforms,
  vertexShader,
  fragmentShader,
}

//extend shm
Objects.assign(shm.uniforms, myUniforms)

const stm = new StandardMaterial()

//extend stm
stm.uniforms = myUniforms

^ let three take care of the uniforms, since it already knows how to treat the same api with the ShaderMaterial

@WestLangley
Imagine if you didn't have to do all this copy pasting 😛 . I'm too lazy, plus it trims some 100 lines of code:

three.js/examples/webgl_buffergeometry_instancing_lambert.html

Line 61 in d953920

customDepthRGBA: { // this is a cut-and-paste of the depth shader -- modified to accommodate instancing for this app

Imagine not having to redeclare things like this. The renderer already does it for LambertMaterial why do it again? Why are you in the ShaderMaterial at all here:

three.js/examples/webgl_buffergeometry_instancing_lambert.html

Line 318 in d953920

// The renderer does not set these defines for ShaderMaterial

This seems like a great example for onBeforeCompile since you want to change just some chunks from the Lambert material but you had to copy over a bunch of templates.

Why can't this be a familiar interface of the lambert material, with some fragments of data stored under some keys that no one ever looks at?

[pailhead]

An interesting find is that copying a StandardMaterial just straight up nukes the defines, that could be otherwise used with monkey-patched THREE.ShaderChunk.

https://github.com/mrdoob/three.js/blob/dev/src/materials/MeshStandardMaterial.js#L123

[pailhead]

I found this:

https://en.wikipedia.org/wiki/Cloning_(programming)

In computer science, cloning refers to the making of an exact copy of an object

"Exact copy" does this mean the object should behave like the original?

If you copy a robot into a wax statue, they might look exactly the same, but the robot can move, shoot lazers and stuff, not sure if this would qualify for cloning :)

So this makes clone not a clone or not a deterministic clone? It works with 99% of the Material API but there's a gotcha, one method will break it, if you use it, clone no longer clones.

[donmccurdy]

When extending a builtin material as you are doing, and adding an onBeforeFoo callback in the constructor, you should also be extending the clone or copy function to copy that callback if you intend for it to be preserved.

"Exact copy" does this mean the object should behave like the original?

No — 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.