three.js icon indicating copy to clipboard operation
three.js copied to clipboard

Introduce MeshGouraudMaterial

Open WestLangley opened this issue 3 years ago • 10 comments

Related issue: #24452.

This PR adds MeshGouraudMaterial.js to the the newly-created /examples/jsm/materials/ directory.

MeshGouraudMaterial implements a Lambert illumination model with Gouraud (per-vertex) shading.

MeshGouraudMaterial has the same feature set, and uses the same shaders, as the legacy version of MeshLambertMaterial.

WestLangley avatar Aug 08 '22 03:08 WestLangley

Current limitations:

  1. Equirectangular environment maps do not work for this custom material. There is apparently a conflict when the renderer auto-converts the equirectangular map to a cube map.

     WebGL: INVALID_OPERATION: bindTexture: textures can not be used with multiple targets
    

AFAIK, there are no other limitations.

@Mugen87 I expect the getter/setter for envMap is related to the problem, because built-in materials do not use such a pattern. (Actually, maybe built-in materials should use it, as it makes the renderer more material-agnostic. I think the use of the pattern in built-in materials would eliminate the refreshUniformsFoo pattern in WebGLRenderer. But that's a separate issue.)

WestLangley avatar Aug 08 '22 03:08 WestLangley

I expect the getter/setter for envMap is related to the problem, because built-in materials do not use such a pattern.

Um, adding the getter should actually resolve the issue since the renderer can now access the common envMap material property. The shader materials in WebGLBackground do it like that.

Mugen87 avatar Aug 08 '22 08:08 Mugen87

The shader materials in WebGLBackground do it like that.

Oh, see #15187. FWIW, I think that pattern is no longer needed, because the code injection (getTexelDecodingFunction()) was removed.

WestLangley avatar Aug 08 '22 10:08 WestLangley

Good news! However, if you want automatic cube map conversion for MeshGouraudMaterial, you still need the envMap property on material level.

Mugen87 avatar Aug 08 '22 10:08 Mugen87

Good news! However, if you want automatic cube map conversion for MeshGouraudMaterial, you still need the envMap property on material level.

Right. It's main purpose was to enable automatic uniform updates, because the renderer does not do that for custom materials.

//

BTW, this was really hard to get working... I'd hate to ask users to have to do this. That is why I created an actual material -- not just a shader.

In any event, for whatever reason, automatic equirectangular conversion for (at least) this non-built-in material is not working, and I have no idea why...

WestLangley avatar Aug 08 '22 10:08 WestLangley

I'll download your branch and debug offline. Maybe I find something.

Mugen87 avatar Aug 08 '22 14:08 Mugen87

The problem is that the shader of MeshGouraudMaterial expects a cube map, however, the renderer still uploads the equirectangular texture extracted from material.envMap. This does not happen for built-in materials because of the following section in refreshUniformsCommon().

https://github.com/mrdoob/three.js/blob/9e5512f067e3a0de8d4069217d20903c1725edf7/src/renderers/webgl/WebGLMaterials.js#L172-L184

As you can see the code correctly configures the uniforms with the environment map from WebGLCubeMaps before the uniform upload happens. But that works only for built-in materials.

So adding the envMap property is unfortunately not enough for custom shaders. Something like #18399 would potentially solve this issue. Or we just force ShaderMaterial to go through refreshUniformsCommon() when e.g. a new flag is enabled or something.

Mugen87 avatar Aug 08 '22 14:08 Mugen87

Thanks for your sleuthing, @Mugen87. :-)

As you can see the code correctly configures the uniforms with the environment map from WebGLCubeMaps before the uniform upload happens. But that works only for built-in materials.

So it's a sequencing thing...

WestLangley avatar Aug 08 '22 16:08 WestLangley

In order to get things going without breaking the equirect support, I suggest to evaluated isMeshGouraudMaterial in the renderer for now which triggers the environment map update. The code could be placed below this line and look like so:

if ( material.isMeshGouraudMaterial ) {

    m_uniforms.envMap.value = envMap; 
  
    m_uniforms.flipEnvMap.value = ( envMap.isCubeTexture && envMap.isRenderTargetTexture === false ) ? - 1 : 1; 

}

We can refactor this later.

Mugen87 avatar Aug 09 '22 07:08 Mugen87

@Mugen87 Ugh... You have to also test for null in your code snippet, but even doing that, it is rendering reflected on the first render for me, and then corrects subsequently. (tip: don't test with an animation loop). So there is still a sequencing problem -- apparently...

I agree, it would be nice if the app does not have to set flipEnvMap. Currently I have to expose that property in this PR.

WestLangley avatar Aug 09 '22 12:08 WestLangley

Sorry, the code block is executed too late. Can you please move it below this if block:

https://github.com/mrdoob/three.js/blob/d80e640a2990277a942b9ea43f1ea4acdb04daec/src/renderers/WebGLRenderer.js#L1730-L1735

Mugen87 avatar Aug 10 '22 12:08 Mugen87

PR looks good to me now!

What do you think about using the new material in at least one example? Maybe webgl_materials_envmaps? It would be also okay to create a new example like webgl_materials_gouraud. Separate PR if you prefer.

Mugen87 avatar Aug 10 '22 20:08 Mugen87

OK, in a separate PR... Let me recover from this one first. :-)

WestLangley avatar Aug 10 '22 20:08 WestLangley

Thanks!

mrdoob avatar Aug 11 '22 03:08 mrdoob

This PR requires a new build.

WestLangley avatar Aug 11 '22 22:08 WestLangley

Done!

mrdoob avatar Aug 12 '22 02:08 mrdoob