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

Respect heading while walking #2215

Closed
wants to merge 4 commits into from
Closed

Respect heading while walking #2215

wants to merge 4 commits into from

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Aug 24, 2019

This PR contains these user-facing improvements to walking navigation:

  • The user’s heading is used instead of the course when reorienting the camera while walking, since heading is more granular and more accurate at low speeds (i.e., walking speeds).
  • When rerouting a pedestrian, avoid restricting the route’s initial heading. When driving or cycling, it makes sense to restrict the initial heading, to avoid routes that confusingly begin with a U-turn. However, a pedestrian can turn on a dime, and the direction in an initial “Head [direction] on [street]” instruction is more meaningful to a pedestrian than to a motorist or cyclist.

and these developer-facing improvements:

  • Added RouteControllerNotificationUserInfoKey.headingKey to the user info dictionary of Notification.Name.routeControllerWillReroute, Notification.Name.routeControllerDidReroute, and Notification.Name.routeControllerProgressDidChange notifications. Core Location treats heading as first-class state instead of subordinating it under location, because heading updates from the magnetometer come in at a different rate than location updates from the GPS receiver.
  • Both RouteController and LegacyRouteController can store a heading, so now the Router protocol requires it publicly. (Also fixed a latent issue where RouteController ignored heading updates. This issue didn’t matter previously because nothing was using RouteController.heading.)
  • Deprecated the public but undocumented NavigationMapView.updateCourseTracking(location:camera:animated:) method in favor of a publicly documented NavigationMapView.updateCourseTracking(location:heading:camera:animated:) argument to the method for transitioning the map camera. (The existing method signature without the extra argument was undocumented but used in examples, so it has been retained in deprecated form.)

The user’s mode of transportation is determined by the current step’s transportMode, which is generally based on the routing profile but can be something else like train or ferry for walking and cycling routes. The improvements above affect only the walking transport mode.

To recap and to do:

  • Loosen rerouting heading while walking
  • Make heading a first-class piece of context alongside location
  • Rotate the map by heading instead of course while walking
  • Initially rotate the map by heading instead of course when enabling course tracking mode while walking
  • Update heading orientation: Rotate map based on heading while walking #1942 (comment)
  • Test loosened rerouting – may be challenging given the lack of heading simulation

Fixes #1942.

/cc @mapbox/navigation-ios @d-prukop

When rerouting a pedestrian, avoid restricting the route’s initial heading, since pedestrians can turn on a dime. Cleaned up an argument label.
Both RouteController and LegacyRouteController can store a heading, so now the Router protocol requires it publicly. Fixed an issue where RouteController ignored heading updates.
Prefer the heading over the course when reorienting the camera while walking. Added a heading argument to the method for transitioning the map camera and documented the method. The existing method signature without the extra argument was undocumented but used in examples, so it has been retained in deprecated form.
@1ec5 1ec5 added bug Something isn’t working topic: location topic: camera labels Aug 24, 2019
@1ec5 1ec5 added this to the v0.38.0 milestone Aug 24, 2019
@1ec5 1ec5 self-assigned this Aug 24, 2019
@1ec5 1ec5 changed the title Various walking mode improvements Respect heading while walking Aug 24, 2019
@1ec5 1ec5 requested a review from JThramer August 24, 2019 02:58
@1ec5 1ec5 added wip ⚠️ DO NOT MERGE PR should not be merged! labels Aug 24, 2019
@pintonos
Copy link

Is there currently a workaround like rotating the arrow dependent of the user location heading? Or is it not possible to change the arrow orientation from outside the box?

@bsudekum
Copy link
Contributor

Do you think this should also be applied to bikes?

@1ec5
Copy link
Contributor Author

1ec5 commented Aug 29, 2019

Is there currently a workaround like rotating the arrow dependent of the user location heading? Or is it not possible to change the arrow orientation from outside the box?

There isn’t currently a workaround, but this PR implements this behavior out of the box. Feel free to check out this branch and try it out – let us know what you think.

Do you think this should also be applied to bikes?

Not really, because I think it’s a bit of a sledgehammer specific to walking. A pedestrian on foot has a turning radius of 0 and can turn on a dime, whereas a cyclist may need to find a good place to turn around. Also, I’d be annoyed if I saw the map wiggle around to match my handlebar as I struggle to get going. 😅

I think there will be some opportunities for rerouting heuristics that benefit cyclists as well as pedestrians, but it probably will be orthogonal to using the magnetometer.

Copy link
Contributor

@JThramer JThramer left a comment

Choose a reason for hiding this comment

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

Looks great! one comment.

.rawLocationKey: rawLocation, //raw
]
userInfo[.headingKey] = heading
userInfo[.rawHeadingKey] = heading // heading snapping not yet implemented (an unnecessary?)
Copy link
Contributor

Choose a reason for hiding this comment

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

YAGNI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could get rid of rawHeadingKey, but that could be problematic if we ever do need to implement heading snapping for some reason. We’d want the non-raw values to be snapped when a raw value is available. That said, the comment refers to the prospect that heading snapping may not be especially useful.

@1ec5 1ec5 modified the milestones: v0.38.0, v0.39.0 Oct 2, 2019
@1ec5 1ec5 modified the milestones: v0.39.0, v1.0.0 Nov 26, 2019
@JThramer
Copy link
Contributor

JThramer commented Apr 1, 2020

Blocked by #2319

@1ec5 1ec5 modified the milestones: v1.0.0, v0.x next Apr 14, 2020
@1ec5 1ec5 added the size: S label Apr 14, 2020
@1ec5 1ec5 removed this from the v0.40.0 milestone Apr 24, 2020
@1ec5 1ec5 changed the base branch from master to main October 7, 2020 18:16
@mapbox mapbox deleted a comment from AhmerKhan1 May 27, 2021
@1ec5 1ec5 added the UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. label May 28, 2021
@1ec5 1ec5 added this to the v2.1 (beta) milestone Jul 12, 2021
@truburt truburt assigned jill-cardamon and unassigned 1ec5 Oct 4, 2021
@1ec5 1ec5 modified the milestones: v2.1-beta, v2.2-beta Nov 11, 2021
@MaximAlien
Copy link
Contributor

Closing this PR in favor of #3620.

@MaximAlien MaximAlien closed this Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn’t working ⚠️ DO NOT MERGE PR should not be merged! topic: camera topic: location 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.

Rotate map based on heading while walking
7 participants