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

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

Show file tree
Hide file tree
Changes from 1 commit
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
10 changes: 4 additions & 6 deletions reference/v8.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,17 +49,17 @@
"default": "viewport",
"values": {
"map": {
"doc": "The direction of the light source is aligned to the rotation of the map."
"doc": "The position of the light source is aligned to the rotation of the map."
},
"viewport": {
"doc": "The direction of the light source is aligned to the rotation of the 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"
},
"direction": {
"position": {
"type": "array",
Copy link
Contributor

@1ec5 1ec5 Sep 17, 2016 •
edited
Loading

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,
Expand All @@ -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).",
Copy link
Contributor

Choose a reason for hiding this comment

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

“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.anchor is set to viewport

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.

"example": [1.5, 90, 80]
Copy link

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 •
edited
Loading

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": {
Expand Down Expand Up @@ -1695,7 +1695,6 @@
"property-function": true,
"default": 0,
"minimum": 0,
"units": "meters",
"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
},
Expand All @@ -1706,7 +1705,6 @@
"property-function": true,
"default": 0,
"minimum": 0,
"units": "meters",
"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": [
Expand Down
Loading