-
Notifications
You must be signed in to change notification settings - Fork 311
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
Add more turn lane configurations #2882
Conversation
This comment has been minimized.
This comment has been minimized.
f8e6fde
to
e58e4b3
Compare
I have verified that the dual-use icons are now composited correctly and that the "slight" turn variants are shown correctly. I haven't been able to verify all the combinations in the simulator, but I believe we can merge this and deal with any specific issues as they pop up. |
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.
Passes my test cases and the code looks good. Hopefully we can simplify this soon using additional context from the Directions API.
Exported a new version of LanesStyleKit from PaintCode.
Added cases to LaneView for drawing the new sharp turn, dual use, and triple use configurations.
e58e4b3
to
ebb3417
Compare
LaneView now supports the additional turn lane icon assets introduced in mapbox/navigation-ui-resources#21, including the combination U-turn–left turn that’s so common in the iOS simulator’s home of Cupertino:
As seen in the screenshot, some of the wider U-turn-laden icons get cropped even though they don’t bleed over the edge of the canvas in PaintCode. It’s probably a layout issue in LaneView or LanesView.(Ignore the fact that the number 2 lane is highlighted as a valid lane; that seems to be a bug in OSRM that isn’t present in a Valhalla-based Directions API endpoint. I’m guessing OSRM isn’t recognizing the divided intersection, so it thinks there are two very closely spaced left turns, the first of which can use either left turn lane.)Unfortunately, I was unable to meaningfully update LaneViewTests, because it’s part of MapboxNavigationTests, which don’t run as part of SPM builds due to #2791 and don’t run as part of Carthage builds due to a lack of support for Carthage in MapboxNavigation. All I was able to do there was to incorporate some renames that took place internally in LaneView, but there are no test cases to exercise the new assets.
More to come:
LaneIndication.ranked(favoring:)
LaneConfiguration
that reflect the expanded repertoire of assetsLaneView.draw(_:)
for calling the new LanesStyleKit methodsLaneConfiguration(rankedIndications:drivingSide:)
Fixes #576.
/cc @mapbox/navigation-ios @abhishek1508 @avi-c