-
Notifications
You must be signed in to change notification settings - Fork 313
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
Insert route below text on reroute #91
Conversation
@@ -9,8 +9,8 @@ let routeLayerCasingIdentifier = "routeLayerCasing" | |||
|
|||
extension MGLMapView { | |||
|
|||
public func annotate(_ routes: [Route], clearMap: Bool) { | |||
guard let route = routes.first, var coordinates = route.coordinates else { | |||
public func annotate(_ route: Route) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method originally took an array in anticipation of alternative route support, but the next line brushed that idea away anyways. 😄
@@ -33,6 +33,7 @@ class RouteMapViewController: UIViewController, PulleyPrimaryContentControllerDe | |||
let webImageManager = SDWebImageManager.shared() | |||
var shieldAPIDataTask: URLSessionDataTask? | |||
var shieldImageDownloadToken: SDWebImageDownloadToken? | |||
var arrowCurrentStep: RouteStep? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(sigh) If we were using annotations instead of runtime styling, we could just make RouteStep conform to MGLAnnotation and ask the map which step is being annotated…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using runtime styling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapView.addArrow(routeProgress) | ||
if step != arrowCurrentStep { | ||
mapView.removeArrow() | ||
mapView.addArrow(routeProgress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of tearing down all the style layers and sources then rebuilding them from scratch, we should simply update the source’s shape
property to reflect the current step. The check above becomes unnecessary, because mbgl will automatically redraw the style layers without any flashing.
Updated with feedback |
MapboxNavigationUI/MGLMapView.swift
Outdated
style?.addSource(arrowSourceStroke) | ||
style?.addSource(arrowSource) | ||
if let source = style.source(withIdentifier: arrowSourceIdentifier) { | ||
let s = source as! MGLShapeSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combine these two lines:
if let source = style.source(withIdentifier: arrowSourceIdentifier) as? MGLShapeSource
MapboxNavigationUI/MGLMapView.swift
Outdated
s.shape = MGLShapeCollection(shapes: maneuverArrowPolylines) | ||
} else { | ||
style.addSource(arrowSource) | ||
style.addLayer(arrow) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move all the code that sets up arrowSource
and arrow
into this else
block. That way it’s clear that, on the second time around, the only thing that happens is the existing source’s shape getting modified.
MapboxNavigationUI/MGLMapView.swift
Outdated
s.shape = MGLShapeCollection(shapes: maneuverArrowStrokePolylines) | ||
} else { | ||
style.addSource(arrowSourceStroke) | ||
style.insertLayer(arrowStroke, below: arrow) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, move all the code that sets up arrowSourceStroke
and arrowStroke
into this else
block.
@@ -122,15 +123,14 @@ class RouteMapViewController: UIViewController, PulleyPrimaryContentControllerDe | |||
|
|||
func notifyDidReroute(route: Route) { | |||
routePageViewController.notifyDidReRoute() | |||
mapView.annotate([route], clearMap: true) | |||
mapView.addArrow(routeController.routeProgress) | |||
mapView.annotate(route) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s fix annotate(_:)
the same way you’ve fixed addArrow(_:)
: instead of blowing away the source and layers and adding them back in, just change the source’s shape. That’ll fix any flashing that occurs during rerouting and also reduce the amount of work MGLMapView has to do every time the route changes.
00ac3ec
to
49d7a82
Compare
49d7a82
to
0f688b0
Compare
let arrowSourceStroke = MGLShapeSource(identifier: arrowSourceStrokeIdentifier, shape: arrowStrokeShape, options: nil) | ||
let arrowStroke = MGLLineStyleLayer(identifier: arrowSourceIdentifier, source: arrowSourceStroke) | ||
let arrowSource = MGLShapeSource(identifier: arrowSourceIdentifier, shape: arrowShape, options: nil) | ||
let arrow = MGLLineStyleLayer(identifier: arrowLayerIdentifier, source: arrowSource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arrow and arrowStroke should ideally be declared down in the else block instead of up here, since they aren't used in the if let block.
Structured road names, codes, destinations
annotate
arrow
instead.closes: #90 #89
/cc @1ec5