Add fill-extrude-height, fill-extrude-base, and light root property #495
Conversation
| /* | ||
| * { command: 'setLighting', args: [lightProperties] } | ||
| */ | ||
| setLighting: 'setLighting' |
There was a problem hiding this comment.
It looks like we use the name light rather than lighting elsewhere to refer to light properties. What do you think about changing this name to setLight?
There was a problem hiding this comment.
I'm fine with that — lighting was just instinctual but you're right that light would make more sense with the spec additions 👍
There was a problem hiding this comment.
The name and arguments of the command should match the API in gl-js.
There was a problem hiding this comment.
@scothis I changed it while you weren't looking 🙈
| "value": "number", | ||
| "default": [1.15, 210, 30], | ||
| "doc": "Light direction for lighting extruded geometries, in [r radial coordinate, a azimuthal angle, p polar angle] where r indicates the distance from the center of an object to its light, a indicates the position of the light relative to 0º (like a clock), and p indicates the height of the light (from 0º, directly above, to 180º, directly below). The light direction property will only be used if it has not been set by map options upon initialization.", | ||
| "example": [1.5, 90, 80] |
There was a problem hiding this comment.
Do you think our code would be more self-documenting if this were split into 3 separate properties?
There was a problem hiding this comment.
Maybe more self-documenting, but I hesitate because:
- the naming of spherical coordinates is so fraught as it is, and I'd rather loosely suggest names in the spec docstring than encode a certain naming convention in every style, and
- (related) using an array kind of allows a user to forget their "names" — even in my mind already, arr[0] is the length, arr[1] is the clock angle, arr[2] is the light height. Having to use "radial," "azimuthal," and "polar" in the style or, worse, in a
setLight()call, seems worse than supplying an array to "direction"
There was a problem hiding this comment.
Using an [x, y, z] array to describe a vector is very common. I'm less familiar with polar coordinates.
There was a problem hiding this comment.
It is common to express polar coordinates as a tuple as well. However, if the concern is that most developers would be unfamiliar with this convention, we could use the terms yaw, pitch, and roll (or heading, elevation, and bank), which are better understood than “radial”, “azimuthal”, and “polar”.
Whatever the case, I think we should be very explicit in our documentation for these properties, similar to Apple’s documentation for a similar construct.
There was a problem hiding this comment.
The radial coordinate doesn't have an equivalent in yaw, pitch, roll. Moreover, those descriptions of a person's head don't match up in my mind to descriptions of a light source outside someone's head.
I'm open to improvements to this spec description, but I think we should merge this for now.
|
|
||
| var newProps = {}; | ||
|
|
||
| for (var i in lightProps) { |
There was a problem hiding this comment.
Creating a minimal diff feels like overkill. We can check if !isEqual(before, after) and push a command for the entire after lighting object if so.
Iterating over each property was only necessary when the setLighting command accepted a single property per invocation.
| "type": "array", | ||
| "value": "number", | ||
| "default": [1.15, 210, 30], | ||
| "doc": "Light direction for lighting extruded geometries, in [r radial coordinate, a azimuthal angle, p polar angle] where r indicates the distance from the center of an object to its light, a indicates the position of the light relative to 0º (like a clock), and p indicates the height of the light (from 0º, directly above, to 180º, directly below). The light direction property will only be used if it has not been set by map options upon initialization.", |
There was a problem hiding this comment.
º is the masculine ordinal used for instance in the Spanish abbreviation “1º” (primero). The degree symbol is °. (On a Mac using the U.S. or Extended ABC keyboard layout, use ⇧⌥8 to access the degree symbol.)
| "example": "map" | ||
| }, | ||
| "direction": { | ||
| "type": "array", |
| "type": "array", | ||
| "value": "number", | ||
| "default": [1.15, 210, 30], | ||
| "doc": "Light direction for lighting extruded geometries, in [r radial coordinate, a azimuthal angle, p polar angle] where r indicates the distance from the center of an object to its light, a indicates the position of the light relative to 0º (like a clock), and p indicates the height of the light (from 0º, directly above, to 180º, directly below). The light direction property will only be used if it has not been set by map options upon initialization.", |
There was a problem hiding this comment.
I think it would be more intuitive to describe this property as the position of the light source relative to the feature, rather than the lighting direction. “Lighting direction” could be confused with the direction to which the light source points, as opposed to the position from which it comes. (Or am I misunderstanding this property?)
| "value": "number", | ||
| "default": [1.15, 210, 30], | ||
| "doc": "Light direction for lighting extruded geometries, in [r radial coordinate, a azimuthal angle, p polar angle] where r indicates the distance from the center of an object to its light, a indicates the position of the light relative to 0º (like a clock), and p indicates the height of the light (from 0º, directly above, to 180º, directly below). The light direction property will only be used if it has not been set by map options upon initialization.", | ||
| "example": [1.5, 90, 80] |
There was a problem hiding this comment.
It is common to express polar coordinates as a tuple as well. However, if the concern is that most developers would be unfamiliar with this convention, we could use the terms yaw, pitch, and roll (or heading, elevation, and bank), which are better understood than “radial”, “azimuthal”, and “polar”.
Whatever the case, I think we should be very explicit in our documentation for these properties, similar to Apple’s documentation for a similar construct.
| "maximum": 1, | ||
| "minimum": 0, | ||
| "default": 0.5, | ||
| "doc": "Intensity of lighting (on a scale from 0 to 1). Higher numbers will present as more extreme contrast. The light intensity property will only be used if it has not been set by map options upon initialization." |
There was a problem hiding this comment.
We should spell out the visual effect of the default value of 0.5. (By the way, this Apple documentation may be helpful in figuring out the right way to discuss light intensity for physical versus non-physical lighting.)
| "property-function": true, | ||
| "default": 0, | ||
| "minimum": 0, | ||
| "doc": "The height with which to extrude this fill layer.", |
| "property-function": true, | ||
| "default": 0, | ||
| "minimum": 0, | ||
| "doc": "The height with which to extrude the base of this layer.", |
| "map", | ||
| "viewport" | ||
| ], | ||
| "doc": "Whether extruded geometries are lit relative to map or viewport. The light anchor property will only be used if it has not been set by map options upon initialization.", |
There was a problem hiding this comment.
Should this be light-anchor or light.anchor? Should be in backticks in any case.
| "map", | ||
| "viewport" | ||
| ], | ||
| "doc": "Whether extruded geometries are lit relative to map or viewport. The light anchor property will only be used if it has not been set by map options upon initialization.", |
| "value": "number", | ||
| "length": 3, | ||
| "default": [1.15, 210, 30], | ||
| "doc": "Position of the light source relative to lit (extruded) geometries, in [r radial coordinate, a azimuthal angle, p polar angle] where r indicates the distance from the center of an object to its light, a indicates the position of the light relative to 0° (like a clock), and p indicates the height of the light (from 0°, directly above, to 180°, directly below). The light direction property will only be used if it has not been set by map options upon initialization.", |
There was a problem hiding this comment.
- Is the distance in meters?
- Which direction does a = 0° point? (Also, clockwise, not "like a clock".)
- Is the distance and angle measured from the volumetric center of the extruded geometry, or from the surface?
| "doc": "Whether extruded geometries are lit relative to the map or viewport. The `light.anchor` property will only be used if it has not been set by map options upon initialization.", | ||
| "example": "map" | ||
| }, | ||
| "direction": { |
There was a problem hiding this comment.
Given the current documentation, I think you should strongly consider renaming the property to "position", even if the goal of setting this property is to change the lighting direction. (You can say as much in the documentation.)
| "values": [ | ||
| "map", | ||
| "viewport" | ||
| ], |
There was a problem hiding this comment.
The format for enum values changed in #510. 😬
| "transition": true, | ||
| "requires": [ | ||
| { | ||
| "<=": "fill-extrude-height" |
There was a problem hiding this comment.
This <= key to a requires object is new, right? So its introduction will require changes to documentation generation code and Studio.
…line-color to extrusion-edge-color
Uses the `setLightProperty` command with two args: property name and
value.
{ command: 'setLightProperty', args: [property, value] }
|
ok ready ready ready |
| @@ -68,7 +68,7 @@ | |||
| "function": "interpolated", | |||
| "zoom-function": true, | |||
| "property-function": false, | |||
| "doc": "Position of the light source relative to lit (extruded) geometries, in [r radial coordinate, a azimuthal angle, p polar angle] where r indicates the distance from the center of an object to its light, a indicates the position of the light relative to 0° (like a clock), and p indicates the height of the light (from 0°, directly above, to 180°, directly below).", | |||
| "doc": "Position of the light source relative to lit (extruded) geometries, in [r radial coordinate, a azimuthal angle, p polar angle] where r indicates the distance from the center of an object to its light, a indicates the position of the light relative to 0° (0° corresponds to 12:00 on a clock, and degrees proceed clockwise; 0° with `light.anchor: viewport` corresponds to the top of the viewport, or with `light.anchor: map` to due north), and p indicates the height of the light (from 0°, directly above, to 180°, directly below).", | |||
There was a problem hiding this comment.
“12:00 on a clock” is no longer necessary, because “top of the viewport” and “due north” are descriptive enough.
Replace:
with
light.anchor: viewport
with:
when
light.anchoris set toviewport
The iOS and macOS SDKs have a script that translates the construct to something more appropriate for the language, so it’s best to use it when possible.
This
fill-extrude-heightandfill-extrude-baseas fill layer paint propertieslightroot object with the following properties, for use in lighting extruded features:anchorcolordirectionintensityPart of mapbox/mapbox-gl-js#684, result of discussion on #456.
cc @lucaswoj @jfirebaugh @mourner @mollymerp @scothis @nickidlugash