Three.js: [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
• [x] r92
• [ ] ...

Browser

• [x] All of them
• [ ] Chrome
• [ ] Firefox
• [ ] Internet Explorer

OS

• [x] 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 https://github.com/mrdoob/three.js/issues/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 )
https://github.com/mrdoob/three.js/issues/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.

https://github.com/mrdoob/three.js/blob/04e86d0e9f7f4dafffc134064c36049a7368deca/examples/js/loaders/GLTFLoader.js#L775

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 https://github.com/mrdoob/three.js/issues/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:

https://github.com/mrdoob/three.js/blob/d953920c628f6a571e785d6a260c4af9d5119ca6/examples/webgl_buffergeometry_instancing_lambert.html#L61

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:
https://github.com/mrdoob/three.js/blob/d953920c628f6a571e785d6a260c4af9d5119ca6/examples/webgl_buffergeometry_instancing_lambert.html#L318

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.

[pailhead]

Thank you for providing details.

I'm still not convinced that onBeforeCompile compares that easily to a generic callback. You ever do only one thing in onBeforeCompile. Am i just being stupid or this is a bit confusing? Does the dom consider attached callbacks when caching internally as three.js does (onBeforeCompile.toString)?

NodeMaterial is more promising for this purpose.

MaterialIncludes available now, are not?

Is there a reason why one wouldn't want to clone this? Is this non-imperative (declarative?)?

myMaterial.extraInput = { foo: {value: primitive } }
myMaterial.extraLogic = { bar: string }

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'm suggesting that onBeforeCompile does only BAR, and as such should not be an event callback. Why can't i do the same with just declaring things, rather than writing callbacks? (I can with material-includes).

When will this callback ever follow a pattern other than:

materia.onBeforeCompile = shader=>{
  shader.uniforms[ uName].value = uValue
  shader[`${vertex || fragment}Shader`] = shader[`${vertex || fragment}Shader`].replace(`#include <${chunkName}`, chunk}
}

What would warrant something else being in there? What other kind of operation would you do that i not similar to decorating/mutating the ShaderMaterial? Other than these two lines repeated over and over again, i don't see a good reason to write anything else in there. Given the limitations of toString() i wouldn't write any logic, branching nothing. I can't make any of these conclusions about an EventDispatcher's event.

//before compile time

var sm = new ShaderMaterial()
sm.uniforms.foo = { value : foo }  // i want to add a uniform

var clone = sm.clone() // copies over uniform

var ss = new StandardMaterial()
sm.onBeforeCompile = addUniform // i want to add a uniform

var ssClone = ss.clone() // all i want is that stupid uniform here

Why do i have a uniform available in the ShaderMaterial to be copied over at before compile time but i do not have the onBeforeCompile uniform? Am i not in the before compile at this moment in code? If this is the imperative way and it's wrong, is there a simpler alternative to onBeforeCompile that could fill the void before NodeMaterial, and what's wrong with material-includes?

If i only use a callback to declare a uniform, why am i forced to use a callback in the first place or wait for the NodeMaterial overhaul, when the ShaderMaterial api is much more convenient? Why introduce the limitation that you're describing, into this process - "declare a uniform"?

Digesting what you said makes it sound like no documentation here is even necessary. "Like other callbacks, this does not get cloned" sounds like it should be known already just from the context of javascript (and or DOM?).

[donmccurdy]

Digesting what you said makes it sound like no documentation here is even necessary.

I'm fine with documenting this. I just don't think documentation solves #11573, because users trying to clone an object probably have no idea (1) what a glTF extension is, (2) that one particular extension creates meshes with a custom material and others do not, and (3) that these custom materials involve .onBeforeRender and that the callback must be copied manually. There's nowhere we can put that documentation to really help, because users expect (very reasonably) that once they've loaded an object, they can forget about the file format used to load it.

In any case, although we seem to disagree on it, please feel free to open a PR to copy .onBeforeFoo or document the omission. It's a subjective question and ultimately others will need to weigh in. :)

[pailhead]

and (3) that these custom materials involve .onBeforeRender and that the callback must be copied manually.

I don't think that this should be the holy word. I've suggested an implementation that does it without the onBeforeRender.