[WIP] GLTFLoader: Add .setUseNodes() option. #14572

donmccurdy · 2018-07-28T08:34:02Z

Follow-up from #14149.

This isn't meant to be merged, or at least not at the moment, does make it easier for me to test glTF models with NodeMaterial, and hopefully helps @sunag a bit. Supporting NodeMaterial as an "opt-in" experimental thing in GLTFLoader is looking much easier with the MeshStandardNodeMaterial. :)

A few issues found:

~~MeshStandardNodeMaterial and others don't apply constructor arguments. (would help for backward compatibility)~~
~~.copy() methods should return this or cloning will fail.~~
~~sRGB textures are treated as linear. I tried to fix this below but didn't find the problem yet.~~
aoMap missing?

@sunag I'd be glad to open separate PRs for any of this, but if you have other PRs in progress I'll avoid the merge conflict and leave it to you. 😇

donmccurdy · 2018-07-28T08:36:49Z

examples/js/nodes/materials/nodes/MeshStandardNode.js

+		// * ao
+		// * aoMap
+
+		var ao = builder.findNode( props.ao, inputs.ao ),


Oops this should be aoMapIntensity and included in shortcuts

Yeah. To shortcuts in MeshNodeMaterial you can add here:
https://github.com/donmccurdy/three.js/blob/6a498fd2793befd33aa315716ae2af2022c71bb9/examples/js/nodes/materials/nodes/MeshStandardNode.js#L18
And
https://github.com/donmccurdy/three.js/blob/6a498fd2793befd33aa315716ae2af2022c71bb9/examples/js/nodes/materials/nodes/MeshStandardNode.js#L25
*Edit
https://github.com/donmccurdy/three.js/blob/6a498fd2793befd33aa315716ae2af2022c71bb9/examples/js/nodes/materials/MeshStandardNodeMaterial.js#L29

https://github.com/donmccurdy/three.js/blob/6a498fd2793befd33aa315716ae2af2022c71bb9/examples/js/nodes/materials/MeshStandardNodeMaterial.js#L29

sunag

@sunag I'd be glad to open separate PRs for any of this, but if you have other PRs in progress I'll avoid the merge conflict and leave it to you.

Thank you! About conflict do not worry, I will not go post other PR big so early, I can use your code if you post first. Would be great you post PR with these fixes and improvements :-)

sunag · 2018-07-28T14:37:53Z

examples/js/nodes/materials/nodes/MeshStandardNode.js

+		// * ao
+		// * aoMap
+
+		var ao = builder.findNode( props.ao, inputs.ao ),


Yeah. To shortcuts in MeshNodeMaterial you can add here:
https://github.com/donmccurdy/three.js/blob/6a498fd2793befd33aa315716ae2af2022c71bb9/examples/js/nodes/materials/nodes/MeshStandardNode.js#L18
And
https://github.com/donmccurdy/three.js/blob/6a498fd2793befd33aa315716ae2af2022c71bb9/examples/js/nodes/materials/nodes/MeshStandardNode.js#L25
*Edit
https://github.com/donmccurdy/three.js/blob/6a498fd2793befd33aa315716ae2af2022c71bb9/examples/js/nodes/materials/MeshStandardNodeMaterial.js#L29

sunag · 2018-07-28T14:39:37Z

examples/js/nodes/utils/ColorSpaceNode.js

@@ -203,7 +209,7 @@ ColorSpaceNode.LOG_LUV_TO_LINEAR = 'LogLuvToLinear';

 ColorSpaceNode.prototype = Object.create( TempNode.prototype );
 ColorSpaceNode.prototype.constructor = ColorSpaceNode;
-ColorSpaceNode.prototype.nodeType = "ColorAdjustment";
+ColorSpaceNode.prototype.nodeType = "ColorSpaceNode";


Node in final is not necessary, only nodeType = "ColorSpace"

sunag · 2018-07-28T14:40:46Z

examples/js/nodes/materials/nodes/MeshStandardNode.js

+		// * ao
+		// * aoMap
+
+		var ao = builder.findNode( props.ao, inputs.ao ),


https://github.com/donmccurdy/three.js/blob/6a498fd2793befd33aa315716ae2af2022c71bb9/examples/js/nodes/materials/MeshStandardNodeMaterial.js#L29

sunag · 2018-07-28T14:43:46Z

examples/js/nodes/materials/MeshStandardNodeMaterial.js

@@ -14,6 +14,10 @@ function MeshStandardNodeMaterial() {

 	this.type = "MeshStandardNodeMaterial";

+	var options = arguments[ 0 ];
+
+	if ( typeof options === 'object' ) Object.assign( this, options );


I think we could follow this standard?
https://github.com/donmccurdy/three.js/blob/1ffd8db807e096af16960887c2c086bf3856bc3a/src/materials/MeshStandardMaterial.js#L56

sunag · 2018-07-28T15:09:02Z

examples/js/nodes/utils/ColorSpaceNode.js

+
+	if ( this.method === undefined ) {
+
+		this.method = this.getEncodingMethod( this.input.value.encoding );


I think that this already included in TextureNode and CubeTextureNode.

Yes, the lines I added above are incorrect and don't solve the issue I was trying to fix. But with or without these lines, colorspace conversion is coming out backwards... when map.encoding === THREE.sRGBEncoding, I get the following generated GLSL:

vec3 diffuseColor = ( vec4( nVu0, 1.0 ) * LinearTosRGB( tex2D( nVu1, vUv ) ) ).xyz;

which should be sRGBToLinear.

three.js/examples/js/nodes/inputs/TextureNode.js

Line 60 in 8f95052

this.colorSpace = this.colorSpace || new ColorSpaceNode( this );

three.js/examples/js/nodes/inputs/CubeTextureNode.js

Line 56 in 8f95052

this.colorSpace = this.colorSpace || new ColorSpaceNode( this );

I think that it being sould be:

builder.addContext( { input: code, include: builder.isShader( 'vertex' ) } ); this.colorSpace = this.colorSpace || new ColorSpaceNode( this ); this.colorSpace.fromDecoding( builder.getTextureEncodingFromMap( this.value ) ); code = this.colorSpace.build( builder, this.type ); builder.removeContext();

ColorSpaceNode.prototype.fromDecoding= function ( decoding ) { this.method = this.getDecodingMethod ( decoding )[ 0 ]; return this; };

mrdoob · 2018-07-29T05:20:43Z

examples/js/nodes/materials/MeshStandardNodeMaterial.js

@@ -6,21 +6,29 @@ import { MeshStandardNode } from './nodes/MeshStandardNode.js';
 import { NodeMaterial } from './NodeMaterial.js';
 import { NodeUtils } from '../core/NodeUtils.js';

-function MeshStandardNodeMaterial() {
+function MeshStandardNodeMaterial( parameters ) {


I'm not sure about this. I never liked passing parameters as an object, it's error-prone and limited.
We have a chance to move away from it here.

I thought the goal of MeshStandardNodeMaterial was to be a backward-compatible alternative to MeshStandardMaterial, whereas StandardNodeMaterial would provide a new, fully node-based syntax? If we don't want this class to support old syntax then maybe I've misunderstood the difference.

@sunag is that correct, or what would be the goal with the two material classes here? Put another way, is this what we want?

MeshStandardMaterial (current)

MeshStandardNodeMaterial (node-based, fully backward-compatible)

StandardNodeMaterial (node-based, new API, new features)

I’m assuming here that MeshStandardMaterial would eventually be replaced by MeshStandardNodeMaterial. If that’s not the goal, or if we want the existing system to exist alongside node-based materials indefinitely, then perhaps we don’t need a backward-compatible MeshStandardNodeMaterial?

I think the plan was to make MeshStandardMaterial node-based to keep backwards-compatibility.

Maybe MeshStandardMaterial could extend MeshStandardNodeMaterial.

I never liked passing parameters as an object

Personally, I much prefer parameters as an object to

Texture( image, mapping, wrapS, wrapT, magFilter, minFilter, format, type, anisotropy, encoding )

where the order matters. Very annoying.

I agree that passing parameters like Texture currently does is also annoying.

I prefer this kind of API:

new THREE.Texture( image ).setMapping( mapping ).setFormat( format );

Or:

var texture = new THREE.Texture( image ); texture.mapping = mapping; texture.format = format;

But the problem of the last one is that the user could make a typo like texture.fromat and javascript won't complain. Unless we start using Object.freeze().

We could just delete MeshStandardMaterial, rename MeshStandardNodeMaterial as MeshStandardMaterial, and if everything is backward-compatible there is no user-facing change. Users should only know about MeshStandardMaterial and the new NodeStandardMaterial.

I think that's a big change. Too many tutorials/articles that will get obsolete.

I think it's easier to think about *NodeMaterial as having a different API. If we bring parameters to the new API we're carrying over the problem and it'll be harder to maintain.

We can always add parameter support to MeshStandardNodeMaterial later. For the time being I prefer to not over complicate the new API.

Hm... I don't think that we should introduce two new standard materials (MeshStandardNodeMaterial, StandardNodeMaterial) that both have different, breaking APIs for the same features. For the new API, StandardNodeMaterial seems like the place to start — we can leave parameter support out there. If MeshStandardNodeMaterial will not have the same API as MeshStandardMaterial, I think we should just delete it (MeshStandardNodeMaterial) to prevent confusion, unless it has some other purpose I've missed.

I never liked passing parameters as an object, it's error-prone and limited.

Can you explain why this:

const mat = new MeshStandardMaterial( { color: oxff0000, roughness: 0.25, transparency: true, map, alphaMap, normalMap } ) ;

is more error prone or limited than

const mat = new MeshStandardMaterial().setColor( 0xff0000).setRoughness( 0.25 ).setTransparent( true ).setMap( map ).setAlphaMap( alphaMap ).setNormalMap( normalMap );

The second approach seems much more verbose to me, with no advantage in clarity - especially since this would be a breaking change.

But the problem of the last one is that the user could make a typo like texture.fromat and javascript won't complain. Unless we start using Object.freeze().

If that is the problem I think this could be fixed with hasOwnProperty declaring all properties before of setValues even undefined variables:

Example (1)

var obj = { format : undefined }; // inside while var prop = "fromat", val = "any value"; if ( obj.hasOwnProperty( prop ) ) obj[prop] = val; else console.warn(prop, "property does not exist or has become obsolete.");

Example( 2)

NodeMaterial.prototype.setValues = function( values ) { if ( values === undefined ) return; for ( var key in values ) { if ( this.hasOwnProperty( key ) ) { this[ key ] = values[ key ]; } else { console.warn(key, "property does not exist or has become obsolete."); } } };

Personally, I much prefer parameters as an object to [ordered parameters]

+1 I think this could come from the an evolution of compiler too like interface of typescript. For me, besides being practical to define parameters it techniques is good to avoid changes if a property is to be depreciate in relation to ordered parameters, in relation at setParamA(a).setParamB(b) could help to reduce the code.

Another third thing could be to create a classe just for this.

new THREE.SetValues( this, params ); // or THREE.SetValues( this, params )

donmccurdy · 2018-10-02T16:58:14Z

Hm... I don't think that we should introduce two new standard materials (MeshStandardNodeMaterial, StandardNodeMaterial) that both have different, breaking APIs for the same features. For the new API, StandardNodeMaterial seems like the place to start — we can leave parameter support out there. If MeshStandardNodeMaterial will not have the same API as MeshStandardMaterial, I think we should just delete it (MeshStandardNodeMaterial) to prevent confusion, unless it has some other purpose I've missed.

I'd meant to help write documentation for NodeMaterial but it is blocked on this question... I don't know which of these classes we actually want people to start using, and which are internal or transitional. 🤔

mrdoob · 2018-10-15T19:57:16Z

Hm... I don't think that we should introduce two new standard materials (MeshStandardNodeMaterial, StandardNodeMaterial) that both have different, breaking APIs for the same features.

What's the StandardNodeMaterial one for?

donmccurdy · 2018-10-15T20:11:01Z

I believe (@sunag please correct me if I'm wrong) that StandardNodeMaterial will be a node-based material with a new, flexible, future-proof API — backward-compatibility not required. In addition, MeshStandardMaterial will be upgraded to rely on nodes internally, keeping a backwards-compatible API, but not benefiting from the full flexibility of nodes. MeshStandardNodeMaterial tries to implement that backward-compatible, node-based version of MeshStandardMaterial, but if we want existing users to be "automatically" moved over to node materials we should rename it as MeshStandardMaterial and delete the existing one.

^Or that's my guess at what the code currently does. Whether the names should be kept or changed is another question.

sunag · 2018-10-16T01:20:43Z

@donmccurdy Exactly. Complementing only that MeshStandardNodeMaterial is a candidate material for replace MeshStandardMaterial if node-material system is merged in the core.

donmccurdy · 2019-10-07T17:48:05Z

I'd like to rebase this PR and change its API so that nodes are looked up from a dictionary, rather than direct dependence on the THREE.* namespace (which won't work with ES modules). For example:

import { GLTFLoader } from 'three/examples/jsm/loaders/GLTFLoader.js';
import * as Nodes from 'three/examples/jsm/nodes/Nodes.js';

var loader = new GLTFLoader();
loader.setNodeMaterialDictionary( Nodes ); // Optional. If set, NodeMaterial is used.

loader.load( 'model.glb', ... );

With that done, I think it would be alright to merge this, providing users an easy way to get started with NodeMaterial from an existing glTF model. Does this sound OK?

mrdoob · 2019-10-07T20:29:12Z

I think that sounds good to me.

donmccurdy · 2020-01-27T03:41:23Z

Closing for now, this will need a rewrite...

GLTFLoader: Add .setUseNodes() option.

6a498fd

donmccurdy commented Jul 28, 2018

View reviewed changes

sunag reviewed Jul 28, 2018

View reviewed changes

donmccurdy added 2 commits July 28, 2018 12:45

NodeMaterial: back-compat fixes.

8f95052

NodeMaterial: Default ColorSpaceNode.LINEAR_TO_LINEAR

f694212

mrdoob reviewed Jul 29, 2018

View reviewed changes

donmccurdy mentioned this pull request Aug 7, 2018

GLTFLoader: Implement KHR_texture_transform extension. #13831

Merged

sunag mentioned this pull request Aug 16, 2018

THREE.SetValues #14726

Closed

donmccurdy mentioned this pull request Jan 4, 2019

Multiple UV Layouts KhronosGroup/glTF-Blender-IO#179

Closed

donmccurdy mentioned this pull request Apr 21, 2019

GLTFLoader texCoord #16297

Closed

Mugen87 mentioned this pull request Sep 17, 2019

GLTFLoader: Support map in LineBasicMaterial #17511

Closed

donmccurdy mentioned this pull request Nov 20, 2019

NodeMaterial documentation #17971

Open

donmccurdy closed this Jan 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] GLTFLoader: Add .setUseNodes() option. #14572

[WIP] GLTFLoader: Add .setUseNodes() option. #14572

donmccurdy commented Jul 28, 2018 •

edited

Loading

donmccurdy Jul 28, 2018

sunag Jul 28, 2018 •

edited

Loading

sunag Jul 28, 2018

sunag left a comment

sunag Jul 28, 2018 •

edited

Loading

sunag Jul 28, 2018

sunag Jul 28, 2018

sunag Jul 28, 2018

sunag Jul 28, 2018

donmccurdy Jul 28, 2018 •

edited

Loading

sunag Jul 28, 2018

mrdoob Jul 29, 2018

donmccurdy Jul 29, 2018 •

edited

Loading

donmccurdy Aug 13, 2018

mrdoob Aug 14, 2018

WestLangley Aug 14, 2018

mrdoob Aug 14, 2018 •

edited

Loading

mrdoob Aug 14, 2018 •

edited

Loading

donmccurdy Aug 14, 2018

looeee Aug 15, 2018

sunag Aug 15, 2018 •

edited

Loading

donmccurdy commented Oct 2, 2018

mrdoob commented Oct 15, 2018

donmccurdy commented Oct 15, 2018 •

edited

Loading

sunag commented Oct 16, 2018

donmccurdy commented Oct 7, 2019 •

edited

Loading

mrdoob commented Oct 7, 2019

donmccurdy commented Jan 27, 2020


		if ( this.method === undefined ) {

		this.method = this.getEncodingMethod( this.input.value.encoding );

[WIP] GLTFLoader: Add .setUseNodes() option. #14572

[WIP] GLTFLoader: Add .setUseNodes() option. #14572

Conversation

donmccurdy commented Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

sunag Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunag left a comment

Choose a reason for hiding this comment

sunag Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy Jul 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdoob Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

mrdoob Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunag Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

donmccurdy commented Oct 2, 2018

mrdoob commented Oct 15, 2018

donmccurdy commented Oct 15, 2018 • edited Loading

sunag commented Oct 16, 2018

donmccurdy commented Oct 7, 2019 • edited Loading

mrdoob commented Oct 7, 2019

donmccurdy commented Jan 27, 2020

donmccurdy commented Jul 28, 2018 •

edited

Loading

sunag Jul 28, 2018 •

edited

Loading

sunag Jul 28, 2018 •

edited

Loading

donmccurdy Jul 28, 2018 •

edited

Loading

donmccurdy Jul 29, 2018 •

edited

Loading

mrdoob Aug 14, 2018 •

edited

Loading

mrdoob Aug 14, 2018 •

edited

Loading

sunag Aug 15, 2018 •

edited

Loading

donmccurdy commented Oct 15, 2018 •

edited

Loading

donmccurdy commented Oct 7, 2019 •

edited

Loading