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

Rework how NodeMaterial is built and make other updates #23797

Merged
merged 20 commits into from
Mar 30, 2022

Conversation

LeviPesin
Copy link
Contributor

@LeviPesin LeviPesin commented Mar 26, 2022

Related issue: -

Description

The main change in this PR is to replace WebGPUNodeBuilder._parseObject and WebGLNodeBuilder._parseObject with NodeMaterial.build.

  • Remove BypassNode,
  • Add more exports to the ShaderNode,
  • Split ShaderNode into two parts,
  • Change SkinningNode, ColorSpaceNode, and BSDFs to allow circular inclusion. A better fix for this would be greatly appreciated,
  • Add NodeMaterial.fromMaterial,
  • Replace WebGPUNodeBuilder._parseObject with NodeMaterial.build,
  • Make the same with WebGLNodeBuilder - I am not sure how to do it without breaking the lighting. @sunag can you help with this, please?

@sunag
Copy link
Collaborator

sunag commented Mar 26, 2022

@LeviPesin I think we're going to need compatibility for non NodeMaterials in WebGPU like MeshStandardMaterial, etc.

@LeviPesin
Copy link
Contributor Author

For this I added the function Materials.toNodeMaterial, and added an use of it to WebGPUNodeBuilder.

@sunag
Copy link
Collaborator

sunag commented Mar 27, 2022

For this I added the function Materials.toNodeMaterial, and added an use of it to WebGPUNodeBuilder.

I think it would be better NodeMaterial.fromMaterial()?

@LeviPesin
Copy link
Contributor Author

I think that with Materials it would be easier to keep track of whether it is MeshBasicNodeMaterial, MeshStandardNodeMaterial, or so on (and also with NodeMaterial.fromMaterial NodeMaterial should import all other materials which will create another circular inclusion problem).

@sunag
Copy link
Collaborator

sunag commented Mar 27, 2022

I think It doens't look like a common function style of TJS Materials.toNodeMaterial() we don't even use the Materials class for API. Is more common the Class.from...() nomeclature.

The other reson it that it currently does not bind NodeMaterial depedencies in the core.

@Mugen87 @mrdoob
For a NodeMaterial conversion function from any Material what do you think is more appropriate?

  • NodeMaterial.fromMaterial( material )
  • Materials.toNodeMaterial( material )
  • some other?

@LeviPesin
Copy link
Contributor Author

we don't even use the Materials class for API

It is not a part of Materials class API, it is just a function exported from the three-nodes/materials/Materials.js.

@sunag
Copy link
Collaborator

sunag commented Mar 27, 2022

It is not a part of Materials class API, it is just a function exported from the three-nodes/materials/Materials.js.

I see, I think that is more a reason to change.

I think it would be more appropriate for us to follow this rule for now:

The other reason it that it currently does not bind NodeMaterial dependencies in the core.

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Mar 27, 2022

I am not sure that I understand what "does not bind NodeMaterial dependencies in the core" means...

The main problem with NodeMaterial.fromMaterial is the circular dependency problem which cannot be solved, because classes are initialized synchronously (and there is no way to change this).
I.e. this means if I write import MeshBasicNodeMaterial from './MeshBasicNodeMaterial.js'; export default class NodeMaterial {} in NodeMaterial.js and import NodeMaterial from './NodeMaterial.js'; export default class MeshBasicNodeMaterial extends NodeMaterial {} in MeshBasicNodeMaterial.js, it will break (with something like "NodeMaterial is not initialized", I think this depends on the browser).

@sunag
Copy link
Collaborator

sunag commented Mar 27, 2022

The main problem with NodeMaterial.fromMaterial is the circular dependency problem which cannot be solved, because classes are initialized synchronously (and there is no way to change this).

I fixed a circular dependecy in Material.fromType(), is need created first a interface function and extended it in some moment, maybe in Nodes.js for example..

@sunag
Copy link
Collaborator

sunag commented Mar 27, 2022

Another approach I wanted to test is create a static lib where we can add the classes without need to extend the function...

@LeviPesin
Copy link
Contributor Author

This is a good idea - i.e. make materialsLib empty by default and extend it in each Material class. But I think it should be a separate PR.

@LeviPesin
Copy link
Contributor Author

@sunag So what is your opinion about node material conversion function? If you think that it should be made NodeMaterial.fromMaterial, then please provide an example code how it should be done (I still think that it is impossible because of circular inclusion...).

@sunag
Copy link
Collaborator

sunag commented Mar 28, 2022

I am not sure that I understand what "does not bind NodeMaterial dependencies in the core" means...

Sorry for this! I some moment I thought you were editing Materials.js of the core.

@sunag
Copy link
Collaborator

sunag commented Mar 28, 2022

I think that this is probably because is it missing BypassNode?

Dev
image

PR's Ignoring vertex stage normals calcs
image

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Mar 29, 2022

Can you please test the original (i.e. before this PR) WebGPUNodeBuilder with the code

// VERTEX STAGE

let vertex = new PositionNode( PositionNode.GEOMETRY );

if ( material.positionNode && material.positionNode.isNode ) {

	const assignPositionNode = new OperatorNode( '=', new PositionNode( PositionNode.LOCAL ), material.positionNode );

	vertex = new BypassNode( vertex, assignPositionNode );

}

if ( object.isSkinnedMesh === true ) {

	vertex = new BypassNode( vertex, new SkinningNode( object ) );

}

this.context.vertex = vertex;

this.addFlow( 'vertex', new VarNode( new ModelViewProjectionNode(), 'MVP', 'vec4' ) );

changed to

// VERTEX STAGE

if ( material.positionNode ) this.addFlow( 'vertex', assign( positionLocal, material.positionNode ) );
if ( object.isSkinnedMesh ) this.addFlow( 'vertex', skinning( object ) );

this.addFlow( 'vertex', modelViewProjection() );

? When I've tested it locally it was working the same as before the PR.

And this cannot be an issue with the BypassNode - TempNode.build(); Node.build(); is perfectly equivalent to the BypassNode( Node, TempNode ).build(). It also should not be an issue with removing this.context.vertex (and I also think that if ( this.context.vertex && this.context.vertex.isNode ) { this.flowNodeFromShaderStage( 'vertex', this.context.vertex ); } code can be removed now).

@sunag
Copy link
Collaborator

sunag commented Mar 30, 2022

Set a context.vertex will force assign variations of position and normals to come first in flow. If you add this code bellow for example, the flow will now work normally.

let vertex = positionLocal;
		
if ( this.positionNode ) builder.addFlow( 'vertex', assign( positionLocal, vertex = this.positionNode ) );
if ( builder.object.isSkinnedMesh ) builder.addFlow( 'vertex', vertex = skinning( builder.object ) );

builder.context.vertex = vertex;

@sunag
Copy link
Collaborator

sunag commented Mar 30, 2022

BypassNode can be used mainly to call void functions in the flow process. If we remove this, assume we will not have support to any FunctionNode that returns void.

It is possible to notice in two Skinning examples that BypassNode is used for flow control, and not necessarly for a simple assigning.

@sunag
Copy link
Collaborator

sunag commented Mar 30, 2022

@LeviPesin Still this fix break Skinning Points. It is safer to introduce the BypassNode to leave the assigns in the final process. I think that it can be only bypass( target, voidNode ).

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Mar 30, 2022

It is possible to notice in two Skinning examples that BypassNode is used for flow control, and not necessarly for a simple assigning.

But in SkinningNode it can be fixed changing SkinningNode extends Node to SkinningNode extends TempNode and BypassNode( vertex, SkinningNode ) to just SkinningNode (because as I understand TempNode and BypassNode BypassNode( Node, TempNode ).build() is the same as TempNode.build(); Node.build()).

@sunag
Copy link
Collaborator

sunag commented Mar 30, 2022

I think that are not the same thing...

BypassNode is used to change the flow process, it is not related with cache.

let vertex = positionLocal;
vertex = bypass( vertex, displace( .. ) );
vertex = bypass( vertex, morpher( .. ) );
vertex = bypass( vertex, skinning( builder.object ) );

// This will process all vars, cache, etc before returning positionLocal to the output

@LeviPesin
Copy link
Contributor Author

OK, I will revert the changes to BypassNode and add it to ShaderNode.

@LeviPesin
Copy link
Contributor Author

@sunag So what do you think about WebGLNodeBuilder? Is there a way for it to use the NodeMaterial.fromMaterial().build() so that it will not break the lighting (i.e. how to construct the lightNode)?

@sunag
Copy link
Collaborator

sunag commented Mar 30, 2022

@sunag So what do you think about WebGLNodeBuilder? Is there a way for it to use the NodeMaterial.fromMaterial().build() so that it will not break the lighting (i.e. how to construct the lightNode)?

In case of the user use the property .lightNode it can added extra lights results between lights_fragment_begin and lights_fragment_end. I think that it is the first step.

We still don't have some optimizers for WebGL for this reason some calcs can be duplicated like normals used in lights and skinning too. This seems complicated at the moment.

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Mar 30, 2022

In case of the user use the property .lightNode it can added extra lights results between lights_fragment_begin and lights_fragment_end. I think that it is the first step.

I meant the scene lightNode (and fogNode), similarly to how it is done in WebGPUNodeBuilder. If we add some code to construct them, then we can remove a large part of the WebGLNodeBuilder (that allows using "native" shaders) and replace it with just NodeMaterial.fromMaterial().build(), the same as it was done by me for WebGPUNodeBuilder.

We still don't have some optimizers for WebGL for this reason some calcs can be duplicated like normals used in lights and skinning too. This seems complicated at the moment.

I am not sure that even WebGPUNodeBuilder's current optimizers are working too good... See #23728.

@sunag sunag merged commit 11546d7 into mrdoob:dev Mar 30, 2022
@sunag
Copy link
Collaborator

sunag commented Mar 30, 2022

Thanks!! 👍

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Mar 31, 2022

@sunag Now ShaderNodeUtils does not work, because there is no function uint there.
Also, why did you revert changes about NodeProxying some nodes?

@LeviPesin LeviPesin deleted the rework-node-materials branch March 31, 2022 04:37
@LeviPesin LeviPesin restored the rework-node-materials branch March 31, 2022 04:37
@sunag
Copy link
Collaborator

sunag commented Mar 31, 2022

@sunag Now ShaderNodeUtils does not work, because there is no function uint there.

I will fix this.
use ShaderNodeElements instead, all syntax must be there, including float, uint etc

Also, why did you revert changes about NodeProxying some nodes?

not all arguments must be nodeObjects

@LeviPesin LeviPesin deleted the rework-node-materials branch March 31, 2022 05:12
@LeviPesin
Copy link
Contributor Author

OK. Will think how it would be possible to construct LightNode and FogNode in WebGLNodeBuilder...

@sunag
Copy link
Collaborator

sunag commented Apr 1, 2022

OK. Will think how it would be possible to construct LightNode and FogNode in WebGLNodeBuilder...

The implementation is similar to what already exist, the problem here is to reuse variables created in WebGLRenderer as normal and position calcs for the lights and others. This is the optimization I'm referring here.

@LeviPesin
Copy link
Contributor Author

The implementation is similar to what already exist

Can you please share it?

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Rework NodeMaterial and make other updates

* Fix DeepScan issues

* Fix DeepScan issues

* Cleanup

* NodeMaterial.fromMaterial()

* Fix

* Revert BypassNode removal

* Fix

* Split ShaderNode into two parts

* Fix

* Fix

* Another fix

* One more fix

* Apply suggestions

* One more fix

* Move ShaderNodeUtils to core

* move ShaderNode* to ./shadernode/ and fixes

* fix from deepscan

Co-authored-by: sunag <jcdeconto@igalia.com>
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.

None yet

2 participants