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

Add line-gradient property #6303

Merged
merged 15 commits into from
Apr 9, 2018
Merged

Add line-gradient property #6303

merged 15 commits into from
Apr 9, 2018

Conversation

lbud
Copy link
Contributor

@lbud lbud commented Mar 8, 2018

This PR adds support for gradient lines, with the following limitations:

  • line-gradient can only be used for lines with GeoJSON sources
    • the GeoJSON source used for these lines must specify the "lineMetrics": true option
  • line-gradient cannot be used in conjunction with line-dasharray or line-pattern
  • like heatmap-color, line-gradient must be specified using an expression with a special line-progress property: for example,
"line-gradient": [
    "interpolate",
    ["linear"],
    ["line-progress"],
    0, "rgba(0, 0, 255, 0.5)",
    0.5, "red",
    1, "#ffff00"
]

Overview of changes:

  • Adds a lineMetrics option to GeoJSON sources, which is passed through to geojson-vt in order to track distance metrics for linestring features
  • Adds a line-gradient line paint property, which specifies a gradient and is used in the same way heatmap-color is used to write a 256x1px texture containing the gradient
    • Changes HeatmapColorProperty to ColorRampProperty to be used for both, and abstracts color ramp image creation into a utility module for use in both
  • LineBucket:
    • checks for the presence of mapbox_clip_start and mapbox_clip_end properties, which are special properties added by geojson-vt when lineMetrics=true, and if so
    • precalculates the total distance of the tiled feature in tile units, and
    • scales the distance attribute of each vertex to [0, 215) relative to the length of the entire original line geometry
  • Adds new line_gradient shaders which sample from the line gradient texture based on the distance varying.

Refs #4095.

Benchmarks:

http://bl.ocks.org/lbud/raw/e96f30170bfc39a6e3f1ddd2c0cc8fa1/ — the only benchmark I'd expect to potentially be affected by this is LayerLine, which in the linked benchmarks looks like it improves but when repeatedly running benchmarks actually stays just about equal. I also added a temporary LayerLineGradient benchmark (bottom of page) and it is, expectedly, just a little bit slower than the plain LayerLine, which doe not render to or sample from a texture.

Launch Checklist

@lbud lbud force-pushed the gradient-lines branch 3 times, most recently from 356f32f to 121f74a Compare March 8, 2018 22:47
@@ -44,8 +44,8 @@ global.propertyType = function (property) {
return `DataDrivenProperty<${flowType(property)}>`;
} else if (/-pattern$/.test(property.name) || property.name === 'line-dasharray') {
return `CrossFadedProperty<${flowType(property)}>`;
} else if (property.name === 'heatmap-color') {
return `HeatmapColorProperty`;
} else if (property.name === 'heatmap-color' || property.name === 'line-gradient') {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's great that we have a second property with the same semantics as heatmap-color -- that increases the likelihood that heatmap-color is not some odd exception (and as we know, the odd exceptions are disproportionately expensive to maintain).

Since we now have two similarly-behaving properties, it's worth finding a generic way to capture that the style spec and replace the property name comparisons here with the use of a semantic property like isDataDriven uses. Such a property would probably be useful for Studio as well. As a schema design question, it intersects with #4194 -- although in light of this new information, I'm not sure my proposal there is still the best choice. @lbud do you want to think about this and come up with a proposal?

@lbud
Copy link
Contributor Author

lbud commented Mar 30, 2018

It's great that we have a second property with the same semantics as heatmap-color -- that increases the likelihood that heatmap-color is not some odd exception (and as we know, the odd exceptions are disproportionately expensive to maintain).

Since we now have two similarly-behaving properties, it's worth finding a generic way to capture that the style spec and replace the property name comparisons here with the use of a semantic property like isDataDriven uses. Such a property would probably be useful for Studio as well. As a schema design question, it intersects with #4194 -- although in light of this new information, I'm not sure my proposal there is still the best choice. @lbud do you want to think about this and come up with a proposal?

I'm working on this (#6389) but ended up splitting it off this branch as it complicates things quite a bit and conflates two different issues. I'm thinking maybe we should merge this as is (not as part of the 0.45 release) and follow it up immediately with a PR accomplishing #6389 (comment). Does that sound alright?

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

LGTM. Can you post benchmarks results?

@lbud lbud changed the base branch from master to release-0.45 April 6, 2018 22:27
jfirebaugh and others added 14 commits April 6, 2018 15:56
Instead of indexing all features for querying, index only those that are
actually within the tile boundaries. This prevents duplicate results
from being returned for point features. This is only a partial fix for
the duplicate results problem (lines and polygons can still return
duplicate results).
…e of consistency with vector tiles), necessitating splitting MultiLineString features into individual LineStrings so they can hold their own distance attributes

* Add render test
* Add unit tests for color ramp rendering
…s), clean up style-spec ref, add validation to ensure gradients are only used with geojson sources
@lbud lbud merged commit 759eee1 into release-0.45 Apr 9, 2018
@lbud lbud deleted the gradient-lines branch April 9, 2018 17:45
pozdnyakov added a commit to mapbox/mapbox-gl-native that referenced this pull request Aug 9, 2018
Porting of mapbox/mapbox-gl-js#6303
See the link above for the description of the feature and
its limitations).

Based on patch from @lbud (Lauren Budorick).
pozdnyakov added a commit to mapbox/mapbox-gl-native that referenced this pull request Aug 10, 2018
Porting of mapbox/mapbox-gl-js#6303
See the link above for the description of the feature and
its limitations).

Based on patch from @lbud (Lauren Budorick).
@danpe
Copy link

danpe commented Aug 15, 2018

👍

pozdnyakov added a commit to mapbox/mapbox-gl-native that referenced this pull request Aug 20, 2018
Porting of mapbox/mapbox-gl-js#6303
See the link above for the description of the feature and
its limitations).

Based on patch from @lbud (Lauren Budorick).
pozdnyakov added a commit to mapbox/mapbox-gl-native that referenced this pull request Aug 23, 2018
Porting of mapbox/mapbox-gl-js#6303
See the link above for the description of the feature and
its limitations).

Based on patch from @lbud (Lauren Budorick).
pozdnyakov added a commit to mapbox/mapbox-gl-native that referenced this pull request Aug 23, 2018
Porting of mapbox/mapbox-gl-js#6303
See the link above for the description of the feature and
its limitations).

Based on patch from @lbud (Lauren Budorick).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants