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

MeshLambertMaterial: convert to per-fragment shading #24452

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

WestLangley
Copy link
Collaborator

@WestLangley WestLangley commented Aug 5, 2022

This PR converts MeshLambertMaterial from Gouraud (per-vertex) shading to per-fragment shading.

  1. MeshLambertMaterial now supports:

    • normal maps
    • object-space normal maps
    • bump maps
    • displacement maps
    • tangents
    • flat-shading using screen-space derivatives
  2. Shadows are now correctly modeled as the absence of light, just as the other built-in materials that respond to light. The shadowMask approximation, which results in solid-black shadows, is no longer needed.

  3. No features have been removed. In particular, the legacy specularMap is still supported.

  4. It remains to see if per-vertex shading has a detrimental effect on mobile performance.

  5. As suggested by @mrdoob offline, I will create a custom ShaderMaterial that uses Lambert illumination and Gouraud (per-vertex) shading, and add it to the /examples/jsm/materials/ directory. It will have the same feature set as the legacy version of MeshLambertMaterial.

@WestLangley WestLangley added this to the r144 milestone Aug 5, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 5, 2022

It remains to see if per-vertex shading has a detrimental effect on mobile performance.

It would be interesting to see the performance implications of this PR. Meaning before it is merged.

I know from several users that they use MeshLambertMaterial in a mobile and VR context because MeshStandardMaterial as well as MeshPhongMaterial are slower.

@N8python
Copy link
Contributor

N8python commented Aug 5, 2022

I have conducted some tests with Lambert vs. Phong on a modern M1 Macbook Pro. Not exactly a low-end mobile gpu, but still good for testing performance. The benchmark was rendering a single torus not at 800x600 resolution with an pixel ratio of 10 to firmly test the speed of fillrate. MeshLambertMaterial was only 2% faster than MeshPhongMaterial.

Here is the scene rendered (for reference):
aha

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 5, 2022

Sorry, but such a simple scene on such a powerful device is not a viable test case. This needs to be primarily tested with low and mid-range smart phones with a fragment shader bound code (so it's possible to measure a difference between gouraud and phong shading)

@donmccurdy
Copy link
Collaborator

A few ideas on what might be needed to measure this:

  1. low-end mobile device
  2. selected material (lambert, phong, standard) filling the viewport at full resolution
  3. perhaps add 2-3x overdraw with several layers of transparent PlaneGeometries
  4. measure both framerate, and average time spent on .render(sceen, camera) per frame

@N8python
Copy link
Contributor

N8python commented Aug 5, 2022

Sorry, but such a simple scene on such a powerful device is not a viable test case. This needs to be primarily tested with low and mid-range smart phones with a fragment shader bound code (so it's possible to measure a difference between gouraud and phong shading)

This makes sense. Sorry for wasting your time!

@donmccurdy
Copy link
Collaborator

donmccurdy commented Aug 8, 2022

Sorry if this has been discussed somewhere else that I've missed. I believe users are choosing MeshLambertMaterial today because it has better performance than Phong on GPU-bound mobile devices, presumably because of the per-vertex vs. per-fragment shading.

Assuming we want to maintain both a per-fragment Lambert material and a per-vertex Gourad shader (#24467), we have several choices:

  • (A) Lambert in core, Gourad in examples/jsm
  • (B) Gourad in core, Lambert in examples/jsm
  • (C) Both in examples/jsm
  • (D) Both in core

For bundle size reasons, I doubt we want (D). If users are indeed looking for per-vertex shading, rather than a Lambert shading model, then I don't think (A) has much benefit. I would be fine with (B) or (C).

@WestLangley
Copy link
Collaborator Author

I have seen users use MeshLambertMaterial for the performance, others for the illumination model.

//

I am trying to make the shader code more maintainable. Part of that is removing per-vertex materials from Core. Hence, MeshLambertMaterial is converted to per-fragment. A side benefit is it is able to become more feature-rich.

Concurrently, per-vertex MeshGourardMaterial with Lambert illumination is added to the examples in #24467 for users who need a per-vertex material for mobile.

So, I'd say the current proposal would be considered as Option (E).

//

Option (F) would be to remove Lambert from Core and rename the newly-added MeshGourardMaterial in the examples to MeshLambertMaterial. In that case, we would just say: "MeshLambertMaterial has been moved to the examples".

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 8, 2022

TBH, I don't see the purpose of a separate per-fragment Lambert material. Why not directly using MeshPhongMaterial with zero specular?

I understand the library will be easier to maintain if no materials with per-vertex lighting are part of the core. However, I have the feeling a decision is made without enough information about the performance benefits of per-vertex lighting. Hence, I think it's important to make a performance analysis before this PR is merged.

@WestLangley
Copy link
Collaborator Author

Why not directly using MeshPhongMaterial with zero specular?

Because it is not the same, but MeshPhongMaterial could be modified so it is the same.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 8, 2022

Because it is not the same,

I what way would it be different? Sorry, I have not read the new lambert shader in detail yet.

@WestLangley
Copy link
Collaborator Author

Why not directly using MeshPhongMaterial with zero specular?

@Mugen87 Did you mean zero-shininess?

Because even if shininess is zero, the fresnel goes to 1 at grazing angles. So specular reflections still remain.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Aug 8, 2022

@WestLangley I think your (E) and my (A) are the same, right? I guess what I mean is, why keep Lambert in core at all, if it has primarily been used for performance of per-vertex shading and we're removing that "feature" from it? Why not move both to examples/jsm?

@WestLangley
Copy link
Collaborator Author

WestLangley commented Aug 8, 2022

@donmccurdy

I think your (E) and my (A) are the same, right?

Yes, if your (A) is referring to the new per-pixel Lambert with the normalMap support.

I guess what I mean is, why keep Lambert in core at all, if it has primarily been used for per-vertex shading

I think that is a false premise. Users use Lambert for the illumination model, too.

Why not move both to examples/jsm?

That is an option, too. But, instead, you could do Option (F), which is equivalent to just moving per-vertex Lambert from Core to the examples.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Aug 8, 2022

Ok, thanks @WestLangley! It sounds like a primary goal here is to simplify what we're maintaining in core, and to not have the additional complexity of a per-vertex shader there. Moving per-vertex to MeshGouradMaterial in examples/jsm, and having MeshLambertMaterial be per-fragment could be the most direct way to improve things. Many users probably assumed MeshLambertMaterial was per-fragment anyway — the naming was a bit confusing, and this PR fixes that.

I'm satisfied with keeping MeshLambertMaterial (now per-fragment) in core. 👍🏻

@mrdoob
Copy link
Owner

mrdoob commented Aug 9, 2022

Ok, thanks @WestLangley! It sounds like a primary goal here is to simplify what we're maintaining in core, and to not have the additional complexity of a per-vertex shader there. Moving per-vertex to MeshGouradMaterial in examples/jsm, and having MeshLambertMaterial be per-fragment could be the most direct way to improve things. Many users probably assumed MeshLambertMaterial was per-fragment anyway — the naming was a bit confusing, and this PR fixes that.

Exactly this 👍

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 9, 2022

Ok, thanks @WestLangley! It sounds like a primary goal here is to simplify what we're maintaining in core, and to not have the additional complexity of a per-vertex shader there. Moving per-vertex to MeshGouradMaterial in examples/jsm, and having MeshLambertMaterial be per-fragment could be the most direct way to improve things. Many users probably assumed MeshLambertMaterial was per-fragment anyway — the naming was a bit confusing, and this PR fixes that. I'm satisfied with keeping MeshLambertMaterial (now per-fragment) in core. 👍🏻

Okay, I'm convinced^^, too. Since MeshGouraudMaterial will be in place, users can fallback to this material if the shading performance actually becomes a problem.

@WestLangley
Copy link
Collaborator Author

Okay, I'm convinced^^, too.

Yay!

@mrdoob mrdoob merged commit 535b42d into mrdoob:dev Aug 10, 2022
@mrdoob
Copy link
Owner

mrdoob commented Aug 10, 2022

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Aug 10, 2022

I guess we no longer need this file?

https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/lights_lambert_vertex.glsl.js

@WestLangley
Copy link
Collaborator Author

I guess we no longer need this file?

We do need it. Gouraud replicates legacy Lambert.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 10, 2022

Maybe we remove the chunk but copy the code directly into GouraudShader? Better to keep only shader chunks in the core which are actually used by built-in materials.

@WestLangley WestLangley deleted the dev_per-fragment-lambert branch August 10, 2022 19:43
makc added a commit to makc/Edelweiss that referenced this pull request Aug 21, 2022
basically to support small lights, but this was going to happen eventually any way - see mrdoob/three.js#24452
abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* MeshLambertMaterial: per-fragment shading

* Remove unnecessary shader chunk
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 21, 2022
* MeshLambertMaterial: per-fragment shading

* Remove unnecessary shader chunk
makc added a commit to makc/Edelweiss that referenced this pull request Dec 4, 2022
basically to support small lights, but this was going to happen eventually any way - see mrdoob/three.js#24452
makc added a commit to makc/Edelweiss that referenced this pull request Dec 5, 2022
basically to support small lights, but this was going to happen eventually any way - see mrdoob/three.js#24452
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.

5 participants