Three.js: If only the sidedness changes, the material program is not recompiled

[erich666]

Description of the problem

If you call setFaceCulling https://github.com/mrdoob/three.js/blob/master/src/renderers/WebGLRenderer.js#L2409 and nothing else changes for the material, the material's vertex and fragment shader programs do not get recompiled. That is, material.needsUpdate does not get set to "true". Most shaders use the DOUBLE_SIDED and FLIP_SIDED defines, so this is a bug.

What should happen is that material.needsUpdate should be set to true when the double-sided or flip-sided state changes, so that the shader programs will reflect the changes.

This is admittedly a rare case, but I wanted to point it out, as it is a confusing one when encountered. That said, forcing a program recompile update may lead to extreme slowdowns, if you flip sidedness for an object every frame - you may want to keep using the original shader programs and indeed have sidedness affect only the culling state and/or vertex order. I don't have a good solution myself, so this is a bug report vs. a pull request.

Three.js version

• [x] Dev
• [ ] r77

• [ ] ...

  Browser

• [x] All of them

• [ ] Chrome
• [ ] Firefox

• [ ] Internet Explorer

  OS

• [x] All of them

• [ ] Windows
• [ ] Linux
• [ ] Android
• [ ] IOS
  

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

none

[WestLangley]

@erich666 What is your use case for which you are calling renderer.setFaceCulling() after the scene has been rendered?

[erich666]

Here's a typical use case: I add an option to my model viewer to turn
culling on or off. So I load a model, it starts out double-sided. I turn
culling on, so changing material.side. The shaders don't get recompiled.

On Mon, Jun 20, 2016 at 1:15 PM, WestLangley [email protected]
wrote:

@erich666 https://github.com/erich666 What is your use case for which
you are calling renderer.setFaceCulling() after the scene has been
rendered?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/mrdoob/three.js/issues/9179#issuecomment-227207429,
or mute the thread
https://github.com/notifications/unsubscribe/AAxEm4ooc8MXAqQhZhDiiLY7bB0rhuALks5qNsrIgaJpZM4I53Z2
.

[WestLangley]

I think you should be resetting material.side, instead.

[erich666]

Ah, right, my mistake. I guess I can do this:

material.side = THREE.FrontSide;
material.needsUpdate = true;

and do that for every material. It's too bad I need to manually set .needsUpdate, but I guess this is proper and expected?

[WestLangley]

You should not have to set the needsUpdate flag. Did you try it?

[erich666]

This took longer than I expected, but I made a standalone program showing the problem:

http://www.realtimerendering.com/erich/three_bug/examples/webgl_geometries_my2all.html

Run that and you'll see it start with backfaces only, then it switches to frontfaces only, and cycles back to backfaces, etc. You can change the period of cycling. It's pretty clear which one is showing backfaces, just look at a cube "rotate" and you'll see it go the other way, as it were - you're seeing the bottom of the cube from the inside.

The "set needsUpdate" box is the key thing here. If you turn it on, the only difference in the cycling pattern is that a material.needsUpdate = true; is done after every material.side = modification. It makes a difference for how things get rendered. Specifically, look at the top and bottom of the cube. These are both fully lit (you see only one or the other) when "set needsUpdate" is on. When off, one or the other is dark, because the shaders are not being updated each toggle.

So, clearly, needsUpdate must be called whenever "side" is changed in order for the "side" setting to fully take effect. Culling happens properly, but the shaders are not updated.

Basically, if backfaces are on when you turn "set needsUpdate" off, it'll use the backfaces-compiled version of the shader; if frontfaces are on when you turn this toggle off, it'll use the frontfaces version of the shader.

You can confirm this by debugging into three.js in this example and putting a break where initMaterial is called, around 15285 (on my machine, at least). This gets called when needsUpdate is set, it is not called when needsUpdate is not set.

A little confusing (hey, it took me hours to realize this was the problem), but think of it this way: if needsUpdate was _not_ needed, then toggling it on or off should have no effect. It does have an effect.

[erich666]

By the way, a simple fix is to update the documentation: "If you change the setting of .side, you usually want to also set .needsUpdate to true". In fact, I highly recommend this as the fix.

The problem is that there are times when you really don't care or even want to change the shader. For example, switching from double sided to front sided the shader results basically look the same, as the double-sided shader is a slightly less-efficient version of the front-face shader. However, if you started with the front-face shader as the one compiled, you probably _do_ want to recompile when going to the double-sided shader, since backfaces will render differently if you use the front-face shader for double-sided objects (the surface normals for backfaces won't get flipped).

[WestLangley]

the surface normals for backfaces won't get flipped

@eric666 You are correct. : - )

The normals are not being updated for the reason you mentioned -- their definition is inside a #define. material.side is not a uniform.

I think it would be fair to say that if material.side is changed, one should set material.needsUpdate = true. Perhaps in the case of MeshBasicMaterial, it may not be necessary.

[WestLangley]

material.alphaTest is another property that requires setting material.needsUpdate = true when it is changed.

[looeee]

@WestLangley should I update the docs to say this?

[WestLangley]

@WestLangley should I update the docs to say this?

@looeee Yes, please. :)

[looeee]

@WestLangley it would be pretty straightforward to write a setter for Material.side that sets material.needsUpdate = true whenever it is changed.

That would give the default behaviour that most people want rather than expecting them to notice an obscure note in the docs for behaviour which, as @eric666 noted above, is quite hard to spot and pin down in the first place.

[WestLangley]

it would be pretty straightforward to write a setter for Material.side that sets material.needsUpdate = true whenever it is changed.

We could do it for alphaTest, too.

Thing is, if after the first render, the user does this, for example

mesh.material.map = null;

the needsUpdate flag also must be set. There are other examples where changing uniforms requires setting the flag.

I am not sure of the best solution, here.

[looeee]

Hmm... my feeling on this is that it should be automatic whenever that is what the user would reasonably expect. Changing the sidedness should 'just work', if possible, rather than leading to hard to pin down bugs.
The same for material.alphatest, and any other places where simple setter functions can be created.

Ideally the user should never have to worry about this flag, unless they have some advanced use case where they want to set it to false, in which case they can still do so manually.

For material.map the following setter should work.

set map( value ) { 
    //this will account for null, undefined or any other falsy value
    if ( ! value ) { 
        this.needsUpdate = true;
    }
}

[mrdoob]

Agreed. And I like the setter approach.

[WestLangley]

If only the sidedness changes, the material program is not recompiled.

This is now automatic.