Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce MeshGouraudMaterial #24467

Merged
merged 5 commits into from
Aug 11, 2022
Merged

Conversation

WestLangley
Copy link
Collaborator

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
Copy link
Collaborator Author

WestLangley commented Aug 8, 2022

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.)

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 8, 2022

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.

@WestLangley
Copy link
Collaborator Author

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.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 8, 2022

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

@WestLangley
Copy link
Collaborator Author

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...

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 8, 2022

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

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 8, 2022

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().

const envMap = properties.get( material ).envMap;
if ( envMap ) {
uniforms.envMap.value = envMap;
uniforms.flipEnvMap.value = ( envMap.isCubeTexture && envMap.isRenderTargetTexture === false ) ? - 1 : 1;
uniforms.reflectivity.value = material.reflectivity;
uniforms.ior.value = material.ior;
uniforms.refractionRatio.value = material.refractionRatio;
}

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.

@WestLangley
Copy link
Collaborator Author

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...

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 9, 2022

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.

@WestLangley
Copy link
Collaborator Author

@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 WestLangley added this to the r144 milestone Aug 9, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 10, 2022

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

if ( refreshMaterial || materialProperties.receiveShadow !== object.receiveShadow ) {
materialProperties.receiveShadow = object.receiveShadow;
p_uniforms.setValue( _gl, 'receiveShadow', object.receiveShadow );
}

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 10, 2022

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.

@WestLangley
Copy link
Collaborator Author

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

@mrdoob mrdoob merged commit 82c1a91 into mrdoob:dev Aug 11, 2022
@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2022

Thanks!

@WestLangley WestLangley deleted the dev_gouraud_material branch August 11, 2022 03:34
@WestLangley
Copy link
Collaborator Author

This PR requires a new build.

@mrdoob
Copy link
Owner

mrdoob commented Aug 12, 2022

Done!

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Introduce MeshGouraudMaterial

* Remove unnecessary shader chunk

* Support MeshGouraudMaterial envMap

* Inline lights_lambert_vertex and remove it from core
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 21, 2022
* Introduce MeshGouraudMaterial

* Remove unnecessary shader chunk

* Support MeshGouraudMaterial envMap

* Inline lights_lambert_vertex and remove it from core
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants