Building extrusion highlights #2535
Conversation
|
Do we want to squash the commit history? |
|
Here are some results which contain changes made by Dave and arrival tweaks: |
| return nil | ||
| } | ||
|
|
||
| private func highlightBuildings(_ buildings: [(identifier: UInt64, highlightColor: UIColor)]?, in3D: Bool, extrudeAll: Bool) { |
There was a problem hiding this comment.
Since extrudeAll feature is not really supported I propose to remove it completely. If needed we can add it in future, but for now it'll simplify current logic.
There was a problem hiding this comment.
I'd like to keep this capability in for further design and UX experimentation
There was a problem hiding this comment.
Does the Android implementation have the ability to highlight multiple buildings in different colors at the same time? If not, we should ticket that out as tail work in the mapbox/mapbox-navigation-android repository.
There was a problem hiding this comment.
I think @d-prukop can provide more info on this one.
There was a problem hiding this comment.
This functionality isn’t exposed in the public API, so we’re maintaining it in the codebase for no obvious benefit. In general, we should remove edge cases like these that applications can’t possibly trigger, so that they don’t bitrot and also don’t contribute to low code coverage.
|
Overall this looks good to me. We expose public
They can be taken care in scope of other PRs. PR which adds showcase sample can be found here. I'll also add it to documentation as well. @1ec5, please feel free to take a look and let us know whether you have any comments. Many thanks to @d-prukop for hard work on this PR. |
MaximAlien
force-pushed
the
dp-building-extrusions
branch
from
a3b8066
to
61383bc
Compare
…ith showAllBuildings since it's the only method inside of it.
…ldings separately.
… Add day/night styling for highlighted building.
…ing the map also unhighlights the building
…l destination. Re-center puck view and zoom-in in case of arrival.
…uilding extrusion.
| let fillExtrusionHeightStops = [0: NSExpression(forConstantValue: 0), | ||
| 13: NSExpression(forConstantValue: 0), | ||
| 13.25: NSExpression(format: highlightedBuildingHeightExpression)] |
There was a problem hiding this comment.
I think the purpose of these stops is to make the buildings appear to grow out of the ground when they first appear. But if somehow the camera stays put at z13.1, the buildings will stay short. Can we accomplish the same effect without the edge case by setting fillExtrusionHeight to TERNARY($zoomLevel > 13.25, %@, 0) and relying on the duration in fillExtrusionHeightTransition to make the buildings’ appearance less sudden?
| @@ -262,6 +266,8 @@ open class NavigationMapView: MGLMapView, UIGestureRecognizerDelegate { | |||
| addGestureRecognizer(mapTapGesture) | |||
|
|
|||
| installUserCourseView() | |||
|
|
|||
| styleURL = MGLStyle.navigationDayStyleURL | |||
There was a problem hiding this comment.
This overrides the style URL explicitly passed into init(frame:styleURL:) and possibly also init(coder:). I think we should move it into init(frame:) and find a way to only conditionally set it in init(coder:). This might be why we hadn’t customized NavigationMapView’s default style URL up to now.
There was a problem hiding this comment.
Good catch. There is one really strange issue with new approach though. When I add let styleURL = decoder.decodeObject(of: [URL.self], forKey: "styleURL") as? URL anywhere in the code in NavigationMapView Xcode gets Segmentation fault: 11 error during build. I think it'd be better to create separate ticket to handle this, because of mentioned issue and to prevent any side effects.
There was a problem hiding this comment.
URL is a pure Swift type, so it’s incompatible with NSCoder.decodeObject(of:forKey:). But it should work with TopLevelDecoder.decode(_:from:).
There was a problem hiding this comment.
Interesting finding, on the other hand seems that TopLevelDecoder.decode(_:from:) is still part of Combine. In any case I'll consider this approach as well.
There was a problem hiding this comment.
Oops, I meant KeyedDecodingContainer.decode(_:). We need to create a local decoding container in order to decode that key.
…pe` to control building highlighting.
MaximAlien
added
feature
Added the ability to highlight buildings in the NavigationMapView using a geographic coordinate.