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

Jump to bottom

Rough update to KHR_lights support to match current spec #13341

Merged
merged 7 commits into from
Apr 6, 2018
Merged

Rough update to KHR_lights support to match current spec #13341

merged 7 commits into from
Apr 6, 2018

Conversation

MiiBond
Copy link
Contributor

@MiiBond MiiBond commented Feb 15, 2018

Updated to match current spec.
KhronosGroup/glTF#1223

@mrdoob mrdoob added this to the r91 milestone Feb 16, 2018
@MiiBond MiiBond mentioned this pull request Feb 16, 2018
Closed
@donmccurdy
Copy link
Collaborator

For the renderer light settings, I think it's ok just to add:

// The KHR_lights extension should use physically-correct lighting mode.
renderer.physicallyCorrectLights = true;

... to the entire example, no need to turn it on/off when the extension is selected.

@mrdoob
Copy link
Owner

mrdoob commented Mar 3, 2018

@donmccurdy looks good?

@donmccurdy
Copy link
Collaborator

@mrdoob we should plan this one for r92, more likely, it will about a week before we've answered a couple last questions about this extension (partly my fault on that 😅).

@donmccurdy
Copy link
Collaborator

@MiiBond I think i've come around for having ambient lights on the scene... do you want to include that change in this PR?

@mrdoob mrdoob modified the milestones: r91, r92 Mar 3, 2018
@MiiBond
Copy link
Contributor Author

MiiBond commented Mar 3, 2018

@donmccurdy I think this pull request handles the ambient light on the scene, doesn't it? Or did you mean something else?
This pull request doesn't attempt to limit the number of ambient lights or prevent them from being on a node or anything but I gather we're not relying too much on the loader validating the glTF.

@MiiBond
Copy link
Contributor Author

MiiBond commented Mar 3, 2018

It did just occur to me that the handling of direction for SpotLight and DirectionalLight are wrong. In three.js, the direction doesn't come from the matrix, right? It's dependent on the position and target of the light?

@donmccurdy
Copy link
Collaborator

I think this pull request handles the ambient light on the scene, doesn't it? Or did you mean something else?

Oh! My mistake, I'd expected to see something in your PR checking for a light in the scene, but I guess that part was already here.

In three.js, the direction doesn't come from the matrix, right? It's dependent on the position and target of the light?

Hmm yeah. If we want a spotlight to point correctly in a direction relative to its local transform, even if the node is animated, perhaps we do this:

light.target.position.set( 0, 0, 1 );
light.add( light.target );

@MiiBond
Copy link
Contributor Author

MiiBond commented Mar 7, 2018

@donmccurdy Yeah, I think that works.
I've updated the pull request.

@mrdoob
Copy link
Owner

mrdoob commented Mar 8, 2018

Can we replace the current Monster.gltf model with the one with a light instead of having 2 Monster.gltf?

@MiiBond
Copy link
Contributor Author

MiiBond commented Mar 8, 2018

@mrdoob I just followed the convention already being used and just added a new one for the new extension. There are already 5 other copies of monster.gltf; one for each extension.

@mrdoob
Copy link
Owner

mrdoob commented Mar 8, 2018

I see...

@donmccurdy
Copy link
Collaborator

We could drop glTF-Embedded/Monster/*, to just keep the current count.

For a future cleanup PR, I don't think we need to have all of glTF, glTF-Embedded, and glTF-Binary for every model — has been a long time since we've seen any bugs related to container structure. Keeping extensions around that add functionality like pbrSpecularGlossiness and lights seems more important.

@mrdoob
Copy link
Owner

mrdoob commented Apr 4, 2018

/ping @donmccurdy

donmccurdy
donmccurdy reviewed Apr 4, 2018
View reviewed changes
examples/webgl_loader_gltf_extensions.html
@@ -202,14 +202,13 @@
var ambient = new THREE.AmbientLight( 0x222222 );
scene.add( ambient );

var directionalLight = new THREE.DirectionalLight( 0xdddddd );
var directionalLight = new THREE.DirectionalLight( 0xdddddd, 4 );
Copy link
Collaborator

@donmccurdy donmccurdy Apr 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why the intensity increase here? Related to physicallyCorrectLights setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, yeah. That was a while ago...

donmccurdy
donmccurdy approved these changes Apr 4, 2018
View reviewed changes
Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Looks good — We can remove some other example models in another PR.

mrdoob
mrdoob reviewed Apr 4, 2018
View reviewed changes
examples/js/loaders/GLTFLoader.js Outdated
@@ -261,7 +262,14 @@ THREE.GLTFLoader = ( function () {

case 'spot':
lightNode = new THREE.SpotLight( color );
lightNode.position.set( 0, 0, 1 );
// Handle spotlight properties.
var spot = light.spot || {};
Copy link
Owner

Choose a reason for hiding this comment

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

How about doing this instead?

var spot = light.spot || { innerConeAngle: 0, outerConeAngle: Math.PI / 4.0 };
lightNode.angle = spot.outerConeAngle;
lightNode.penumbra = 1.0 - ( spot.innerConeAngle / spot.outerConeAngle );
lightNode.target.position.set( 0, 0, 1 );
lightNode.add( lightNode.target );

Copy link
Collaborator

Choose a reason for hiding this comment

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

innerConeAngle and outerConeAngle are not required to be present, so their default values must be set independently. @MiiBond is the spot property required? Perhaps:

light.spot = light.spot || {};
light.spot.innerConeAngle = light.spot.innerConeAngle !== undefined ? light.spot.innerConeAngle : 0;
light.spot.outerConeAngle = light.spot.innerConeAngle !== undefined ? light.spot.innerConeAngle : Math.PI / 4.0;
// ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spot isn't required, no.

I'm not sure I follow what you're suggesting with your change. Are you just suggesting to get rid of the variable declarations within the case statement?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that is the idea; i'm happy with or without that change.

mrdoob
mrdoob reviewed Apr 4, 2018
View reviewed changes
examples/js/loaders/GLTFLoader.js Outdated
var outerConeAngle = spot.outerConeAngle !== undefined ? spot.outerConeAngle : Math.PI / 4.0;
lightNode.angle = outerConeAngle;
lightNode.penumbra = 1.0 - innerConeAngle / outerConeAngle;
lightNode.target.position.set( 0, 0, 1 );
Copy link
Owner

Choose a reason for hiding this comment

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

The target of a spot is 0, 0, 1 by default?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The local direction of the glTF light is always +Z; the only way to rotate it is by rotating the object or its parents in the scene graph.

@mrdoob mrdoob merged commit b69d7c7 into mrdoob:dev Apr 6, 2018
@mrdoob
Copy link
Owner

mrdoob commented Apr 6, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrdoob mrdoob mrdoob left review comments

@donmccurdy donmccurdy donmccurdy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
r92
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@MiiBond @donmccurdy @mrdoob