Three.js: Crash setting .lights to false for MeshStandardMaterials

[ghost]

Description of the problem

When I set .lights to false on a MeshStandardMaterial the renderer crashes setting up the uniforms for the material.

Here is an example taken from the MeshStandardMaterial demo in the three documentation.

https://jsfiddle.net/cleveard/xyu5tav2/

I am not sure what is happening, but the shader is being created with the lighting chunks, but the uniform values aren't being initialized because material.lights is false.

Three.js version

• [ ] Dev
• [x] r91
• [x] r90

Browser

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

OS

• [ ] All of them
• [ ] Windows
• [x] macOS
• [ ] Linux
• [ ] Android
• [ ] iOS

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

[WestLangley]

The.lights property is not a property that can be changed for that material. We should probably do something about that.

What is your use case for setting it to false?

[ghost]

It is mostly laziness with a dash of incomplete documentation. I have some other materials that can use image based lighting with or without lights and we use .lights to control whether lights are used or not. I noticed that changing the .lights property of the mesh standard material crashed and that the documentation didn't say that .lights was read-only. I was wondering what was up.

Can the .lights property of any of the materials be changed, or is it strictly an indicator to the renderer about whether the shader supports lights?

[WestLangley]

Can the .lights property of any of the materials be changed

The .lights property can be changed for ShaderMaterial and RawShaderMaterial only.

is it strictly an indicator to the renderer about whether the shader supports lights?

It is that, too.

[Mugen87]

Duplicate of #10491

[mrdoob]

@Mugen87 @WestLangley

Have we seen anyone sharing a compelling example for material.lights to exist?

Seems to me that it's an "abstraction leak" from WebGLRenderer:

https://github.com/mrdoob/three.js/blob/dev/src/renderers/WebGLRenderer.js#L1604
https://github.com/mrdoob/three.js/blob/dev/src/renderers/WebGLRenderer.js#L1670
https://github.com/mrdoob/three.js/blob/dev/src/renderers/WebGLRenderer.js#L1854

We could achieve the same by adding this to WebGLRenderer:

var usesLights = material.isMeshLambertMaterial || material.isMeshPhongMaterial || material.isToonMaterial || material.isStandardMaterial;

[WestLangley]

Add ShadowMaterial to your list?

So, .lights would only be a property of ShaderMaterial? Unless I am missing something, that sounds like a good idea to me.

[Mugen87]

I would love to see this issue fixed since users frequently think it's okay to configure .lights on app level.

Yeah, defining .lights in ShaderMaterial and enhancing WebGLRenderer seems a proper solution to me.

[mrdoob]

Add ShadowMaterial to your list?

Yes!

So, .lights would only be a property of ShaderMaterial? Unless I am missing something, that sounds like a good idea to me.

Ah, yeah, I guess that's the only case it's actually useful.

var usesLights = material.isMeshLambertMaterial || material.isMeshPhongMaterial || 
    material.isToonMaterial || material.isStandardMaterial || material.isShadowMaterial ||
    ( material.isShaderMaterial && material.lights === true );

[Mugen87]

How about make a new function in WebGLRenderer and use it in the mentioned places? Something like:

function materialNeedsLights( material ) {

    return material.isMeshLambertMaterial || material.isMeshPhongMaterial || 
    material.isToonMaterial || material.isStandardMaterial || material.isShadowMaterial ||
    ( material.isShaderMaterial && material.lights === true );

}

The notation is similar to the functions in WebGLTexture (e.g. textureNeedsPowerOfTwo()).

[Mugen87]

BTW: The test for material.isToonMaterial is actually not necessary since it's derived from MeshPhongMaterial.
https://jsfiddle.net/9hL5p6q7/

[WestLangley]

I thought we wanted to make WebGLRenderer material agnostic... That is, avoid handling built-in materials as special cases.

[Mugen87]

This could be achieved at a later point, too. The proposed refactoring would be a good solution in order to finally solve the .lights issue.

[mrdoob]

Yeah, step by step. @Mugen87 That method looks good!

[Mugen87]

Here is a PR 🎉 #17570