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.

Sign up for GitHub

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

Jump to bottom

MeshBasicNodeMaterial: Refactor env map support. #28798

Merged
merged 8 commits into from
Jul 5, 2024
Merged

MeshBasicNodeMaterial: Refactor env map support. #28798

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
17b214d
MeshBasicNodeMaterial: Refactor env map support.
Mugen87 Jul 3, 2024
1d88016
MeshBasicNodeMaterial: Move env map logic to lighting model.
Mugen87 Jul 4, 2024
886959a
NodeMaterial: Clean up.
Mugen87 Jul 4, 2024
e29b391
MeshLambertNodeMaterial: Fix creation of lighting model.
Mugen87 Jul 4, 2024
35d7a14
Update MeshLambertNodeMaterial.js
Mugen87 Jul 4, 2024
b120a02
update
sunag Jul 4, 2024
7bd69c8
cleanup
sunag Jul 4, 2024
c794ccc
revision
sunag Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/nodes/functions/BasicLightingModel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import LightingModel from '../core/LightingModel.js';
import { diffuseColor } from '../core/PropertyNode.js';

class BasicLightingModel extends LightingModel {

constructor() {

super();

}

finish( context ) {

context.outgoingLight.rgb.assign( diffuseColor.rgb ); // TODO: Optimize LightsNode to avoid this assignment

}

}

export default BasicLightingModel;
36 changes: 4 additions & 32 deletions src/nodes/materials/MeshBasicNodeMaterial.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import NodeMaterial, { addNodeMaterial } from './NodeMaterial.js';
import { diffuseColor } from '../core/PropertyNode.js';
import { materialSpecularStrength, materialReflectivity } from '../accessors/MaterialNode.js';
import { MeshBasicMaterial } from '../../materials/MeshBasicMaterial.js';
import { MultiplyOperation, MixOperation, AddOperation } from '../../constants.js';
import { mix } from '../math/MathNode.js';
import BasicLightingModel from '../functions/BasicLightingModel.js';

const defaultValues = new MeshBasicMaterial();

Expand All @@ -15,7 +12,7 @@ class MeshBasicNodeMaterial extends NodeMaterial {

this.isMeshBasicNodeMaterial = true;

this.lights = false;
this.lights = true;
//this.normals = false; @TODO: normals usage by context

this.setDefaultValues( defaultValues );
Expand All @@ -24,34 +21,9 @@ class MeshBasicNodeMaterial extends NodeMaterial {

}

setupVariants( builder ) {

const envNode = this.getEnvNode( builder );

if ( envNode !== null ) {

switch ( this.combine ) {

case MultiplyOperation:
diffuseColor.assign( mix( diffuseColor.rgb, diffuseColor.rgb.mul( envNode.rgb ), materialSpecularStrength.mul( materialReflectivity ) ) );
break;

case MixOperation:
diffuseColor.assign( mix( diffuseColor.rgb, envNode.rgb, materialSpecularStrength.mul( materialReflectivity ) ) );
break;

case AddOperation:
diffuseColor.addAssign( envNode.rgb.mul( materialSpecularStrength.mul( materialReflectivity ) ) );
break;

default:
console.warn( 'THREE.MeshBasicNodeMaterial: Unsupported .combine value:', this.combine );
break;

}

}
setupLightingModel( /*builder*/ ) {

return new BasicLightingModel();

}

Expand Down
2 changes: 2 additions & 0 deletions src/nodes/materials/MeshStandardNodeMaterial.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ class MeshStandardNodeMaterial extends NodeMaterial {

this.isMeshStandardNodeMaterial = true;

this.environment = true;

this.emissiveNode = null;

this.metalnessNode = null;
Expand Down
50 changes: 43 additions & 7 deletions src/nodes/materials/NodeMaterial.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { Material } from '../../materials/Material.js';
import { NormalBlending } from '../../constants.js';
import { NormalBlending, MultiplyOperation, MixOperation, AddOperation } from '../../constants.js';

import { getNodeChildren, getCacheKey } from '../core/NodeUtils.js';
import { attribute } from '../core/AttributeNode.js';
import { output, diffuseColor, varyingProperty } from '../core/PropertyNode.js';
import { materialAlphaTest, materialColor, materialOpacity, materialEmissive, materialNormal } from '../accessors/MaterialNode.js';
import { materialAlphaTest, materialColor, materialOpacity, materialEmissive, materialNormal, materialSpecularStrength, materialReflectivity } from '../accessors/MaterialNode.js';
import { modelViewProjection } from '../accessors/ModelViewProjectionNode.js';
import { transformedNormalView, normalLocal } from '../accessors/NormalNode.js';
import { instance } from '../accessors/InstanceNode.js';
Expand Down Expand Up @@ -45,6 +45,8 @@ class NodeMaterial extends Material {
this.lights = true;
this.normals = true;

this.environment = false;
Mugen87 marked this conversation as resolved.
Show resolved Hide resolved

this.lightsNode = null;
this.envNode = null;
this.aoNode = null;
Expand Down Expand Up @@ -354,15 +356,17 @@ class NodeMaterial extends Material {

setupLights( builder ) {

const envNode = this.getEnvNode( builder );
const materialLightsNode = [];

//
if ( this.environment === true ) {

const materialLightsNode = [];
const envNode = this.getEnvNode( builder );

if ( envNode ) {
if ( envNode ) {

materialLightsNode.push( new EnvironmentNode( envNode ) );
materialLightsNode.push( new EnvironmentNode( envNode ) );

}

}

Expand Down Expand Up @@ -423,6 +427,38 @@ class NodeMaterial extends Material {

}

// ENV MAP

if ( this.environment === false ) {
Copy link
Collaborator

@sunag sunag Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could add this operation in BasicLighthingModel removing this.environment, and extends PhongLightingModel from BasicLightingModel

Copy link
Collaborator Author

@Mugen87 Mugen87 Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@Mugen87 Mugen87 Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, that looks much cleaner now!

Besides, I love these kind of reviews. They are ideal to learn the details of the new system^^.

Copy link
Collaborator Author

@Mugen87 Mugen87 Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, I have to do another review, ideally we should have a node like EnvironmentNode to handle this.

I done this part! :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Let's merge the current state. We can add more revisions in other PRs.


const envNode = this.getEnvNode( builder );

if ( envNode ) {

switch ( this.combine ) {

case MultiplyOperation:
outgoingLightNode = mix( outgoingLightNode, outgoingLightNode.mul( envNode.rgb ), materialSpecularStrength.mul( materialReflectivity ) );
break;

case MixOperation:
outgoingLightNode = mix( outgoingLightNode, envNode.rgb, materialSpecularStrength.mul( materialReflectivity ) );
break;

case AddOperation:
outgoingLightNode = outgoingLightNode.add( envNode.rgb.mul( materialSpecularStrength.mul( materialReflectivity ) ) );
break;

default:
console.warn( 'THREE.NodeMaterial: Unsupported .combine value:', this.combine );
break;

}

}

}

// EMISSIVE

if ( ( emissiveNode && emissiveNode.isNode === true ) || ( material.emissive && material.emissive.isColor === true ) ) {
Expand Down