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

Fixed Flying Puck issue #1732

Closed
wants to merge 1 commit into from
Closed

Fixed Flying Puck issue #1732

wants to merge 1 commit into from

Conversation

apavani
Copy link

@apavani apavani commented Sep 19, 2018

Ensures that puck is positioned in the right location immediately after the camera is set to the appropriate the location.

Closes #1731

cc @1ec5

…ra is set.

Co-authored-by: Vincent Sam <Vincent.Sam@mapbox.com>
@apavani apavani requested a review from 1ec5 September 19, 2018 23:57
@vincethecoder vincethecoder added wip and removed wip labels Sep 19, 2018
@akitchen
Copy link
Contributor

akitchen commented Sep 20, 2018

This is fantastic, @vincethecoder @apavani ! Thanks for diving in together to address this.

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.

Please verify that the puck continues to animate between location updates, whether tracking course or not, and that the puck does not animate when panning, zooming, or rotating using gestures. We’ll also need to verify that the puck appears in the correct position accounting for content insets, particularly in CarPlay.

}
if !tracksUserCourse || userAnchorPoint != userCourseView?.center ?? userAnchorPoint {
if !tracksUserCourse {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change effectively reverts #1653. We’ll need to verify the results in CarPlay.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tried this in a CarPlay unit – it’s worse than what I saw in #1731 (comment) on the simulator for some reason. The main map has the puck shifted upwards by about a third of the screen, and it isn’t possible to show the route at any time (whether in preview, turn-by-turn, or overview mode).

setCamera(newCamera, withDuration: duration, animationTimingFunction: function, edgePadding: padding) { [weak self] in
guard let strongSelf = self else { return }
if strongSelf.userAnchorPoint != strongSelf.userCourseView?.center ?? strongSelf.userAnchorPoint {
UIView.animate(withDuration: 0, delay: 0, options: [.curveLinear, .beginFromCurrentState], animations: {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I’m reading this correctly, we’re chaining animations together, snapping the course view back to an accurate location after an initial animation. But isn’t it that initial animation that looks incorrect?

Copy link
Contributor

Choose a reason for hiding this comment

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

As suggested in the ticket (#1731), I think setting the duration on the outer animation closure to 0 when a user is interacting with the map is a better idea.

Copy link
Contributor

@frederoni frederoni Sep 26, 2018

Choose a reason for hiding this comment

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

This patch seems to be fixing the turntable part in #1731

turntable.patch
diff --git a/MapboxNavigation/NavigationMapView.swift b/MapboxNavigation/NavigationMapView.swift
index 3d2b9ea6..ceaf87da 100644
--- a/MapboxNavigation/NavigationMapView.swift
+++ b/MapboxNavigation/NavigationMapView.swift
@@ -411,6 +411,16 @@ open class NavigationMapView: MGLMapView, UIGestureRecognizerDelegate {
             guard let location = userLocationForCourseTracking else { return }
             userCourseView?.layer.removeAllAnimations()
             userCourseView?.center = convert(location.coordinate, toPointTo: self)
+            
+            if let view = userCourseView as? UserCourseView,
+                let location = userLocationForCourseTracking {
+                
+                view.update?(location: location,
+                             pitch: self.camera.pitch,
+                             direction: self.direction,
+                             animated: false,
+                             tracksUserCourse: self.tracksUserCourse)
+            }
         }
     }
     

@akitchen
Copy link
Contributor

@frederoni can you also take a look please?

We need to account for how this affects CarPlay as well as transitioning between overview and route following modes.

@frederoni
Copy link
Contributor

CarPlay aside, I have a hard time seeing any regressions or improvements with this change compared to master.

@nitaliano
Copy link
Contributor

I'm closing this out, this doesn't seem like the solution.

@nitaliano nitaliano closed this Oct 11, 2018
@vincethecoder
Copy link
Contributor

vincethecoder commented Oct 11, 2018

@nitaliano Currently looking into this issue. #1731 We already have a ticket that's looking into a similar issue with the puck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants