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

Split InputNode into ConstNode and UniformNode #23663

Merged
merged 25 commits into from
Mar 12, 2022

Conversation

LeviPesin
Copy link
Contributor

@LeviPesin LeviPesin commented Mar 5, 2022

Related issue: #23646

Description

Split InputNode into ConstNode and UniformNode.

Breaking changes:

  • InputNode had been rewrited and now should not be used directly - ConstNode and UniformNode should be used instead.
  • IntNode, FloatNode, BoolNode, UintNode, Vector2Node, Vector3Node, Vector4Node, Matrix2Node, Matrix3Node, and Matrix4Node had been removed - ConstNode and UniformNode should be used instead.
  • BufferNode and TextureNode had been moved from inputs/ to accessors/.
  • ArrayInputNode was renamed to ArrayUniformNode.

@LeviPesin
Copy link
Contributor Author

@sunag I think this is better than #23660...

@LeviPesin
Copy link
Contributor Author

I think the NodeEditor also requires a fix for InputNode, but I think I will do it tomorrow.

@LeviPesin
Copy link
Contributor Author

@sunag now this PR passes all the tests but not all examples are working - e.g. https://raw.githack.com/LeviPesin/three.js-1/split-input-node/examples/webgl_materials_standard_nodes.html (gun does not show even without serialization code). Not sure what is the problem with it...

@sunag
Copy link
Collaborator

sunag commented Mar 6, 2022

const value = new THREE.Vector3();

// current

const vec3 = new Nodes.Vector3Node( value  );
const vec3Const = new Nodes.Vector3Node( value  ).setConst( true );

// proposed by @LeviPesin

const vec3 = new Nodes.UniformNode( 'vec3', value  );
const vec3Const = new Nodes.ConstNode( 'vec3', value  );

// proposed by @LeviPesin with my cleanup

const vec3 = new Nodes.UniformNode( value );
const vec3Const = new Nodes.ConstNode( value );

@mrdoob do you have any thoughts on this?

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Mar 7, 2022

If you remove the type argument, then it would be impossible to e.g. distinguish between float 1.0 and int 1. The best thing would be to just use a function from the ShaderNode - Nodes.float( 1.0 ), Nodes.int( 1 ), Nodes.vec3( value ), etc.

@sunag
Copy link
Collaborator

sunag commented Mar 7, 2022

I don't think in remove it entirely. It can be auto-detected and used as second parameter if needed.

@sunag
Copy link
Collaborator

sunag commented Mar 7, 2022

Nodes.float( 1.0 ), Nodes.int( 1 ), Nodes.vec3( value )

For me, this breaks the current class style.

I think it would be a valid update to do in this PR is:

UniformNode( value, type = null )

@LeviPesin
Copy link
Contributor Author

I still think that for the class approach it is better to always specify the type. But the ShaderNode approach is much better than the class one, I think - it is much easier to write and understand.

@sunag
Copy link
Collaborator

sunag commented Mar 7, 2022

I still think that for the class approach it is better to always specify the type.

This is unnecessary most of the time, I don't see why. If the users puts a Vector3 as a value, the type must be the same, unless he uses the second argument.

@LeviPesin
Copy link
Contributor Author

If user e.g. puts a Vector3(1, 1, 1) as a value, it could be vec3, ivec3, or uvec3.

@sunag
Copy link
Collaborator

sunag commented Mar 7, 2022

If user e.g. puts a Vector3(1, 1, 1) as a value, it could be vec3, ivec3, or uvec3.

These are exceptions, it is unusual to use ivec3 even in GLSL, but for this can be use the second argument. I think that we have to prioritize common use. Vector3 is a floating vector as default.

@LeviPesin
Copy link
Contributor Author

OK, so you are suggesting new UniformNode( value, type ) where if type is undefined it will be assumed float, vec2, vec3, vec4, mat3, mat4, or color based on the value?

@sunag
Copy link
Collaborator

sunag commented Mar 7, 2022

OK, so you are suggesting new UniformNode( value, type ) where if type is undefined it will be assumed float, vec2, vec3, vec4, mat3, mat4, or color based on the value?

Exactly, and bool too.

@LeviPesin
Copy link
Contributor Author

@sunag I've managed to fix serialization/deserialization in the https://raw.githack.com/LeviPesin/three.js-1/split-input-node/examples/webgl_materials_standard_nodes.html, but for some reason NodeObjectLoader.parse.onLoad is called twice there (as it also was before this PR), but now the first call throws an error for some reason... Could you please look into it and fix it?

@mrdoob
Copy link
Owner

mrdoob commented Mar 7, 2022

@mrdoob do you have any thoughts on this?

This one looks good to me:

const value = new THREE.Vector3();

const vec3 = new Nodes.UniformNode( value );
const vec3Const = new Nodes.ConstNode( value );

OK, so you are suggesting new UniformNode( value, type ) where if type is undefined it will be assumed float, vec2, vec3, vec4, mat3, mat4, or color based on the value?

That sounds good too!

@sunag sunag added this to the r139 milestone Mar 8, 2022
@sunag
Copy link
Collaborator

sunag commented Mar 8, 2022

@sunag I've managed to fix serialization/deserialization in the https://raw.githack.com/LeviPesin/three.js-1/split-input-node/examples/webgl_materials_standard_nodes.html, but for some reason NodeObjectLoader.parse.onLoad is called twice there (as it also was before this PR), but now the first call throws an error for some reason... Could you please look into it and fix it?

Ok, I will test this.

@LeviPesin
Copy link
Contributor Author

@sunag Your last commit breaks serialization/deserialization. You can write data.nodeType = this.getNodeType() instead but it can possibly break if a child of InputNode redefines .getNodeType(). That is why I created a new function ._getNodeType().

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Mar 8, 2022

Or you can inline the body of _getNodeType into both getNodeType and serialize.

@sunag
Copy link
Collaborator

sunag commented Mar 8, 2022

@LeviPesin I commit a possible fix but didn't test it. I will make a revision of this later.

@LeviPesin
Copy link
Contributor Author

I think it works. Then you can also remove .slice(4) from getValueFromType.

@LeviPesin
Copy link
Contributor Author

@sunag Is there any progress on fixing that example?

@sunag
Copy link
Collaborator

sunag commented Mar 12, 2022

@sunag Is there any progress on fixing that example?

It's been revised and fixed! There were other broken examples too.

@sunag
Copy link
Collaborator

sunag commented Mar 12, 2022

@mrdoob Can we merge?

@mrdoob mrdoob merged commit 6e37b45 into mrdoob:dev Mar 12, 2022
@mrdoob
Copy link
Owner

mrdoob commented Mar 12, 2022

Thanks!

@LeviPesin LeviPesin deleted the split-input-node branch March 12, 2022 06:31
@LeviPesin LeviPesin mentioned this pull request Mar 12, 2022
6 tasks
@wmcmurray
Copy link
Contributor

A slight mention of this breaking change in the 138 → 139 Migration Guide would have been helpful 🙂
@Mugen87 ? 🙏🏼

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 27, 2022

We have not documented breaking changes of the node material so far because of its novel character. A lot of things have changed in the recent past. But it seems the node material is now mature enough to change this policy.

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Split InputNode into ConstNode and UniformNode

* ArrayInputNode -> ArrayUniformNode

* Fix nodes

* Fix examples

* Fix

* Fix NodeEditor

* Fix b/u/ivec constants

* Fix

* Fix webgpu_instance_uniform.html

* Change signature

* Fix serialization/deserialization

* Fixes

* Fix for nodeType

* cleanup

* Update InputNode.js

cleanup(2)

* More cleanup

* fix class name

* reuse uniformNode

* fix uniform

* fix non-node params

* fix updateUniforms default values

* intro to InputNode.getInputType()

* cleanup

Co-authored-by: sunag <[email protected]>
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