[BUG]: onBeforeCompile hashing

[pailhead]

Description of the problem

Go to https://codepen.io/anon/pen/KQPBjd. Comment line #41 and the other cube that stays in the scene will change the shader.

This is due to:

three.js/src/renderers/webgl/WebGLPrograms.js

Line 244 in 35a26f1

array.push( material.onBeforeCompile.toString() );

The same onBeforeCompile is provided to both materials, even though there is a condition that will process the shaders differently.

#13198 has a different approach but doesn't have this issue:
https://codepen.io/anon/pen/rJNgYZ

Three.js version

• Dev
• r89
• [ ]...

Browser

• All of them
• Chrome
• Firefox
• Internet Explorer

OS

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

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

[pailhead]

I looked into fixing this, but i'm not sure what to do. There is no way of knowing what's in the onBeforeCompile, other than it does it's own "compile". Since this only makes sense on materials, and not ShaderMaterials one could compile the onBeforeCompile and use that to hash, but seems a bit too hacky.

[Mugen87]

I've looked into this today and i wouldn't say it's a bug. More a feature request. The implementation of material.onBeforeCompile() is currently not designed to support dynamic scenarios like your if statement in the callback function. As you mentioned correctly, the renderer can't distinct between both materials since the program code is identical.

Workaround: It's easy to avoid the problem if you apply for each material a unique callback function.

Fix: https://codepen.io/anon/pen/xYyKjX?editors=0010

[pailhead]

True, but it’s suuuuuuupeer hacky. I use this feature a lot and it’s very cumbersome to work with. Is there any chance you could take a look at my PR and give some feedback?

With a couple more lines of code I think that this can become much more powerful and user friendly.

As is:

• can’t have logic in the callback
• have to know the each specific shader in order to know where to inject uniforms varyings and functions ( why can’t it be just uniform: { type, name } and then three knows what to do with it)
• very verbose
• I think one can’t use this to inject extension directives (they show up in the middle of the shader with a warning)

[pailhead]

is currently not designed to support dynamic scenarios like your if statement in the callback function.

I’d argue the semantics here :)

It was not designed to begin with, I think it was literally “hey this works with 3 lines of code” 🤣
Also, It does actually kinda work with my dynamic scenario - I’m getting multiple materials to use the same chunks and their subsets, it just requires a pretty inconvenient work around.

If I do however make a feature request do you think that this can be redesigned to be more friendly?

[Mugen87]

If I do however make a feature request do you think that this can be redesigned to be more friendly?

Making a feature request is a good idea. You should note your requirements/suggestions in short/summarised form. Besides, link to your PR that illustrates your idea of a concrete implementation.

I guess i would close this issue and create a new one with the mentioned content.

[mrdoob]

I'm having a hard time to figure out an approach that is not a hack to implement this over the current material system. The #11475 hack was included instead.

As I proposed in a previous meetup, I think we should study how to implement this on top of MeshNodeMaterial instead.

https://threejs.org/examples/?q=nodes#webgl_materials_nodes

[pailhead]

This might be a solution for this feature:

const _obc = shader => {...}
const obc = `
function (shader){
  const _${ hash() }
  ${ _obc.toString() }
  _obc(shader)
}
`
material.onBeforeCompile = eval(obc)

[pailhead]

It works

https://codepen.io/pailhead/pen/EGLNzJ