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

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a498fd
GLTFLoader: Add .setUseNodes() option.
donmccurdy Jul 28, 2018
8f95052
NodeMaterial: back-compat fixes.
donmccurdy Jul 28, 2018
f694212
NodeMaterial: Default ColorSpaceNode.LINEAR_TO_LINEAR
donmccurdy Jul 28, 2018
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
25 changes: 19 additions & 6 deletions examples/js/loaders/GLTFLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ THREE.GLTFLoader = ( function () {

this.manager = ( manager !== undefined ) ? manager : THREE.DefaultLoadingManager;
this.dracoLoader = null;
this.useNodes = false;

}

Expand Down Expand Up @@ -76,6 +77,13 @@ THREE.GLTFLoader = ( function () {

},

setUseNodes: function ( useNodes ) {

this.useNodes = useNodes;
return this;

},

parse: function ( data, path, onLoad, onError ) {

var content;
Expand Down Expand Up @@ -168,7 +176,8 @@ THREE.GLTFLoader = ( function () {

path: path || this.path || '',
crossOrigin: this.crossOrigin,
manager: this.manager
manager: this.manager,
useNodes: this.useNodes

} );

Expand Down Expand Up @@ -341,7 +350,9 @@ THREE.GLTFLoader = ( function () {

}

GLTFMaterialsUnlitExtension.prototype.getMaterialType = function ( material ) {
GLTFMaterialsUnlitExtension.prototype.getMaterialType = function ( _, useNodes ) {

if ( useNodes ) throw new Error( 'THREE.GLTFLoader: Nodes not supported with unlit materials.' );

return THREE.MeshBasicMaterial;

Expand Down Expand Up @@ -560,7 +571,9 @@ THREE.GLTFLoader = ( function () {
'refractionRatio',
],

getMaterialType: function () {
getMaterialType: function ( _, useNodes ) {

if ( useNodes ) throw new Error( 'THREE.GLTFLoader: Nodes not supported with spec/gloss materials.' );

return THREE.ShaderMaterial;

Expand Down Expand Up @@ -2070,21 +2083,21 @@ THREE.GLTFLoader = ( function () {
if ( materialExtensions[ EXTENSIONS.KHR_MATERIALS_PBR_SPECULAR_GLOSSINESS ] ) {

var sgExtension = extensions[ EXTENSIONS.KHR_MATERIALS_PBR_SPECULAR_GLOSSINESS ];
materialType = sgExtension.getMaterialType( materialDef );
materialType = sgExtension.getMaterialType( materialDef, this.options.useNodes );
pending.push( sgExtension.extendParams( materialParams, materialDef, parser ) );

} else if ( materialExtensions[ EXTENSIONS.KHR_MATERIALS_UNLIT ] ) {

var kmuExtension = extensions[ EXTENSIONS.KHR_MATERIALS_UNLIT ];
materialType = kmuExtension.getMaterialType( materialDef );
materialType = kmuExtension.getMaterialType( materialDef, this.options.useNodes );
pending.push( kmuExtension.extendParams( materialParams, materialDef, parser ) );

} else {

// Specification:
// https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#metallic-roughness-material

materialType = THREE.MeshStandardMaterial;
materialType = this.options.useNodes ? THREE.MeshStandardNodeMaterial : THREE.MeshStandardMaterial;

var metallicRoughness = materialDef.pbrMetallicRoughness || {};

Expand Down
10 changes: 9 additions & 1 deletion examples/js/nodes/materials/MeshStandardNodeMaterial.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) {
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@donmccurdy donmccurdy Jul 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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?

Copy link
Owner

Choose a reason for hiding this comment

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

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

Maybe MeshStandardMaterial could extend MeshStandardNodeMaterial.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@mrdoob mrdoob Aug 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Owner

@mrdoob mrdoob Aug 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@sunag sunag Aug 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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 )


var node = new MeshStandardNode();

NodeMaterial.call( this, node, node );

this.type = "MeshStandardNodeMaterial";

this.setValues( parameters );

}

MeshStandardNodeMaterial.prototype = Object.create( NodeMaterial.prototype );
MeshStandardNodeMaterial.prototype.constructor = MeshStandardNodeMaterial;

NodeUtils.addShortcuts( MeshStandardNodeMaterial.prototype, 'properties', [
"color",
"emissive",
"emissiveMap",
"emissiveIntensity",
"ao",
"aoMap",
"aoMapIntensity",
"roughness",
"metalness",
"map",
Expand Down
26 changes: 26 additions & 0 deletions examples/js/nodes/materials/NodeMaterial.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,30 @@ Object.defineProperties( NodeMaterial.prototype, {

} );

NodeMaterial.prototype.setValues = function( values ) {

if ( values === undefined ) return;

for ( var key in values ) {

var newValue = values[ key ];

if ( newValue === undefined ) {

console.warn( 'THREE.' + this.type + ': "' + key + '" parameter is undefined.' );
continue;

}

// TODO: The base Material class would warn about setting properties that do not have
// a current value, "...not a property of this material".

this[ key ] = newValue;

}

};

NodeMaterial.prototype.updateFrame = function ( frame ) {

for ( var i = 0; i < this.updaters.length; ++ i ) {
Expand Down Expand Up @@ -108,6 +132,8 @@ NodeMaterial.prototype.copy = function ( source ) {

}

return this;

};

NodeMaterial.prototype.toJSON = function ( meta ) {
Expand Down
38 changes: 38 additions & 0 deletions examples/js/nodes/materials/nodes/MeshStandardNode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,17 @@ function MeshStandardNode() {

this.properties = {
color: new THREE.Color( 0xffffff ),
emissive: new THREE.Color( 0x000000 ),
ao: 1.0,
roughness: 0.5,
metalness: 0.5,
normalScale: new THREE.Vector2( 1, 1 )
};

this.inputs = {
color: new PropertyNode( this.properties, 'color', 'c' ),
emissive: new PropertyNode( this.properties, 'emissive', 'c' ),
ao: new PropertyNode( this.properties, 'ao', 'f' ),
roughness: new PropertyNode( this.properties, 'roughness', 'f' ),
metalness: new PropertyNode( this.properties, 'metalness', 'f' ),
normalScale: new PropertyNode( this.properties, 'normalScale', 'v2' )
Expand Down Expand Up @@ -48,6 +52,40 @@ MeshStandardNode.prototype.build = function ( builder ) {

this.color = map ? new OperatorNode( color, map, OperatorNode.MUL ) : color;

// slots
// * emissive
// * emissiveMap
// * emissiveIntensity

var emissive = builder.findNode( props.emissive, inputs.emissive ),
emissiveMap = builder.resolve( props.emissiveMap ),
emissiveIntensity = builder.resolve( props.emissiveIntensity );

this.emissive = emissiveMap ? new OperatorNode( emissive, emissiveMap, OperatorNode.MUL ) : emissive;

if ( emissiveIntensity !== undefined ) {

this.emissive = new OperatorNode( this.emissive, emissiveIntensity, OperatorNode.MUL );

}

// slots
// * ao
// * aoMap
// * aoMapIntensity

var ao = builder.findNode( props.ao, inputs.ao ),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops this should be aoMapIntensity and included in shortcuts

Copy link
Collaborator

@sunag sunag Jul 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

aoMap = builder.resolve( props.aoMap ),
aoMapIntensity = builder.resolve( props.aoMapIntensity );

this.ao = aoMap ? new OperatorNode( ao, new SwitchNode( aoMap, "r" ), OperatorNode.MUL ) : ao;

if ( aoMapIntensity !== undefined ) {

this.ao = new OperatorNode( this.ao, aoMapIntensity, OperatorNode.MUL );

}

// slots
// * roughness
// * roughnessMap
Expand Down
10 changes: 5 additions & 5 deletions examples/js/nodes/utils/ColorSpaceNode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ function ColorSpaceNode( input, method ) {

this.input = input;

this.method = method || ColorSpaceNode.LINEAR;
this.method = method || ColorSpaceNode.LINEAR_TO_LINEAR;

}

Expand Down Expand Up @@ -203,15 +203,15 @@ ColorSpaceNode.LOG_LUV_TO_LINEAR = 'LogLuvToLinear';

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

ColorSpaceNode.prototype.generate = function ( builder, output ) {

var input = builder.context.input || this.input.build( builder, 'v4' ),
encodingMethod = builder.context.encoding !== undefined ? this.getEncodingMethod( builder.context.encoding ) : [ this.method ],
factor = this.factor ? this.factor.build( builder, 'f' ) : encodingMethod[ 1 ];
decodingMethod = builder.context.encoding !== undefined ? this.getDecodingMethod( builder.context.encoding ) : [ this.method ],
factor = this.factor ? this.factor.build( builder, 'f' ) : decodingMethod[ 1 ];

var method = builder.include( ColorSpaceNode.Nodes[ encodingMethod[ 0 ] ] );
var method = builder.include( ColorSpaceNode.Nodes[ decodingMethod[ 0 ] ] );

if ( factor ) {

Expand Down
7 changes: 5 additions & 2 deletions examples/webgl_loader_gltf.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@
<script src="js/Detector.js"></script>
<script src="js/libs/stats.min.js"></script>

<script>
<script type="module">

import './js/nodes/THREE.Nodes.js';

if ( ! Detector.webgl ) Detector.addGetWebGLMessage();

Expand Down Expand Up @@ -74,7 +76,7 @@
path + 'posz' + format, path + 'negz' + format
] );

scene = new THREE.Scene();
scene = window.scene = new THREE.Scene();
scene.background = envMap;

light = new THREE.HemisphereLight( 0xbbbbff, 0x444422 );
Expand All @@ -83,6 +85,7 @@

// model
var loader = new THREE.GLTFLoader();
loader.setUseNodes( true );
loader.load( 'models/gltf/DamagedHelmet/glTF/DamagedHelmet.gltf', function ( gltf ) {

gltf.scene.traverse( function ( child ) {
Expand Down
7 changes: 6 additions & 1 deletion examples/webgl_loader_gltf_extensions.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,10 @@
<script src="js/loaders/DDSLoader.js"></script>
<script src="js/loaders/GLTFLoader.js"></script>

<script>
<script type="module">

import './js/nodes/THREE.Nodes.js';

var orbitControls;
var container, camera, scene, renderer, loader;
var gltf, envMap, mixer, gui, extensionControls;
Expand Down Expand Up @@ -269,6 +272,8 @@

loader = new THREE.GLTFLoader();

loader.setUseNodes( true );

THREE.DRACOLoader.setDecoderPath( 'js/libs/draco/gltf/' );
loader.setDRACOLoader( new THREE.DRACOLoader() );

Expand Down