Skip to content

GLTFLoader: Fix lights list iterator. #14515

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

Merged
merged 3 commits into from
Jul 25, 2018
Merged

GLTFLoader: Fix lights list iterator. #14515

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8dbca07
GLTFLoader: Fix lights list type.
Jul 20, 2018
c7a988a
GLTFLoader: Clean up KHR_lights_punctual impl.
Jul 20, 2018
a7b7151
GLTFLoader: Clean up loop iterator.
Jul 20, 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
48 changes: 19 additions & 29 deletions examples/js/loaders/GLTFLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,28 +272,22 @@ THREE.GLTFLoader = ( function () {

this.name = EXTENSIONS.KHR_LIGHTS_PUNCTUAL;

this.lights = {};
this.lights = [];

var extension = ( json.extensions && json.extensions[ EXTENSIONS.KHR_LIGHTS_PUNCTUAL ] ) || {};
var lights = extension.lights || {};
var lightDefs = extension.lights || [];

for ( var lightId in lights ) {
for ( var i = 0; i < lightDefs.length; i ++ ) {

var light = lights[ lightId ];
var lightDef = lightDefs[ i ];
var lightNode;

// the color default value is [1, 1, 1]
var color = new THREE.Color( 0xffffff );
if ( light.color !== undefined ) {
color.fromArray( light.color )
}
if ( lightDef.color !== undefined ) color.fromArray( lightDef.color );

var range = 0;
if ( light.range !== undefined ) {
range = light.range;
}
var range = lightDef.range !== undefined ? lightDef.range : 0;

switch ( light.type ) {
switch ( lightDef.type ) {

case 'directional':
lightNode = new THREE.DirectionalLight( color );
Expand All @@ -310,31 +304,27 @@ THREE.GLTFLoader = ( function () {
lightNode = new THREE.SpotLight( color );
lightNode.distance = range;
// Handle spotlight properties.
light.spot = light.spot || {};
light.spot.innerConeAngle = light.spot.innerConeAngle !== undefined ? light.spot.innerConeAngle : 0;
light.spot.outerConeAngle = light.spot.outerConeAngle !== undefined ? light.spot.outerConeAngle : Math.PI / 4.0;
lightNode.angle = light.spot.outerConeAngle;
lightNode.penumbra = 1.0 - light.spot.innerConeAngle / light.spot.outerConeAngle;
lightDef.spot = lightDef.spot || {};
lightDef.spot.innerConeAngle = lightDef.spot.innerConeAngle !== undefined ? lightDef.spot.innerConeAngle : 0;
lightDef.spot.outerConeAngle = lightDef.spot.outerConeAngle !== undefined ? lightDef.spot.outerConeAngle : Math.PI / 4.0;
lightNode.angle = lightDef.spot.outerConeAngle;
lightNode.penumbra = 1.0 - lightDef.spot.innerConeAngle / lightDef.spot.outerConeAngle;
lightNode.target.position.set( 0, 0, 1 );
lightNode.add( lightNode.target );
break;

}

if ( lightNode ) {

lightNode.decay = 2;
default:
throw new Error( 'THREE.GLTFLoader: Unexpected light type, "' + lightDef.type + '".' );

if ( light.intensity !== undefined ) {
}

lightNode.intensity = light.intensity;
lightNode.decay = 2;

}
if ( lightDef.intensity !== undefined ) lightNode.intensity = lightDef.intensity;

lightNode.name = light.name || ( 'light_' + lightId );
this.lights[ lightId ] = lightNode;
lightNode.name = lightDef.name || ( 'light_' + i );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't new in this PR but I think the following would be better

lightNode.name = lightDef.name !== undefined ? lightDef.name : ( 'light_' + i );

because lightDef.name can be empty strings ''.

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW any special reasons why you wanna name light as light_num if lightDef.name isn't defined? I think user may want the empty strings name (default in Three.js) in that case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See KhronosGroup/glTF-Validator#93 — I'm increasingly thinking we should ensure that all nodes have unique names, even if it means changing user-specified names. Duplicate names are a common source of animation bugs. Because empty strings are practically guaranteed to be non-unique, I don't think we should be trying to preserve those.

Copy link
Collaborator

@takahirox takahirox Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm one of the users who don't want loader to pollute the names as default (even if it's an empty strings name). And currently glTF spec allows non-unique name.

But I can understand non-unique name can sometimes cause problems so wondering if I can first add a method to SceneUtils or somewhere which ensures non-unique name in a scene and ask users to explicitly call. We can move it into the loader later if glTF spec becomes to recommend or request unique name.

Let's make a thread and discuss unique-name handling in Three.js there when necessary. And I want glTF spec to update or add a node about unique name first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The validator thread (KhronosGroup/glTF-Validator#93) goes into some detail on this — there are many tools that expect or require uniquely named objects, even though the glTF spec does not. Currently renaming seems to be the recommended way to handle this. But I would be OK with consolidating the renaming logic into one place rather than having one-offs like this, whether that's within GLTFLoader somewhere or in a Utils class.

Technically we don't need globally unique names — just unique within the asset, so that an AnimationMixer has references to nodes that are both (a) unique and (b) portable if animations are loaded from a different file than the mesh.

Copy link
Collaborator

@takahirox takahirox Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

there are many tools that expect or require uniquely named objects, even though the glTF spec does not.

I think first glTF spec should discuss and add a note or update the spec, it'd be a lack of consideration for asset runs anywhere. And if glTF spec requests unique name, the situation would be much simpler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You may want to comment on the thread, then. The current consensus is that the spec will not require unique names.

Copy link
Collaborator

@takahirox takahirox Jul 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I won't because I don't request unique-name in the spec. Rather than that, I wanna first think how to resolve animation issue with non-unique name without name pollution in Three.js.


}
this.lights.push( lightNode );

}

Expand Down