MeshBasicNodeMaterial: Refactor env map support. #28798
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
src/nodes/materials/NodeMaterial.js
Outdated
| @@ -423,6 +427,38 @@ class NodeMaterial extends Material { | |||
|
|
|||
| } | |||
|
|
|||
| // ENV MAP | |||
|
|
|||
| if ( this.environment === false ) { | |||
There was a problem hiding this comment.
I think we could add this operation in BasicLighthingModel removing this.environment, and extends PhongLightingModel from BasicLightingModel
There was a problem hiding this comment.
Okay, I rewritten the code like that!
One thing that can be optimized though (ideally with a different PR) is that NodeMaterial.setupLights() should only add an instance of EnvironmentNode for PBR materials. All other materials which do not support PMREM are incompatible with that type of node. Imo, it seems more clean if this node isn't created for that materials in the first place.
There was a problem hiding this comment.
I made a small revision. The idea of the LightingModel parameters would be equivalent to the defines that enable the specific modules of the light model, normally boolean, since we could access the material in the setup process.
Still, I have to do another review, ideally we should have a node like EnvironmentNode to handle this.
There was a problem hiding this comment.
Awesome, that looks much cleaner now!
Besides, I love these kind of reviews. They are ideal to learn the details of the new system^^.
There was a problem hiding this comment.
ideally we should have a node like EnvironmentNode to handle this.
Indeed. It would be nice if NodeMaterial could somehow setup a different type of EnvironmentNode depending on the type of material. That would automatically solve #28798 (comment).
There was a problem hiding this comment.
Still, I have to do another review, ideally we should have a node like EnvironmentNode to handle this.
I done this part! :)
There was a problem hiding this comment.
Great! Let's merge the current state. We can add more revisions in other PRs.
Related issue: #28795, #28785
Description
This is a more generic solution for #28795 which also covers
MeshLambertMaterialandMeshPhongMaterial.