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

Add intersection signals along current route. #4185

Merged
merged 10 commits into from
Oct 27, 2022
Merged

Conversation

ShanMa1991
Copy link
Contributor

@ShanMa1991 ShanMa1991 commented Oct 4, 2022

Description

This Pr is to fix #3843 by annotating intersection signals along the current route.

Implementation

  • The intersection signals include railroad crossing, yield sign, stop sign and traffic signal.
  • Added NavigationViewController.annotatesIntersectionsAlongRoute and CarPlayNavigationViewController.annotatesIntersectionsAlongRoute to annotate intersections on the current route step during active navigation.
  • When style change in NavigationViewController and CarPlayNavigationViewController, the signal icons from bundle resource will be added to the MapView style for intersection signals in current style type. And then update the signal layers.
  • When RouteProgress updated or reroute event happened, intersections on the current step and the first intersection on the upcoming step will be annotated on map. When user passed an intersection, the intersection signals will be removed from the source feature and disappear.

Screenshots or Gifs

showIntersections

@ShanMa1991 ShanMa1991 added the UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. label Oct 4, 2022
@ShanMa1991 ShanMa1991 added this to the v2.9 milestone Oct 4, 2022
@ShanMa1991 ShanMa1991 self-assigned this Oct 4, 2022
@ShanMa1991 ShanMa1991 force-pushed the shan/traffic-3843 branch 2 times, most recently from 01c8afe to b16f29b Compare October 5, 2022 19:03
@ShanMa1991 ShanMa1991 marked this pull request as ready for review October 6, 2022 19:04
@ShanMa1991 ShanMa1991 requested a review from a team October 6, 2022 19:04
Sources/MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
Comment on lines 404 to 407
if style.image(withId: ImageIdentifier.trafficSignalDay) == nil,
let trafficSignlaDay = Bundle.mapboxNavigation.image(named: "TrafficSignalDay") {
try style.addImage(trafficSignlaDay, id: ImageIdentifier.trafficSignalDay)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be awesome if the current Style subclass could define these images right alongside Style.mapStyle.

Example/ViewController.swift Outdated Show resolved Hide resolved
Comment on lines +464 to +474
let layerPosition = layerPosition(for: layerIdentifier)
try style.addPersistentLayer(shapeLayer, layerPosition: layerPosition)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the intersection annotation layer is on top of the maneuver arrow layer. That makes logical sense and maintains the illusion of these traffic lights and signs standing upright. However, the maneuver arrow is an important element to be obscuring with an icon.

Will the maneuver arrow be prominent enough to overcome this overlap? Or should be consider a visual mitigation, such as offsetting the traffic light at a maneuver point?

Copy link
Contributor Author

@ShanMa1991 ShanMa1991 Oct 13, 2022

Choose a reason for hiding this comment

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

I think we could solve this issue by having a smaller icon or a longer maneuver arrow, from a UX design perspective. Because it's easy to understand by having the traffic signal icons place above the route line, which means that the user will drive through them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, Google Maps takes a similar approach of elongating the maneuver arrow. That would also make the arrow more intuitive at large intersections: #3589. Apple Maps no longer displays maneuver arrows at all. Both applications display the traffic control device icon and highlight the intersection callout. #2928 would be a major usability improvement, making the maneuver arrow redundant, but maybe we could ticket out a simpler tweak to the maneuver arrow in the meantime.

@mapbox-github-ci-issues-public-1

Breaking Changes in MapboxCoreNavigation

Breaking API Changes

RouteLegProgress

  • removed method: init(leg:stepIndex:spokenInstructionIndex:) in RouteLegProgress

RouteProgress

  • removed method: init(route:options:legIndex:spokenInstructionIndex:) in RouteProgress

1 similar comment
@mapbox-github-ci-issues-public-1

Breaking Changes in MapboxCoreNavigation

Breaking API Changes

RouteLegProgress

  • removed method: init(leg:stepIndex:spokenInstructionIndex:) in RouteLegProgress

RouteProgress

  • removed method: init(route:options:legIndex:spokenInstructionIndex:) in RouteProgress

@ShanMa1991 ShanMa1991 force-pushed the shan/traffic-3843 branch 3 times, most recently from 9aed2c3 to e94914b Compare October 19, 2022 19:41
@ShanMa1991 ShanMa1991 requested a review from 1ec5 October 19, 2022 20:09
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.

This is looking great! There’s some potential follow-up work detailed below, but from an API standpoint the feature is good to go.

Comment on lines +464 to +474
let layerPosition = layerPosition(for: layerIdentifier)
try style.addPersistentLayer(shapeLayer, layerPosition: layerPosition)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, Google Maps takes a similar approach of elongating the maneuver arrow. That would also make the arrow more intuitive at large intersections: #3589. Apple Maps no longer displays maneuver arrows at all. Both applications display the traffic control device icon and highlight the intersection callout. #2928 would be a major usability improvement, making the maneuver arrow redundant, but maybe we could ticket out a simpler tweak to the maneuver arrow in the meantime.

CHANGELOG.md Show resolved Hide resolved
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.

Annotate traffic control devices along the route line
2 participants