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
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
31ca6c2
First draft of khr_lights
MiiBond Jan 10, 2018
524fd6b
Updates to clarify units
MiiBond Jan 18, 2018
8cb0c17
Removing hemisphere light
MiiBond Jan 18, 2018
bd8938e
Update spotlight property description
MiiBond Jan 24, 2018
07a8fec
Adjustments to light descriptions for directional and spotlights
MiiBond Jan 25, 2018
2113228
Clarifying usage of ambient lights and colour space of lights
MiiBond Jan 29, 2018
e7ae552
Adding base node properties to lights
MiiBond Feb 5, 2018
0610717
Removing name properites from lights in KHR_lights example.
MiiBond Feb 5, 2018
72096c4
Adding light's name property back in for consistency
MiiBond Feb 6, 2018
e495b9e
Fix typos in schemas
MiiBond Feb 13, 2018
625b28e
Remove redundant bounds specification for inner and outer cone angles
MiiBond Mar 14, 2018
dbc99db
Fix inconsistency in spotlight angle bounds
MiiBond Mar 14, 2018
8326130
Updates to light schemas
MiiBond Mar 15, 2018
26f376f
Merge branch 'master' of github.com:KhronosGroup/glTF into khr_lights_v1
MiiBond Apr 3, 2018
0a79dc7
Move KHR_lights to match new folder structure
MiiBond Apr 3, 2018
6aff0cc
Few tweaks to address comments
MiiBond Apr 11, 2018
6e871f6
Adding range
MiiBond May 24, 2018
c605724
Add range property to schema
MiiBond May 24, 2018
766ba65
Remove reference to aesthetic reasons for using light range.
MiiBond May 30, 2018
b5dff39
Merge branch 'master' of github.com:KhronosGroup/glTF into khr_lights_v1
MiiBond Jun 27, 2018
8ae54ee
Rename extension and remove ambient lights
MiiBond Jun 27, 2018
3dcdfb0
Format exponents.
Jul 2, 2018
bd047d4
Update readme title.
Jul 3, 2018
34ae110
Update inner and outer angle description for spotlights
MiiBond Jul 6, 2018
382d3d6
Merge branch 'master' of github.com:KhronosGroup/glTF into khr_lights_v1
MiiBond Jul 6, 2018
be36985
Merge branch 'khr_lights_v1' of github.com:MiiBond/glTF into khr_ligh…
MiiBond Jul 6, 2018
2945499
Remove reference to UE4 and Frostbite
MiiBond Jul 6, 2018
08bc3c7
Remove more references to ambient lights
MiiBond Jul 12, 2018
28d2bac
Make it clearer that the spotlight code is for the attenuation betwee…
MiiBond Jul 12, 2018
78a93ae
Updating the list of contributors
MiiBond Aug 9, 2018
d9a6811
Updating the description for range
MiiBond Aug 9, 2018
50050e6
Merge branch 'master' of github.com:KhronosGroup/glTF into khr_lights_v1
MiiBond Aug 24, 2018
66dfb38
Change default light direction from pointing down +Z to pointing down -Z
MiiBond Aug 24, 2018
1048d16
Address several comments
MiiBond Sep 5, 2018
10ed0c6
Scale affects light orientation, not properties.
Oct 2, 2018
d9313cf
Remove space in filename.
Oct 3, 2018
fa6d985
Remove space in filename.
Oct 3, 2018
2d240ec
+z -> -z
Oct 3, 2018
93ae7df
Make 'light' required on node.
Oct 15, 2018
e4989c1
Make 'lights' required on top-level extension.
Oct 15, 2018
03f63e0
Use default values when 'spot' property is omitted.
Oct 15, 2018
f49c51e
Make spot property required when type is 'spot'.
Oct 23, 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
143 changes: 143 additions & 0 deletions extensions/Khronos/KHR_lights/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
# KHR\_lights

## Contributors

* Norbert Nopper, UX3D, <mailto:nopper@ux3d.io>
* Thomas Kress, UX3D, <mailto:kress@ux3d.io>
* Mike Bond, Adobe, <mailto:mbond@adobe.com>

## Status

Draft (not ratified yet)

## Dependencies

Written against the glTF 2.0 spec.

## Overview

This extension defines a set of lights for use with glTF 2.0.

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`.
Copy link

Choose a reason for hiding this comment

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

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


## Lights

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.
Copy link
Contributor

@donmccurdy donmccurdy Feb 25, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

^IMO these concerns are resolved.


A conforming implementation of this extension must be able to load light data defined in the asset and has to render the asset using those lights.

### Defining Lights

Lights are defined within an dictionary property in the glTF manifest file, by adding an `extensions` property to the top-level glTF 2.0 object and defining a `KHR_lights` property with a `lights` array inside it.

Each light defines a mandatory `type` property that designates the type of light (`ambient`, `directional`, `point` or `spot`). The following example defines a white-colored directional light.

```javascript
"extensions": {
"KHR_lights" : {
"lights": [
{
"color": [
1.0,
1.0,
1.0
],
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"type": "directional"
}
]
}
}
```

### 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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'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"?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


```javascript
"nodes" : [
{
"extensions" : {
"KHR_lights" : {
"light" : 0
}
}
}
]
```

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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


### Adding Light Instances to Scenes

Lights that have no transform information can be attached to scenes. `ambient` lights have no position, no orientation and no range. They are therefore global and belong to a scene rather than a node. These lights are attached to the scene by defining the `extensions.KHR_lights` property and, within that, an index into the `lights` array using the `light` property.

```javascript
"scenes" : [
{
"extensions" : {
"KHR_lights" : {
"light" : 0
}
}
}
]
```

### Light Types

All light types share the common set of properties listed below.

#### Light Shared Properties

| Property | Description | Required |
|:-----------------------|:------------------------------------------| :--------------------------|
| `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 |
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, yes I am. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


#### Ambient

Ambient lights define constant lighting throughout the scene. They are referenced only by a scene object and only 1 can be referenced per scene.

#### Directional

Directional lights are light sources that act as though they are infinitely far away and emit light 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).

#### Point

Point lights emit light in all directions from a position in space. The brightness of the light attenuates in a physically correct manner as distance increases from the light's position (i.e. brightness goes like the inverse square of the distance). Point light intensity is defined in candela, which is lumens per square radian (lm/sr).

#### Spot

Spot lights emit light in a cone over a distance. The angle and falloff of the cone is defined using two numbers, the `innerConeAngle` and `outerConeAngle`. As with point lights, the brightness also attenuates in a physically correct manner as distance increases from the light's position (i.e. brightness goes like the inverse square of the distance). Spot light intensity refers to the brightness inside the `innerConeAngle` and is defined in candela, which is lumens per square radian (lm/sr). Engines that don't support two angles for spotlights should use `outerConeAngle` as the spotlight angle (leaving `innerConeAngle` to implicitly be `0`).

| Property | Description | Required |
|:-----------------------|:------------------------------------------| :--------------------------|
| `innerConeAngle` | Angle, in radians, from centre of spotlight where falloff begins. Must be greater than `0`, less than `outerConeAngle` and less than `PI / 2.0`. | No, Default: `0` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Have a copy mistake here — based on default value and paragraph above, there is no requirement that innerConeAngle be greater than 0, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure that I follow. innerConeAngle should be greater than 0, shouldn't it?

I do have some redundancy here as I specify that outerConeAngle should be greater than 0 and greater than innerConeAngle when it would suffice to just mention innerConeAngle. Ditto for innerConeAngle being less than PI/2.

I'll clean that up.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is an inconsistency here, between Must be greater than '0' and Default: '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.

Ah, gotcha :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

| `outerConeAngle` | Angle, in radians, from centre of spotlight where falloff ends. Must be greater than `0`, greater than `innerConeAngle` and less than `PI / 2.0`. | No, Default: `PI / 4.0` |

```javascript
"extensions": {
"KHR_lights" : {
"lights": [
{
"spot": {
"innerConeAngle": 0.785398163397448,
"outerConeAngle": 1.57079632679,
},
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"color": [
1.0,
1.0,
1.0
],
"type": "spot"
}
]
}
}
```
Binary file not shown.
Loading