Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Add line-gradient property #12575

Merged
merged 2 commits into from
Aug 23, 2018
Merged

[core] Add line-gradient property #12575

merged 2 commits into from
Aug 23, 2018

Conversation

pozdnyakov
Copy link
Contributor

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 pozdnyakov force-pushed the line_gradient branch 2 times, most recently from 63208a2 to 1f50d02 Compare August 8, 2018 12:18
@friedbunny friedbunny added feature GL JS parity For feature parity with Mapbox GL JS Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl labels Aug 8, 2018
@pozdnyakov pozdnyakov requested a review from kkaefer August 8, 2018 17:19
@pozdnyakov
Copy link
Contributor Author

@kkaefer please review, tagging @tmpsantos @brunoabinader

@pozdnyakov pozdnyakov force-pushed the line_gradient branch 2 times, most recently from 4573d8d to fb3060f Compare August 9, 2018 13:59
@pozdnyakov
Copy link
Contributor Author

This PR fixes #11718

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

In addition to the following, please update the iOS and macOS changelogs to note the addition of these new properties. Thanks!

#if TARGET_OS_IPHONE
/**
Defines a gradient with which to color a line feature. Can only be used with
GeoJSON sources that specify `"lineMetrics": true`.
Copy link
Contributor

@1ec5 1ec5 Aug 9, 2018

Choose a reason for hiding this comment

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

This documentation comment uses JSON notation, so we’ll need to override it in an entry in platform/darwin/scripts/style-spec-overrides-v8.json:

Defines a gradient with which to color a line feature. This property only has an effect on lines defined by an MGLShapeSource whose MGLShapeSource.calculatesLineDistanceMetrics property is set to YES.

Speaking of which, we’ll need to add that calculatesLineDistanceMetrics property to MGLShapeSource before developers will be able to use this property on iOS/macOS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Defines a gradient with which to color a line feature. Can only be used with
GeoJSON sources that specify `"lineMetrics": true`.

This property is only applied to the style if `lineDasharray` is set to `nil`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should say lineDashPattern. The codegen script isn’t aliasing lineDasharray for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pozdnyakov
Copy link
Contributor Author

@1ec5 Thanks for your comments! I've made a #12604 introducing a new property to MGLShapeSource, please take a look! After #12604 lands I'll include the updated style-spec-overrides-v8.json to this PR.

result += util::dist<double>(coordinates[i], coordinates[i + 1]);
}
return result;
}();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a self-invoking lambda?

Point<double> extrude = normal;
if (lineDistances)
distance = lineDistances->scaleToMaxLineDistance(distance);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add braces here even if there is just one statement.

@@ -392,14 +427,18 @@ void LineBucket::addGeometry(const GeometryCoordinates& coordinates, const Geome
}

void LineBucket::addCurrentVertex(const GeometryCoordinate& currentCoordinate,
double &distance,
double distance,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we no longer need the distance as a reference? We still seem to be modifying it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch! I missed that we need to set it to 0.0 under some conditions

Point<double> flippedExtrude = extrude * (lineTurnsLeft ? -1.0 : 1.0);
if (lineDistances)
distance = lineDistances->scaleToMaxLineDistance(distance);
Copy link
Contributor

Choose a reason for hiding this comment

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

Braces please :)

@@ -68,7 +68,8 @@ bool isConstant(const Expression& expression) {
}

return isFeatureConstant(expression) &&
isGlobalPropertyConstant(expression, std::array<std::string, 2>{{"zoom", "heatmap-density"}});
isGlobalPropertyConstant(expression, std::array<std::string, 2>{{"zoom", "heatmap-density"}}) &&
isGlobalPropertyConstant(expression, std::array<std::string, 2>{{"zoom", "line-progress"}});;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: double semicoli


void LineLayer::setLineGradient(ColorRampPropertyValue value) {
if (value == getLineGradient())
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Braces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a generated file, I could introduce braces generation to scripts/generate-style-code.js in a follow-up PR

@pozdnyakov pozdnyakov force-pushed the line_gradient branch 2 times, most recently from d304e75 to 19e5c4d Compare August 10, 2018 14:20
@pozdnyakov
Copy link
Contributor Author

@kkaefer Thanks for the review! The latest patch is modified accordingly.

@tobrun tobrun added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS labels Aug 13, 2018
@pozdnyakov pozdnyakov force-pushed the line_gradient branch 2 times, most recently from d7fefa8 to 7f5d5c8 Compare August 20, 2018 16:40
@pozdnyakov
Copy link
Contributor Author

@kkaefer could you take a look?

pozdnyakov and others added 2 commits August 23, 2018 17:34
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 pozdnyakov merged commit fc26f2a into master Aug 23, 2018
@pozdnyakov pozdnyakov deleted the line_gradient branch August 23, 2018 16:02
@1ec5 1ec5 added the needs changelog Indicates PR needs a changelog entry prior to merging. label Oct 1, 2018
@friedbunny friedbunny removed needs changelog Indicates PR needs a changelog entry prior to merging. labels Oct 31, 2018
@friedbunny
Copy link
Contributor

For future spelunkers: this was included in release-frappé (which was ios-v4.4.0 and android-v6.5.0).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl feature GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants