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

Allow styling each route differently #2719

Merged
merged 5 commits into from
Nov 24, 2020
Merged

Conversation

MaximAlien
Copy link
Contributor

Initial implementation of route line styling functionality. Example:

output

@MaximAlien MaximAlien added UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. iOS labels Oct 30, 2020
@MaximAlien MaximAlien added this to the v1.2.0 milestone Oct 30, 2020
@MaximAlien MaximAlien requested a review from a team October 30, 2020 22:52
@MaximAlien MaximAlien self-assigned this Oct 30, 2020
@MaximAlien MaximAlien marked this pull request as draft October 30, 2020 22:53
@MaximAlien MaximAlien force-pushed the maxim/651-route-line-styling branch 3 times, most recently from d7570b5 to 24b6799 Compare November 14, 2020 01:07
@MaximAlien MaximAlien marked this pull request as ready for review November 17, 2020 17:14
Comment on lines +486 to +507
let routeShape = navigationMapViewDelegate?.navigationMapView(self, shapeFor: [route]) ??
shape(for: route, legIndex: legIndex, isAlternateRoute: false)

let routeSource = addRouteSource(style, identifier: routeSourceIdentifier, shape: routeShape)

let mainRouteLayer = addMainRouteLayer(style,
source: routeSource,
identifier: routeIdentifier,
lineGradient: routeLineGradient(route, fractionTraveled: 0.0))

let mainRouteCasingShape = navigationMapViewDelegate?.navigationMapView(self, simplifiedShapeFor: route) ??
shape(forCasingOf: route, legIndex: legIndex)

let routeCasingSource = addRouteSource(style, identifier: routeCasingSourceIdentifier, shape: mainRouteCasingShape)

parentLayer = addMainRouteCasingLayer(style,
source: routeCasingSource,
identifier: routeCasingIdentifier,
lineGradient: routeCasingGradient(0.0),
below: mainRouteLayer)

continue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is the most important change. As documentation was slightly changed in #2623 to address that NavigationMapViewDelegate.navigationMapView(_:shapeFor:) and NavigationMapViewDelegate.navigationMapView(_:simplifiedShapeFor:) are meant to be used only for main route in comment:

Resulting `MGLShape` will then be styled using `NavigationMapView.navigationMapView(_: mainRouteStyleLayerWithIdentifier: source:)` provided style or a default congestion style if above delegate method was not implemented. In latter case, consider modifing your custom `MGLShape` `attributes` to have 'isAlternateRoute' key set to 'false'. Otherwise style predicate condition will filter out the shape.

I think it has become more misleading, as NavigationMapViewDelegate.navigationMapView(_:shapeFor:) allows to change shape not only for main route, but for all routes.

After adding the ability to provide route style for every route in this PR NavigationMapViewDelegate.navigationMapView(_:shapeFor:) and NavigationMapViewDelegate.navigationMapView(_:simplifiedShapeFor:) will be called only for main route, this means there will be no ability to provide custom shape for alternative routes and their casings.

Because of this I think NavigationMapViewDelegate.navigationMapView(_:shapeFor:) can be updated to be similar to NavigationMapViewDelegate.navigationMapView(_:simplifiedShapeFor:) and provide Route instead of [Route], but it'd break public API.

@1ec5, @Udumft please let me know if that sounds reasonable and any changes are needed.

Example app was also updated to showcase the ability to provide custom layers and shapes in mapbox/mapbox-navigation-ios-examples#83.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because of this I think NavigationMapViewDelegate.navigationMapView(_:shapeFor:) can be updated to be similar to NavigationMapViewDelegate.navigationMapView(_:simplifiedShapeFor:) and provide Route instead of [Route], but it'd break public API.

We can add a new delegate method and deprecate the old one, as long as the old one continues to function reasonably appropriately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To play it safe I'm inclined to leave current implementation without adding new/deprecating old APIs: end user will be able to change shape only for main route instead of all routes (main + alternative). This will match current docs as well.

@@ -469,62 +468,101 @@ open class NavigationMapView: MGLMapView, UIGestureRecognizerDelegate {
*/
public func show(_ routes: [Route], legIndex: Int = 0) {
Copy link
Contributor Author

@MaximAlien MaximAlien Nov 20, 2020

Choose a reason for hiding this comment

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

@1ec5, do you remember how legIndex can be used here? As I can see after adding vanishing route line functionality it's no longer used when creating shape for the route, and the only place I can see where it was used before was to control public let MBCurrentLegAttribute = "isCurrentLeg" property inMGLPolylineFeature.

After running few tests I don't see that setting isCurrentLeg attribute to true or false affects anything though (I think idea was to use it in route line styling).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is functionality that broke when implementing #1307. Any leg other than the current leg is supposed to be dimmed out: #2678. That no longer matters as much for already-passed legs, but it does matter for legs ahead of the current leg.

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 clarifying this. In this case probably good idea would be to tackle it in #2733, fix for it might resolve #2678 as well.

@1ec5 1ec5 changed the title Implement route line styling functionality. Allow styling each route differently Nov 20, 2020
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.

Remember to add a changelog entry. Thanks!

@MaximAlien MaximAlien merged commit daa1143 into main Nov 24, 2020
@MaximAlien MaximAlien deleted the maxim/651-route-line-styling branch November 24, 2020 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI Work related to visual components, Android Auto, Camera, 3D, voice, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants