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

WebGLNodes: Uniform instance node example #22504

Merged
merged 2 commits into from
Sep 10, 2021
Merged

WebGLNodes: Uniform instance node example #22504

merged 2 commits into from
Sep 10, 2021

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Sep 8, 2021

Related issue: #21305

Description

Example of how change the uniforms values per object preserving the same material and nodes.

image

this.inputNode.value.copy( meshColor );

// force refresh material uniforms
rendererState.useProgram( null );
Copy link
Collaborator

@Mugen87 Mugen87 Sep 8, 2021

Choose a reason for hiding this comment

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

Um, it's not ideal that WebGLState.useProgram() is used in the example code. The method is considered to be private and should not be used outside of the renderer.

I guess we need a different approach in the future to trigger a refresh of the material's uniforms. For now, it's okay.

@mrdoob mrdoob added this to the r133 milestone Sep 8, 2021
const instanceUniform = new InstanceUniformNode();

const material = new THREE.MeshStandardMaterial();
material.colorNode = instanceUniform;
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, THREE.MeshStandardMaterial doesn't have this property... 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In new NodeMaterial system all albedo(.map) and .color properties is used as Material.colorNode like:

material.colorNode = new OperatorNode( '*', new TextureNode( diffuseMap ), new Vector3Node( material.color ) );
// roughness is in G channel, metalness is in B channel
material.roughnessNode = new SwitchNode( mpMapNode, 'g' );
material.metalnessNode = new SwitchNode( mpMapNode, 'b' );
material.normalNode = new NormalMapNode( new TextureNode( normalMap ) );

Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't it be const material = new MeshStandardNodeMaterial(); then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that yes, It would be better for serialization to extend. Currenty this pattern works for any material that use color and map like: MeshBasicMaterial, MeshPhongMaterial MeshPhysicalMaterial... would we then have to extended all the materials? The new NodeMaterial system not need more special materials like StandardNodeMaterial.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can do this in an other PR.

Copy link
Collaborator

@Mugen87 Mugen87 Sep 9, 2021

Choose a reason for hiding this comment

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

The new NodeMaterial system not need more special materials like StandardNodeMaterial.

Yeah, that was the original design idea. Meaning just monkey-patch the existing material classes with new node properties like colorNode. The default material properties like color are going to internally mapped to nodes for backwards compatibility reasons. Meaning an existing material definition from WebGL also works with WebGPU. However, users can start with node features by just adding node properties to the material.

I guess this a good opportunity to clarify whether we want to use this approach or introduce separate node material classes again (like with the previous node system).

Copy link
Owner

@mrdoob mrdoob Sep 9, 2021

Choose a reason for hiding this comment

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

Yeah, that was the original design idea. Meaning just monkey-patch the existing material classes with new node properties like colorNode. The default material properties like color are going to internally mapped to nodes for backwards compatibility reasons.

Hmm, I think that's a bit confusing at this point...
We can try doing that later but I think I would keep the materials separate for now.

@mrdoob mrdoob merged commit 444e181 into mrdoob:dev Sep 10, 2021
@mrdoob
Copy link
Owner

mrdoob commented Sep 10, 2021

Thanks!

@sunag sunag deleted the dev-nodes branch September 10, 2021 16:26
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.

3 participants