-
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
Update to Maps SDK for iOS v4.0.0 #1076
Conversation
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’m impressed that you got this far with the rudimentary state of NSExpression in the map SDK. 👍 There’s a lot more room for concision; I think this PR, as written, points to improvements we can make in our expression documentation and especially the expression migration guide we’re preparing.
/cc @jmkiley
MapboxNavigation.podspec
Outdated
@@ -44,7 +44,7 @@ Pod::Spec.new do |s| | |||
s.module_name = "MapboxNavigation" | |||
|
|||
s.dependency "MapboxCoreNavigation", "#{s.version.to_s}" | |||
s.dependency "Mapbox-iOS-SDK", "~> 3.6" | |||
s.dependency "Mapbox-iOS-SDK", "4.0.0-beta.1" |
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.
Remember to update the Cartfile and MapboxNavigation-Documentation.podspec before merging.
19: MGLStyleValue(rawValue: 22), | ||
22: MGLStyleValue(rawValue: 28) | ||
|
||
let routeLineWidthAtZoomLevels: [Int: NSExpression] = [ |
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.
No need to wrap these constant values in NSExpression. The dictionary can be [Int: Float]
or [Int: Int]
, which also means you don’t need to declare the type, either.
13: NSExpression(forConstantValue: 9), | ||
16: NSExpression(forConstantValue: 11), | ||
19: NSExpression(forConstantValue: 22), | ||
22: NSExpression(forConstantValue: 28) |
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.
@anandthakker, currently, it isn’t possible for a zoom expression ($zoomLevel
) to appear outside a curve or interpolation. But would it be feasible to relax this restriction, so that this curve could be expressed as a single multiplication expression involving $zoomLevel
?
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.
Technically, it would be possible to relax the restriction on zoom expressions to:
- If the expression is a zoom-only expression, no restrictions at all.
- If the expression is also feature-dependent, it must be in the form
F(zoom) * G(feature)
, whereF
is a step or interpolation expression usingzoom
as the input (and nowhere else), andG
is not zoom-dependent.
We've chosen so far to go with the existing, less permissive restriction because it's so much simpler to understand. I think the above, which is maximally permissive (barring significant changes to the data-driven styling implementation), is likely way too complicated. We could go for something in between, but it'll be tricky to decide just where to draw the line.
arrow.lineColor = MGLStyleValue(rawValue: .white) | ||
arrow.lineCap = NSExpression(forConstantValue: cap) | ||
arrow.lineJoin = NSExpression(forConstantValue: join) | ||
arrow.lineWidth = NSExpression(format: "FUNCTION($zoomLevel, 'mgl_interpolateWithCurveType:parameters:stops:', 'linear', nil, %@)", argumentArray: [routeLineWidthAtZoomLevels.multiplied(by: 1.5)]) |
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 calling multiplied(by:)
on the array, multiply the FUNCTION
call by 1.5 inside the format string.
arrow.lineColor = MGLStyleValue(rawValue: .white) | ||
arrow.lineCap = NSExpression(forConstantValue: cap) | ||
arrow.lineJoin = NSExpression(forConstantValue: join) | ||
arrow.lineWidth = NSExpression(format: "FUNCTION($zoomLevel, 'mgl_interpolateWithCurveType:parameters:stops:', 'linear', nil, %@)", argumentArray: [routeLineWidthAtZoomLevels.multiplied(by: 1.5)]) |
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.
Since you know in advance how many arguments this format string needs, use NSExpression(format:)
instead of NSExpression(format:argumentArray:)
. The first argument to NSExpression(format:)
takes a comma-separated list of arguments following the format string.
cameraStops: routeLineWidthAtZoomLevels.multiplied(by: 0.80), | ||
options: [.defaultValue : MGLConstantStyleValue<NSNumber>(rawValue: 1.5)]) | ||
arrowStroke.lineColor = MGLStyleValue(rawValue: .defaultArrowStroke) | ||
arrowStroke.lineCap = NSExpression(forConstantValue: cap) |
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.
NSExpression(forConstantValue: "cap")
or NSExpression(format: "'cap'")
is also allowed now, although what you’ve got is also perfectly fine if you want compile-time validation.
lineCasing.lineJoin = MGLStyleValue(rawValue: NSValue(mglLineJoin: .round)) | ||
lineCasing.lineOpacity = MGLStyleValue(rawValue: 0.9) | ||
lineCasing.lineColor = NSExpression(forConstantValue: routeAlternateColor) | ||
lineCasing.lineCap = NSExpression(forConstantValue: NSValue(mglLineCap: .round)) |
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 you can say something like MGLLineCap.round as NSValue
instead of calling NSValue(mglLineCap:)
. NSExpression(forConstantValue: "round")
and NSExpression(format: "'round'")
are also options.
symbol.textFontSize = MGLStyleValue(rawValue: 10) | ||
symbol.textHaloWidth = MGLStyleValue(rawValue: 0.25) | ||
symbol.textHaloColor = MGLStyleValue(rawValue: .black) | ||
symbol.text = NSExpression(forConstantValue: "{name}") |
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.
Pretty sure mapbox/mapbox-gl-native#10399 will prevent the legacy {token}
syntax from working inside an expression. Use a key path expression instead: NSExpression(forKeyPath: "name")
or NSExpression(format: "name")
.
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.
Use a key path expression instead: NSExpression(forKeyPath: "name")
or NSExpression(format: "name")
.
/ref #1076 (comment)
"heavy": NSExpression(forConstantValue: trafficHeavyColor), | ||
"severe": NSExpression(forConstantValue: trafficSevereColor) | ||
]) | ||
line.lineOpacity = NSExpression(format: "FUNCTION($currentLegAttribute, 'mgl_interpolateWithCurveType:parameters:stops:', 'linear', nil, %@)", [ |
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.
$currentLegAttribute
would be a reference to an expression variable declared with mgl_expressionWithContext:
. Use currentLegAttribute
, without the dollar sign, to refer to an attribute of the current feature.
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.
Actually, currentLegAttribute
is the name of this variable. You have two options: either insert a key path expression created from the variable:
NSExpression(format: "FUNCTION(%@, 'mgl_interpolateWithCurveType:parameters:stops:', 'linear', nil, %@)", NSExpression(forKeyPath: currentLegAttribute), …)
or inline the key path and delete currentLegAttribute
:
NSExpression(format: "FUNCTION(isCurrentLeg, 'mgl_interpolateWithCurveType:parameters:stops:', 'linear', nil, %@)", …)
line.lineOpacity = NSExpression(format: "FUNCTION($currentLegAttribute, 'mgl_interpolateWithCurveType:parameters:stops:', 'linear', nil, %@)", [ | ||
true: 1, | ||
false: 0 | ||
]) |
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.
As a Boolean attribute, isCurrentLeg
can have only two possible values, true
and false
, so a full stop dictionary is overkill. With an expression, you can express a condition directly:
NSExpression(format: "TERNARY(isCurrentLeg = true, 1, 0)")
or:
NSExpression(forConditional: NSPredicate(format: "isCurrentLeg = true"), true: 1, false: 0)
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.
Then again, the usual way to accomplish this effect in a style is to let each line know which leg index it represents and add two layers to the style, one for the active leg and another for the inactive legs.
Both layers would have constant opacities. The filters (predicates) for the two layers would be updated every time the leg index changes:
activeLegLayer.predicate = NSPredicate(format: "legIndex = %@", routeProgess.legIndex)
inactiveLegLayer.predicate = NSPredicate(format: "legIndex != %@", routeProgress.legIndex)
This is currently blocked upstream by mapbox/mapbox-gl-native#11277 & mapbox/mapbox-gl-native#10849 |
line.lineJoin = MGLStyleValue(rawValue: NSValue(mglLineJoin: .round)) | ||
line.lineWidth = NSExpression(format: "FUNCTION($zoomLevel, 'mgl_interpolateWithCurveType:parameters:stops:', 'linear', nil, %@)", argumentArray: [MBRouteLineWidthByZoomLevel]) | ||
line.lineOpacity = NSExpression(forConditional: NSPredicate(format: "\(MBCurrentLegAttribute) == true"), trueExpression: NSExpression(forConstantValue: 1), falseExpression: NSExpression(forConstantValue: 0.85)) | ||
line.lineColor = NSExpression(format: "%@.severe", [ |
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.
@anandthakker @1ec5 this is failing with
Assertion failed: (literal), function operator(), file /Users/distiller/project/include/mbgl/style/conversion/data_driven_property_value.hpp, line 50.
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.
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.
This expression assumes all traffic is severe. The format should be:
NSExpression(format: "FUNCTION(%@, 'valueForKeyPath:', congestion)")
Alternatively, use an MGL_MATCH
expression:
NSExpression(format: "MGL_MATCH(congestion, 'unknown', %@, 'low', %@, 'moderate', %@, …)")
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 can get the JSON expression representation via NSExpression.mgl_jsonExpressionObject()
. It might be worth printing that out as part of the exception handler. Currently, the exception handler is in code that only knows about mbgl expressions, not NSExpression.
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.
This is an expression parsing bug -- reopened here: mapbox/mapbox-gl-native#10849
19: MGLStyleValue(rawValue: 22), | ||
22: MGLStyleValue(rawValue: 28) | ||
public let MBRouteLineWidthByZoomLevel: [Int: NSExpression] = [ | ||
10: NSExpression(forConstantValue: 8), |
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.
Pretty sure these don’t need to be NSExpressions; they can be bare Float
s or NSNumber
s.
newCameraStop[stop.key] = MGLStyleValue<NSNumber>(rawValue: NSNumber(value:newValue)) | ||
var newCameraStop: [Int: NSExpression] = [:] | ||
for stop in self { | ||
let currentValue = stop.value.constantValue as! Double |
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.
Note that this will crash if the dictionary contains any expression that isn’t a constant value. Since this is a public extension on Dictionary
, we have to assume the developer is going to try to use it wherever the compiler lets them.
I continue to believe we should make this method private or remove it outright. A few explicit multiplications won’t greatly inconvenience the developer.
|
||
arrow.lineCap = NSExpression(forConstantValue: "cap") | ||
arrow.lineJoin = NSExpression(forConstantValue: "join") | ||
arrow.lineWidth = NSExpression(format: "FUNCTION($zoomLevel, 'mgl_interpolateWithCurveType:parameters:stops:', 'linear', nil, %@)", argumentArray: [MBRouteLineWidthByZoomLevel.multiplied(by: 0.70)]) |
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.
The syntax has now changed to mgl_interpolate:withCurveType:parameters:stops:($zoomLevel, 'linear', nil, %@)
. (×7)
arrowSymbolLayer.iconAllowsOverlap = MGLStyleValue(rawValue: true) | ||
arrowSymbolLayer.iconImageName = NSExpression(forConstantValue: "triangle-tip-navigation") | ||
arrowSymbolLayer.iconColor = NSExpression(forConstantValue: UIColor.white) | ||
arrowSymbolLayer.iconRotationAlignment = NSExpression(forConstantValue: NSValue(mglIconRotationAlignment: .map)) |
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.
Simplify this to NSExpression(forConstantValue: "map")
for consistency. (×2)
arrowSymbolLayerCasing.iconAllowsOverlap = MGLStyleValue(rawValue: true) | ||
arrowSymbolLayerCasing.iconImageName = NSExpression(forConstantValue: "triangle-tip-navigation") | ||
arrowSymbolLayerCasing.iconColor = NSExpression(forConstantValue: UIColor.defaultArrowStroke) | ||
arrowSymbolLayerCasing.iconRotationAlignment = NSExpression(forConstantValue: NSValue(mglIconRotationAlignment: .map)) |
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.
Reduce repetition by saying, for example:
arrowSymbolLayerCasing.iconRotationAlignment = arrowSymbolLayer.iconRotationAlignment
symbol.textFontSize = MGLStyleValue(rawValue: 10) | ||
symbol.textHaloWidth = MGLStyleValue(rawValue: 0.25) | ||
symbol.textHaloColor = MGLStyleValue(rawValue: .black) | ||
symbol.text = NSExpression(forConstantValue: "{name}") |
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.
Use a key path expression instead: NSExpression(forKeyPath: "name")
or NSExpression(format: "name")
.
/ref #1076 (comment)
line.lineJoin = MGLStyleValue(rawValue: NSValue(mglLineJoin: .round)) | ||
line.lineWidth = NSExpression(format: "FUNCTION($zoomLevel, 'mgl_interpolateWithCurveType:parameters:stops:', 'linear', nil, %@)", argumentArray: [MBRouteLineWidthByZoomLevel]) | ||
line.lineOpacity = NSExpression(forConditional: NSPredicate(format: "\(MBCurrentLegAttribute) == true"), trueExpression: NSExpression(forConstantValue: 1), falseExpression: NSExpression(forConstantValue: 0.85)) | ||
line.lineColor = NSExpression(format: "%@.severe", [ |
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.
This expression assumes all traffic is severe. The format should be:
NSExpression(format: "FUNCTION(%@, 'valueForKeyPath:', congestion)")
Alternatively, use an MGL_MATCH
expression:
NSExpression(format: "MGL_MATCH(congestion, 'unknown', %@, 'low', %@, 'moderate', %@, …)")
} | ||
} | ||
|
||
// TODO: |
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.
Is this still to do?
@@ -672,7 +672,7 @@ open class NavigationMapView: MGLMapView, UIGestureRecognizerDelegate { | |||
arrowSymbolLayerCasing.minimumZoomLevel = minimumZoomLevel | |||
arrowSymbolLayerCasing.iconImageName = NSExpression(forConstantValue: "triangle-tip-navigation") | |||
arrowSymbolLayerCasing.iconColor = NSExpression(forConstantValue: UIColor.defaultArrowStroke) | |||
arrowSymbolLayerCasing.iconRotationAlignment = NSExpression(forConstantValue: NSValue(mglIconRotationAlignment: .map)) | |||
arrowSymbolLayerCasing.iconRotationAlignment = arrowSymbolLayer.iconRotationAlignment |
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.
The idea behind #1076 (comment) was that we’d do the same thing for the dozens of other properties in this file.
"severe": trafficSevereColor | ||
]) | ||
|
||
line.lineColor = NSExpression(format: "MGL_MATCH(congestion, 'unknown', %@, 'low' , %@, 'moderate', %@, 'heavy', %@, 'severe', %@, %@)", trafficUnknownColor, trafficLowColor, trafficModerateColor, trafficHeavyColor, trafficSevereColor, UIColor.purple) |
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.
Remove the case for unknown
and make trafficUnknownColor
the fallback.
] | ||
let expression = NSExpression.mgl_expression(withJSONObject: jsonExpression) | ||
let localizedExpression = expression.localized(into: nil) | ||
XCTAssertEqual(localizedExpression.mgl_jsonExpressionObject as! NSArray, localizedJSONExpression as NSArray) |
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.
This test will fail until mapbox/mapbox-gl-native#11615 lands in a build of the map SDK.
This comment has been minimized.
This comment has been minimized.
Fixed arrow width issue with c2b276a |
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.
This PR is ready except for releasing the map SDK first (minor details!) and also updating the changelog.
MapboxNavigation/NSExpression.swift
Outdated
} | ||
|
||
extension NSExpression { | ||
func localized(into locale: Locale?, replacingTokens replacesTokens: Bool) -> NSExpression { |
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.
Nearly all of this functionality has been upstreamed to the map SDK in mapbox/mapbox-gl-native#11651. So the implementation here can be replaced with a simple call to the similarly named method provided by the map SDK.
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.
Done in 7bbfe0deb187c350638ff579788ee843bea50474.
I’ve been pretty involved with this PR, so I’d like to get another pair of 👀 from either @vincethecoder or @JThramer. Thanks! |
bb8c034
to
e0ee67a
Compare
Tests are failing due to #1327. |
line.lineJoin = MGLStyleValue(rawValue: NSValue(mglLineJoin: .round)) | ||
line.lineColor = NSExpression(forConstantValue: UIColor(red:0.00, green:0.45, blue:0.74, alpha:0.9)) | ||
line.lineWidth = NSExpression(forConstantValue: 5) | ||
line.lineCap = NSExpression(forConstantValue: NSValue(mglLineCap: .round)) |
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.
nit: NSExpression(forConstantValue: "round")
seems to be the recommended way according to the documentation and we use throughout the migration except for here.
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.
Fixed in 0294f1209b08f3c7329bd0eecb5752aea359b1a7.
Removed much of the expression localization code, now that it has been upstreamed into the map SDK.
Close: #1057
This PR updates to the Mapbox Maps for iOS dependency to v4.0.0.
todo:
Blocked by mapbox/mapbox-gl-native#11615.
/cc @mapbox/navigation-ios