Skip to content
This repository has been archived by the owner on Apr 10, 2018. It is now read-only.

Add fill-extrude-height, fill-extrude-base, and light root property #495

Merged
merged 31 commits into from
Oct 7, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
1d4e02e
Add some (dummy) building specs
May 25, 2016
277b7f3
Building shadow colors
May 25, 2016
ab5e653
building -> extrusion
May 26, 2016
ccbdf0d
add extrusion-lighting-anchor to spec
Jun 3, 2016
40b2a36
Add extrusion-height and extrusion-min-height
Jun 13, 2016
348b476
Move lighting-anchor to be a root property; add extrusion-layer-opacity
Jun 14, 2016
256c1b8
Antialias and shadow-color: not property fns
Jun 24, 2016
b5b6be2
Incorporate root light properties
Aug 17, 2016
f317d2e
Remove extrusion-shadow-color [ci skip]
Aug 17, 2016
b07472a
Remove extrusion-opacity (per-feature)
Aug 19, 2016
cf1f41c
Add light-intensity; remove extrusion-antialias; rename extrusion-out…
Aug 25, 2016
75a3cb4
Un-namespace light subproperties, modify a few defaults (not final)
Aug 31, 2016
139bbf2
extrusion-height default 1
Aug 31, 2016
e88937e
Add lighting properties to the style diff
scothis Aug 31, 2016
b814b4c
extrusion-min-height -> extrusion-base-height
Sep 1, 2016
a89f6fc
Nix the extrusion type; add fill-extrude-height and fill-extrude-base
Sep 2, 2016
bc2fc7d
Change from cartesian to spherical coordinates + document them
Sep 9, 2016
d03f412
Update diff.js to reflect lighting API changes
Sep 16, 2016
b1046f8
Update diff tests
Sep 16, 2016
7bbb3e4
Rebase, lint, re-minify
Sep 16, 2016
6f6a150
Address PR comments
Sep 19, 2016
75131b5
transitions
Oct 3, 2016
76014c4
Light validation
Oct 4, 2016
59d6f47
eslint
Oct 4, 2016
da73797
Add tests for light properties
Oct 5, 2016
fc31ea0
Allow zoom functions for light properties
Oct 6, 2016
12e212d
Fix light diffing, remove map.light descriptions to reflect moving li…
Oct 7, 2016
9db077c
Use new enum format
Oct 7, 2016
c41f9e3
Rename light.direction to light.position; address other PR comments
Oct 7, 2016
0df913b
Clarify radial distance
Oct 7, 2016
c3a9fd3
:pray: to @1ec5
Oct 7, 2016
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
2 changes: 2 additions & 0 deletions docs/_generate/item.html
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
} else {
if (req['!']) {
return '<em>Disabled by</em> <var>' + req['!'] + '</var>.';
} else if (req['<=']) {
return '<em>Must be less than or equal to</em> <var>' + req['<='] + '</var>.';
} else {
var requiredValue = _.toPairs(req)[0][1];
var requiredDisplay = _.isArray(requiredValue)
Expand Down
10 changes: 9 additions & 1 deletion lib/diff.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,12 @@ var operations = {
/*
* { command: 'setTransition', args: [transition] }
*/
setTransition: 'setTransition'
setTransition: 'setTransition',

/*
* { command: 'setLighting', args: [lightProperties] }
*/
setLight: 'setLight'

};

Expand Down Expand Up @@ -292,6 +297,9 @@ function diffStyles(before, after) {
if (!isEqual(before.transition, after.transition)) {
commands.push({ command: operations.setTransition, args: [after.transition] });
}
if (!isEqual(before.light, after.light)) {
commands.push({ command: operations.setLight, args: [after.light] });
}
diffSources(before.sources, after.sources, commands);
diffLayers(before.layers, after.layers, commands);
} catch (e) {
Expand Down
53 changes: 53 additions & 0 deletions lib/validate/validate_light.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
'use strict';

var ValidationError = require('../error/validation_error');
var getType = require('../util/get_type');
var validate = require('./validate');
var validateTypes = {
enum: require('./validate_enum'),
color: require('./validate_color'),
array: require('./validate_array'),
number: require('./validate_number'),
function: require('./validate_function')
};

module.exports = function validateLight(options) {
var light = options.value;
var styleSpec = options.styleSpec;
var lightSpec = styleSpec.$root.light;
var style = options.style;

var errors = [];

var type;
for (var key in light) {
var transitionMatch = key.match(/^(.*)-transition$/);

if (transitionMatch && lightSpec[transitionMatch[1]] && lightSpec[transitionMatch[1]].transition) {
var baseKey = transitionMatch[1];

type = lightSpec[baseKey].type;
errors = errors.concat(validate({
key: key,
value: light[key],
valueSpec: styleSpec.transition,
style: style,
styleSpec: styleSpec
}));
} else if (lightSpec[key]) {
type = lightSpec[key].type;
if (lightSpec[key].function && getType(light[key]) === 'object') type = 'function';
errors = errors.concat(validateTypes[type]({
key: key,
value: light[key],
valueSpec: lightSpec[key],
style: style,
styleSpec: styleSpec
}));
} else {
errors = errors.concat([new ValidationError(key, light[key], 'unknown property "%s"', key)]);
}
}

return errors;
};
1 change: 1 addition & 0 deletions lib/validate_style.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ module.exports = function validateStyle(style, styleSpec) {
};

exports.source = validateStyleMin.source;
exports.light = validateStyleMin.light;
exports.layer = validateStyleMin.layer;
exports.filter = validateStyleMin.filter;
exports.paintProperty = validateStyleMin.paintProperty;
Expand Down
1 change: 1 addition & 0 deletions lib/validate_style.min.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ function validateStyleMin(style, styleSpec) {
}

validateStyleMin.source = wrapCleanErrors(require('./validate/validate_source'));
validateStyleMin.light = wrapCleanErrors(require('./validate/validate_light'));
validateStyleMin.layer = wrapCleanErrors(require('./validate/validate_layer'));
validateStyleMin.filter = wrapCleanErrors(require('./validate/validate_filter'));
validateStyleMin.paintProperty = wrapCleanErrors(require('./validate/validate_paint_property'));
Expand Down
76 changes: 75 additions & 1 deletion reference/v8.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,55 @@
"doc": "Default pitch, in degrees. Zero is perpendicular to the surface, for a look straight down at the map, while a greater value like 60 looks ahead towards the horizon. The style pitch will be used only if the map has not been positioned by other means (e.g. map options or user interaction).",
"example": 50
},
"light": {
"anchor": {
"type": "enum",
"default": "viewport",
"values": {
"map": {
"doc": "The position of the light source is aligned to the rotation of the map."
},
"viewport": {
"doc": "The position of the light source is aligned to the rotation of the viewport."
}
},
"transition": false,
"doc": "Whether extruded geometries are lit relative to the map or viewport.",
"example": "map"
},
"position": {
"type": "array",
Copy link
Contributor

@1ec5 1ec5 Sep 17, 2016

Choose a reason for hiding this comment

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

Specify "length": 3.

"default": [1.15, 210, 30],
"length": 3,
"value": "number",
"transition": true,
"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 the base of an object to its light, a indicates the position of the light relative to 0° (0° when `light.anchor` is set to `viewport` corresponds to the top of the viewport, or 0° when `light.anchor` is set to `map` corresponds to due north, and degrees proceed clockwise), and p indicates the height of the light (from 0°, directly above, to 180°, directly below).",
"example": [1.5, 90, 80]

Choose a reason for hiding this comment

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

Do you think our code would be more self-documenting if this were split into 3 separate properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"

Copy link
Contributor

@scothis scothis Sep 17, 2016

Choose a reason for hiding this comment

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

Using an [x, y, z] array to describe a vector is very common. I'm less familiar with polar coordinates.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

},
"color": {
"type": "color",
"default": "#ffffff",
"function": "interpolated",
"zoom-function": true,
"property-function": false,
"transition": true,
"doc": "Color tint for lighting extruded geometries."
},
"intensity": {
"type": "number",
"default": 0.5,
"minimum": 0,
"maximum": 1,
"function": "interpolated",
"zoom-function": true,
"property-function": false,
"transition": true,
"doc": "Intensity of lighting (on a scale from 0 to 1). Higher numbers will present as more extreme contrast."
}
},
"sources": {
"required": true,
"type": "sources",
Expand Down Expand Up @@ -1513,7 +1562,7 @@
"default": 1,
"minimum": 0,
"maximum": 1,
"doc": "The opacity of the entire fill layer. In contrast to the `fill-color`, this value will also affect the 1px stroke around the fill, if the stroke is used.",
"doc": "The opacity of the entire fill layer. In contrast to the `fill-color`, this value will also affect the 1px stroke around the fill, if the stroke is used. If a fill layer is extruded, opacity will be per-layer rather than per-feature (and will thus not support property functions).",
"transition": true,
"sdk-support": {
"basic functionality": {
Expand Down Expand Up @@ -1638,6 +1687,31 @@
"android": "2.0.1"
}
}
},
"fill-extrude-height": {
"type": "number",
"function": "interpolated",
"zoom-function": true,
"property-function": true,
"default": 0,
"minimum": 0,
"doc": "The height with which to extrude this fill layer.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Specify "units": "meters".

"transition": true
},
"fill-extrude-base": {
"type": "number",
"function": "interpolated",
"zoom-function": true,
"property-function": true,
"default": 0,
"minimum": 0,
"doc": "The height with which to extrude the base of this layer.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Specify "units": "meters".

"transition": true,
"requires": [
{
"<=": "fill-extrude-height"
Copy link
Contributor

Choose a reason for hiding this comment

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

This <= key to a requires object is new, right? So its introduction will require changes to documentation generation code and Studio.

}
]
}
},
"paint_line": {
Expand Down
2 changes: 1 addition & 1 deletion reference/v8.min.json

Large diffs are not rendered by default.

72 changes: 72 additions & 0 deletions test/diff.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,5 +153,77 @@ t('diff', function (t) {
{ command: 'setPitch', args: [1] }
], 'pitch change');

t.deepEqual(diffStyles({
light: {
anchor: 'map',
color: 'white',
position: [0, 1, 0],
intensity: 1
}
}, {
light: {
anchor: 'map',
color: 'white',
position: [0, 1, 0],
intensity: 1
}
}), [
], 'light no change');

t.deepEqual(diffStyles({
light: { anchor: 'map' }
}, {
light: { anchor: 'viewport' }
}), [
{ command: 'setLight', args: [{'anchor': 'viewport'}] }
], 'light anchor change');

t.deepEqual(diffStyles({
light: { color: 'white' }
}, {
light: { color: 'red' }
}), [
{ command: 'setLight', args: [{'color': 'red'}] }
], 'light color change');

t.deepEqual(diffStyles({
light: { position: [0, 1, 0] }
}, {
light: { position: [1, 0, 0] }
}), [
{ command: 'setLight', args: [{'position': [1, 0, 0]}] }
], 'light position change');

t.deepEqual(diffStyles({
light: { intensity: 1 }
}, {
light: { intensity: 10 }
}), [
{ command: 'setLight', args: [{'intensity': 10}] }
], 'light intensity change');

t.deepEqual(diffStyles({
light: {
anchor: 'map',
color: 'orange',
position: [2, 80, 30],
intensity: 1.0
}
}, {
light: {
anchor: 'map',
color: 'red',
position: [1, 40, 30],
intensity: 1.0
}
}), [
{ command: 'setLight', args: [{
anchor: 'map',
color: 'red',
position: [1, 40, 30],
intensity: 1.0
}] }
], 'multiple light properties change');

t.end();
});
16 changes: 16 additions & 0 deletions test/fixture/light.input.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"version": 8,
"sources": {
"vector": {
"type": "vector",
"url": "mapbox://mapbox.mapbox-streets-v5"
}
},
"light": {
"anchor": true,
"position": [1, 2],
"color": [255, 255, 255, 1],
"intensity": 1.5
},
"layers": []
}
18 changes: 18 additions & 0 deletions test/fixture/light.output.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
[
{
"message": "light.anchor: expected one of [map, viewport], true found",
"line": 10
},
{
"message": "light.position: array length 3 expected, length 2 found",
"line": 11
},
{
"message": "light.color: color expected, array found",
"line": 12
},
{
"message": "light.intensity: 1.5 is greater than the maximum value 1",
"line": 13
}
]
3 changes: 2 additions & 1 deletion test/fixture/root-properties.input.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@
"modified": "",
"name": "Test",
"owner": "peterqliu",
"id": "peterqliu.test"
"id": "peterqliu.test",
"light": true
}
4 changes: 4 additions & 0 deletions test/fixture/root-properties.output.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,9 @@
{
"message": "layers: array expected, object found",
"line": 5
},
{
"message": "light: object expected, boolean found",
"line": 11
}
]