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

KHR_lights_punctual extension proposal #1223

Merged
merged 42 commits into from Oct 31, 2018

Conversation

@MiiBond
Copy link
Contributor

commented Jan 25, 2018

No description provided.


#### Directional

Directional lights are light sources that emit from infinitely far away in the direction of the -z axis. This light type inherits the orientation of the node that it belongs to. Because it is at an infinite distance, the light is not attenuated. It's intensity is defined in lumens per metre squared, or lux (lm/m^2).

This comment has been minimized.

Copy link
@donmccurdy

donmccurdy Jan 25, 2018

Member

... are light sources that emit from infinitely far away in the direction of the -z axis.

I think this could be phrased more clearly, but struggling with it. What about: "... are light sources infinitely far away in the direction of the -z axis, emitting light in the direction of the +z axis".

This comment has been minimized.

Copy link
@MiiBond

MiiBond Jan 25, 2018

Author Contributor

Adjusted.

| Property | Description | Required |
|:-----------------------|:------------------------------------------| :--------------------------|
| `innerConeAngle` | Angle from centre of spotlight where falloff begins. | No, Default: `0` |
| `outerConeAngle` | Angle from centre of spotlight where falloff ends. Must be greater than innerConeAngle. | No, Default: `PI / 2.0` |

This comment has been minimized.

Copy link
@donmccurdy

donmccurdy Jan 25, 2018

Member

Let's specify explicitly that each angle is in radians.

This comment has been minimized.

Copy link
@donmccurdy

donmccurdy Jan 25, 2018

Member

In three.js, neither angle may exceed PI / 2. I only have some old screenshots to go by, but Unreal appears to have the same requirement. So probably we should specify a min/max here.

In that case, maybe the default outerConeAngle value should be less extreme than 90º?

This comment has been minimized.

Copy link
@MiiBond

MiiBond Jan 25, 2018

Author Contributor

Good call.

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

Fixes #945

@pjcozzi pjcozzi added the extension label Jan 31, 2018

|:-----------------------|:------------------------------------------| :--------------------------|
| `color` | RGB value for light's color in linear space. | No, Default: `[1.0, 1.0, 1.0]` |
| `intensity` | Brightness of light in. The units that this is defined in depend on the type of light. `point` and `spot` lights use luminous intensity in candela (lm/sr) while `directional` lights use illuminance in lux (lm/m^2) | No, Default: `1.0` |
| `type` | Declares the type of the light. | :white_check_mark: Yes |

This comment has been minimized.

Copy link
@takahirox

takahirox Feb 1, 2018

Contributor

Perhaps you're missing name property here?
In examples/lights.gltf light has name property.

This comment has been minimized.

Copy link
@MiiBond

MiiBond Feb 5, 2018

Author Contributor

Yes, yes I am. Thank you.

This comment has been minimized.

Copy link
@MiiBond

MiiBond Feb 5, 2018

Author Contributor

Wait, actually, I think this is just a problem with my example. My intension was for lights to use the name of the node that they are attached to so the name in the examples shouldn't be there.

This comment has been minimized.

Copy link
@donmccurdy

donmccurdy Feb 6, 2018

Member

About removing name entirely. I wish it weren't there on meshes or cameras, because in my mind the node is a mesh or a camera, it doesn't contain them, and so one of the two names must be thrown away in loading. But since both do have names in the current spec, it would be more consistent to keep one here. I'm undecided on this..

This comment has been minimized.

Copy link
@MiiBond

MiiBond Feb 6, 2018

Author Contributor

Ah, you're right. I guess that's why I had it there. It is pretty odd to have a name on both the node and the light objects but, as it's consistent, I guess I'll put it back.

@najadojo

This comment has been minimized.

Copy link

commented Feb 2, 2018

I'd really like to see light properties be animatable. Could be an extension of animation/channel/target where a property name (i.e. color, intensity) is given and the output applies to the light's field. Is this for khr_light or a theoretical khr_animatable_property?


### Adding Light Instances to Nodes

Nodes can have lights attached to them, just like any other objects that have a transform. Only lights of types `directional`, `point`, and `spot` have position and/or orientation so only these light types are allowed. These lights are attached to a node by defining the `extensions.KHR_lights` property and, within that, an index into the `lights` array using the `light` property.

This comment has been minimized.

Copy link
@willeastcott

willeastcott Feb 2, 2018

Contributor

I'm unsure why cameras are defined entirely on a node, but lights have to have this extra indirection of using instances. What's the reason for the difference? Certainly in PlayCanvas, lights are not instanced. Maybe other engines do this, but I'm not sure what the point of that would be since light are very light weight. Same deal for cameras. Personally, I'd just define directional, points and spots as valid for a node extension and ambient for the scene extension, and not have the top level array.

This comment has been minimized.

Copy link
@MiiBond

MiiBond Feb 5, 2018

Author Contributor

I'm not sure I follow. Lights are defined on a node exactly the same way cameras are.
Perhaps my use of the word "attached" is what's confusing you? If so, maybe I can rephrase this to something like "light properties are referenced by a node"?

This comment has been minimized.

Copy link
@willeastcott

willeastcott Feb 7, 2018

Contributor

Really? Are you sure? Cameras are wholly defined in the node JSON, right? Lights seem to define a light index on the node with the actual light defined in an array stored in the extensions object in the top-level glTF object. So I was curious as to the reason why they are defined differently in this way.

This comment has been minimized.

Copy link
@MiiBond

MiiBond Feb 7, 2018

Author Contributor

Cameras are defined in an array, just like meshes and lights at the top-level of the glTF object.

This comment has been minimized.

Copy link
@willeastcott

willeastcott Feb 7, 2018

Contributor

Haha. How did I get confused about that?? I withdraw all my previous comments. I should have checked the PlayCanvas glTF parser code to make sure before diving in. D'oh.

1.0,
1.0,
1.0
],

This comment has been minimized.

Copy link
@recp

recp Feb 6, 2018

Does this mean the extension will only support RGB color for lights (not including RGBA)? I don't know if alpha channel has a mean for lights but I used vec4 (RGBA) to specify light color in my shaders :-S

This comment has been minimized.

Copy link
@MiiBond

MiiBond Feb 6, 2018

Author Contributor

Yeah, I'm not aware of a use case (or, at least not a common use case) for alpha in a light's colour.

"spot": {
"innerConeAngle": 0.785398163397448,
"outerConeAngle": 1.57079632679,
},

This comment has been minimized.

Copy link
@recp

recp Feb 6, 2018

since type specifies it is "spot" why it used here again? Obviously it simplifies to access the data after getting type but it could be better name : "cone" (because we already know spot has cone shape):

"cone": {
  "innerAngle": 0.785398163397448,
  "outerAngle": 1.57079632679,
}

it also reduces a few bytes :) it is just feedback.

This comment has been minimized.

Copy link
@donmccurdy

donmccurdy Feb 6, 2018

Member

I think keeping the name spot for the options related to "spot" lights is clearest, and probably consistent with what we'd want for future light types. No preference on keeping Cone in the name or not, though.

This comment has been minimized.

Copy link
@MiiBond

MiiBond Feb 6, 2018

Author Contributor

Yeah, I've debated removing "Cone" from the property names too but I kind of like the explicit description.

@ghost
Copy link

left a comment

A few small suggestions for making everything more consistent. Overall, this is looking great!


Many 3D tools and engines support built-in implementations of light types. Using this extension, tools can export and engines can import these lights.

This extension defines five light types: `ambient`, `directional`, `point` and `spot`.

This comment has been minimized.

Copy link
@ghost

ghost Feb 13, 2018

This should read four now that hemispherical lights have been removed.

"$schema": "http://json-schema.org/draft-04/schema",
"title": "light",
"type": "object",
"description": "An ambient, hemisphere, directional, point, or spot light.",

This comment has been minimized.

Copy link
@ghost

ghost Feb 13, 2018

This description should no longer mention hemispherical lights.

},
"intensity": {
"type": "number",
"description": "Intensity of the light source in lumens.",

This comment has been minimized.

Copy link
@ghost

ghost Feb 13, 2018

It should probably be clarified that the intensity either has units of candela or lux, depending on the light type.

"description": "Specifies the light type.",
"enum": [
"ambient",
"hemisphere",

This comment has been minimized.

Copy link
@ghost

ghost Feb 13, 2018

"hemisphere" should be removed here.

"innerConeAngle" : {
"type" : "number",
"description" : "Angle in radians from centre of spotlight where falloff begins.",
"default" : 0.785398163397448

This comment has been minimized.

Copy link
@ghost

ghost Feb 13, 2018

Either this default should be updated to what the spec says (0), or vice-versa.

"outerConeAngle" : {
"type" : "number",
"description" : "Angle in radians from centre of spotlight where falloff ends.",
"default" : 1.570796326794897

This comment has been minimized.

Copy link
@ghost

ghost Feb 13, 2018

Either this default should be updated to what the spec says (pi/4), or vice-versa.

This comment has been minimized.

Copy link
@MiiBond

MiiBond Feb 13, 2018

Author Contributor

All good finds. Thanks!

```

For light types that have a position (`point` and `spot` lights), the light's position is defined as the node's world location.
For light types that have a direction (`directional` and `spot` lights), the light's direction is defined as the 3-vector `(0.0, 0.0, 1.0)` and the rotation of the node orients the light accordingly.

This comment has been minimized.

Copy link
@takahirox

takahirox Feb 21, 2018

Contributor

Does this mean the direction in a world space will be "light node's world rotation matrix * Vector3(0.0, 0.0, 1.0)"?

],
"extensions": {
"KHR_lights": {
"light": 0

This comment has been minimized.

Copy link
@donmccurdy

donmccurdy Feb 25, 2018

Member

This suggests a scene may only contain a single ambient light; is that intended?

This comment has been minimized.

Copy link
@donmccurdy

donmccurdy Feb 25, 2018

Member

Ah, just saw the comment above.


Lights define light sources within a scene.

`ambient` lights are not transformable and, thus, can only be defined on the scene. All other lights are contained in nodes and inherit the transform of that node.

This comment has been minimized.

Copy link
@donmccurdy

donmccurdy Feb 25, 2018

Member

Thinking through implications here:

  1. In the current syntax, this also means a scene may contain no more than 1 ambient light.
  2. If we were to support animating light properties in the future (e.g. intensity, color) ambient lights will be in a slightly awkward position, such that they have an index in the lights array, whereas all other animated properties are targeted by node. If the same ambient light is reused in multiple scenes the animation syntax must clarify which instance of the ambient light is targeted.
  3. Given issues above, and for consistency, would it be better to allow ambient lights' use in nodes like other lights, but say (either a strict requirement or implementation note + validation warning) that the node must be an immediate child of the scene and have the identity transform?

This comment has been minimized.

Copy link
@donmccurdy

donmccurdy Mar 7, 2018

Member

^IMO these concerns are resolved.

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Feb 25, 2018

Added a couple comments, my remaining concern is that ambient lights might be in an awkward spot for future animation support... I don't have a strong opinion whether animation should be included in this extension or a separate future version, but let's discuss @najadojo's suggestion enough to ensure this syntax lends itself to animation in the future, even if we don't add it right away..

Ideal animation syntax, based on existing spec, would simply add new allowed values to channel.path:

{
    "sampler": 0,
    "target": {
        "node": 25,
        "path": "intensity"
    }
}

Unfortunately, attaching ambient lights to the scene throws a slight wrinkle into that syntax. I think we may want to keep them in nodes, despite not taking a transform?

@MiiBond

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

I'm not that familiar with animations as they are. Does an animation have to target a node? What about the property of a material?

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

Animations may (currently) target: node + path, where node is a node index, and path is one of: rotation, scale, translation, weights. We haven't discussed animation on materials yet, but I would expect animation to affect only a particular instance of a material rather than all uses if we did.

@willeastcott

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

Well, animations can target morph target weights at the moment. So the target string can already apply to non-node properties. You could have 'ambientColor' as a target, say, if ambient was a 'scene' property. I was thinking 'target' should really be a specific path to a specific property.

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

For morph targets, the target must still provide a node index — weights on all morph targets within that node are animated.

@emackey

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

Just tested Blender 2.79. For final renders, with either Cycles or Blender Render, it appears to follow all 3 rules.

One wrinkle, in the realtime preview mode (Blender Render, "Material" mode 3D preview) the spot light cone shape can be distorted by non-uniform node scaling. But this seems like a bug because the effect is not present in the final render.

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

Pushed a new commit with updated language for node scale: 10ed0c6

donmccurdy added some commits Oct 3, 2018

@lexaknyazev

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

Last JSON nits.

In the core glTF, all top-level arrays are optional. Do we want to keep it the same for this extension (and for similar extensions in general)? I.e., should this example be valid?

{
  "asset": {
    "version": "2.0",
    "extensionsUsed": ["KHR_lights_punctual"]
  },
  "extensions": {
    "KHR_lights_punctual": { }
  }
}

Similar question about the lack of light property inside node.

{
  "asset": {
    "version": "2.0",
    "extensionsUsed": ["KHR_lights_punctual"]
  },
  "nodes": [
    {
      "extensions": {
        "KHR_lights_punctual": { }
      }
    }
  ]
}

And the last one about undefined spot object for spot light. In the example below, the second light gets values for spot properties implicitly from the schema, while the first is under-defined:

{
  "asset": {
    "version": "2.0",
    "extensionsUsed": ["KHR_lights_punctual"]
  },
  "extensions": {
    "KHR_lights_punctual": {
      "lights": [
        {
          "type": "spot"
        },
        {
          "type": "spot",
          "spot": { }
        }
      ]
    }
  }
}
@donmccurdy

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

First reactions —

  • require top-level lights array — No preference.
  • require light in node extension entry — I'd prefer light be required; this feels like a distinct case from mesh or camera being optional.
  • undefined spot object — Agree this should be defined. Inheriting implicit defaults from the schema, like with {}, seems best I think?
@donmccurdy

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

Are we ready to merge this, keeping "draft" status? We can then delegate any final tweaks to the JSON schema.

donmccurdy added some commits Oct 15, 2018

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

I've marked 'light' and 'lights' as required; stricter schema seems preferable. I don't think the core spec provides a precedent one way or another, since the extension should not be used if there are no lights.

Added a note that when the spot property is omitted, its default values should be used:

When the spot property is omitted, a spot light must use default values for innerConeAngle and outerConeAngle.

The alternative would be to make it required, when type=spot, I suppose?

@UX3D-nopper

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

@emackey

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

Assume it becomes into core glTF, we need to change the schema.

Moving anything from an extension to core is a breaking change to schema. It should be no problem to require it in the extension, while leaving lights optional in future core.

@lexaknyazev

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

The alternative would be to make it required, when type=spot, I suppose?

That's how camera.type is defined but there're required fields in perspective and orthogonal.

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

As discussed earlier, I've marked the 'spot' property as required when type='spot'.

@dakom

This comment has been minimized.

Copy link

commented Oct 30, 2018

I'm a bit confused by the status of WebGL implementations as a reference point. Loading the PointLightTest.gltf:

  1. Babylon.js: https://sandbox.babylonjs.com/

screen shot 2018-10-30 at 8 12 03

  1. Three.js: https://gltf-viewer.donmccurdy.com/

screen shot 2018-10-30 at 8 17 44

Both of those can be adjusted by changing some settings, but I couldn't get them to match eachother, and neither of them seem to match the reference output from Blender:

pointlighttest_blender

Any tips to get the blender reference, in webgl?

Similarly... are they calculating the falloff the same way? I couldn't quite tell from peeking at the code:

https://github.com/BabylonJS/Babylon.js/blob/master/src/Shaders/ShadersInclude/pbrLightFunctions.fx#L37

vs.

https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/bsdfs.glsl#L3

@donmccurdy

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

My viewer was not up to date with this extension, but should be now. Note that three.js implementations will have to enable the physicallyCorrectLighting mode to get the right results:

three.js:
localhost_3000_

babylonjs:
babylonjs

unreal:
unreal

ux3d:
ux3d

Looks like the BabylonJS Sandbox has other light in the scene (my viewer disables them if the incoming model has its own lighting) which is likely the difference between the screenshot above and what you saw.

I would advise trying to match the implementations above rather than Blender's output, for now. See KhronosGroup/glTF-Blender-IO#24.

@dakom

This comment has been minimized.

Copy link

commented Oct 30, 2018

Oh wow, thank you for the explanation and update!

Part of what was driving me mad was wondering why the blender output has those harsh specular highlights... so if I understand correctly, we do not want those highlights in either the blender output or the renderers above (i.e. blender is currently wrong)?

@bghgary

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

BabylonJS Sandbox has other light in the scene

The default IBL is what is causing the difference for the sandbox. You can turn it off by typing this in the dev console:

BABYLON.Engine.LastCreatedScene.environmentTexture = null

@dakom

This comment has been minimized.

Copy link

commented Oct 30, 2018

woohoo, great - now seeing the same output as above. Thanks y'all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.