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

Reinvent the course tracking wheel #402

Merged
merged 44 commits into from
Oct 12, 2017
Merged

Reinvent the course tracking wheel #402

merged 44 commits into from
Oct 12, 2017

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Jul 19, 2017

This PR reimplements MGLMapView’s course tracking mode in MapboxNavigation. Various hacks that we previously employed to force-snap MGLMapView’s user puck have been removed. The puck continues to look like a puck during turn-by-turn navigation after panning away.

straight ahead overhead

The developer can customize the user puck by setting the NavigationMapView.userCourseView property, which is easier to work with than a delegate method. The default user puck now looks more similar to what the Android navigation SDK displays: a larger disc that lies flatter against the map (with a halo instead of a shadow).

The developer can also customize the user puck’s location on screen by implementing the NavigationViewControllerDelegate.navigationViewController(_:mapViewUserAnchorPoint:) method.

More to come:

  • Move off MGLUserTrackingModeFollowWithCourse (to fix Bespoke course tracking #352)
  • Align the camera with the location view’s bottom-weighted position
  • Make the user puck’s position customizable (to fix Allow updating location puck bottom offset #323)
  • Implement a better-looking user puck (or port the relevant parts of MGLFaux3DUserLocationAnnotationView)
  • Avoid automatically tracking the user location when in non-tracking mode
  • Avoid bouncing back and forth between overview and front-facing views when in overview mode
  • Override pinch and tap gesture recognizers to center on the user puck
  • Disable animated camera transition while on battery power (to fix Throttle map rendering when device unplugged #370)
  • Hide Recenter button while tracking user course
  • Interpolate between location updates (not as much as 60fps, but more than 1fps)
  • Disable implicit animations of the user course view when not tracking course
  • Update example application to customize the new-style user course view

/cc @frederoni @bsudekum

@1ec5 1ec5 added op-ex Refactoring, Tech Debt or any other operational excellence work. topic: location ⚠️ DO NOT MERGE PR should not be merged! labels Jul 19, 2017
@1ec5 1ec5 added this to the v0.6.0-1 milestone Jul 19, 2017
// didUpdateLocationIncrementallyAnimated
// TODO: Call setVisibleCoordinateBounds instead to account for the location view’s bottom-weighted position on-screen.
let camera = MGLMapCamera(lookingAtCenter: location.coordinate, fromDistance: 300, pitch: 45, heading: location.course)
setCamera(camera, animated: true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For #370, we need to turn animation off, at least when the device is on battery power. If we leave animation on, we’d also need to enable a linear timing curve and stretch out the animation for one second to eliminate the jumping that currently occurs.

@frederoni
Copy link
Contributor

frederoni commented Jul 27, 2017

Note that this PR currently only builds with a custom build (mapbox/mapbox-gl-native#9651) of the iOS Maps SDK that exposes -[MGLMapView setCamera:withDuration:animationTimingFunction:edgePadding:completionHandler:]

@ericrwolfe ericrwolfe removed this from the v0.6.0-2 milestone Jul 27, 2017
@frederoni frederoni force-pushed the 1ec5-bespoke-course-352 branch 2 times, most recently from 6dd05f3 to e9514bf Compare July 31, 2017 11:38
@bsudekum
Copy link
Contributor

Wow, this rebase is rough times.

@bsudekum
Copy link
Contributor

bsudekum commented Aug 24, 2017

The rebase was so rough I just started fresh. @frederoni @1ec5 let me know if you want me to force push this branch over this one: https://github.com/mapbox/mapbox-navigation-ios/compare/bs-bespoke-course-352.

Also, do we want to move some of this logic to core? I sort of remember this being part of the conversion around this PR but I'm seeing a lot import UIKit.

@1ec5
Copy link
Contributor Author

1ec5 commented Aug 24, 2017

That branch missed some changes and undid part of #408. The rebase doesn’t seem so daunting in any case, although I’m getting some compiler errors in unrelated code.

Also, do we want to move some of this logic to core? I sort of remember this being part of the conversion around this PR but I'm seeing a lot import UIKit.

No, we shouldn’t have Core Navigation depend on the map SDK nor on UIKit if we can help it.

@1ec5 1ec5 self-assigned this Aug 25, 2017
@1ec5 1ec5 force-pushed the 1ec5-bespoke-course-352 branch from c7e7b37 to dab8f7c Compare August 25, 2017 02:28
@1ec5
Copy link
Contributor Author

1ec5 commented Aug 25, 2017

OK, I’ve rebased the PR. Currently there are two MGLMapView categories containing forward declarations, one in MapboxNavigation and the other in the Swift example application. Hopefully we can eliminate the former by taking advantage of mapbox/mapbox-gl-native#9651 and MGLMapViewDelegate.

@ericrwolfe
Copy link
Contributor

@1ec5 what're the next steps to finishing this PR out?

@1ec5 1ec5 force-pushed the 1ec5-bespoke-course-352 branch from dab8f7c to 29973ab Compare August 31, 2017 15:20
@1ec5
Copy link
Contributor Author

1ec5 commented Aug 31, 2017

what're the next steps to finishing this PR out?

I’ve been keeping the original post updated with the remaining work. Currently we have the following to-do items:

  • Avoid automatically tracking the user location when in non-tracking mode.
    • You can pan the map, but the viewport goes right back to the front-facing view on every location update. This should be pretty simple to fix.
  • Avoid bouncing back and forth between overview and front-facing views when in overview mode.
    • When you tap Overview, the viewport switches back to the front-facing view on every location update, only to immediately zoom back out to the overall route.
  • Override pinch and tap gesture recognizers to center on the user puck.
    • You don’t really notice this effect given the two issues above, but zoom and tap gestures are currently centered on the center of the map instead of the bottom-weighted puck. This may require exposing an additional MGLMapView internal method.
  • Interpolate between location updates.
    • With a 1fps frame rate, it’s just barely possible to follow where the camera is going. But at turns, the camera’s rotation can change significantly, easily causing the user to get disoriented. We’ll need to use a timer in addition to whatever timer the location manager may be using.

@frederoni
Copy link
Contributor

@1ec5 How do you feel about migrating the user puck to runtime styling? Porting the relevant parts from mapbox/mapbox-plugins-android#22?

It would solve the issue we are seeing when starting navigation and going from landscape to portrait where the user puck floats around because the camera and UIView animations are not in sync. I think it could also simplify course tracking. Though, styling it would be slightly more limited.

@1ec5
Copy link
Contributor Author

1ec5 commented Sep 1, 2017

It would solve the issue we are seeing when starting navigation and going from landscape to portrait where the user puck floats around because the camera and UIView animations are not in sync.

I don’t think this behavior is inherent to using a UIView. In fact, 049814b nearly eliminates the initial jump by making the various cameras more consistent with each other. The remaining minor jump seems to correspond to a view’s height – I wonder if we’re initially forgetting to account for a view in the edge padding.

I think it could also simplify course tracking. Though, styling it would be slightly more limited.

There are pros and cons to using runtime styling for the user puck:

  • Con: In user tracking mode, the user dot or user puck must remain fixed on the screen while the camera moves. Mapbox GL has no concept of UI elements that behave completely independently of the map. If we implement the user puck as style layers, as the Android navigation SDK does, we’d have to work around this mismatch by manually updating the puck’s geometry on every frame. That isn’t unreasonably difficult, but it isn’t simpler than what we’re already doing.

    Part of the motivation behind this PR is to reduce battery usage by putting less work onto mbgl’s shoulders. But if you’ve ever played with the Add Animated Annotation demo in macosapp, you know how resource-intensive it can be to frequently change a single point feature’s coordinate in a shape source, and that’s without moving the camera.

  • Weak pro: Once Update shaders: get 'icon-pitch-alignment' and fix for issue #9456 mapbox-gl-native#9479 makes it into a release, we’d get a realistic tilting effect using iconPitchAlignment, something I’ve struggled with for view-backed annotations ([WIP] Configurable degrees of freedom for annotation views mapbox-gl-native#5245). However, the difference would only be noticeable if we were using a rectangular user puck as opposed to the round one we’re using now.

  • Con: Mapbox GL has no concept of 3D models (Add support for rendering 3d models  mapbox-gl-js#3996), whereas a SceneKit model can already be used as a user puck. (Think 3D car model.)

@bsudekum
Copy link
Contributor

bsudekum commented Oct 12, 2017

Per chat yesterday, @frederoni is going to see what can be done to modify the zoom level without leaving followWithCourse.

@bsudekum
Copy link
Contributor

gif2

@frederoni would it be possible to disable animation so the puck is more stuck to the route to fix ^?

@frederoni
Copy link
Contributor

frederoni commented Oct 12, 2017

Fixed user puck while swiping #402 (comment)

tackswipe

@bsudekum bsudekum merged this pull request into master Oct 12, 2017
@bsudekum bsudekum deleted the 1ec5-bespoke-course-352 branch October 12, 2017 20:54
@1ec5 1ec5 restored the 1ec5-bespoke-course-352 branch October 12, 2017 20:59
@1ec5 1ec5 deleted the 1ec5-bespoke-course-352 branch October 12, 2017 21:02
@1ec5
Copy link
Contributor Author

1ec5 commented Oct 12, 2017

The merge was redone without squashing as b141077.

mappy-mobile pushed a commit to Mappy/mapbox-navigation-ios that referenced this pull request Sep 17, 2020
…ciation

Fix round-tripping pronunciations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
op-ex Refactoring, Tech Debt or any other operational excellence work. topic: location
Projects
None yet
5 participants