Fix puck location during navigation #82
Conversation
|
Is this about #80 or something else? |
|
Yes, I wasn't aware that you created an issue already |
|
@michaelkirk could you take a look at this? You recently did some "puck work" so I'd assume you're better suited then me. :) |
| @@ -347,20 +355,18 @@ open class NavigationMapView: MLNMapView, UIGestureRecognizerDelegate { | |||
| return | |||
| } | |||
|
|
|||
| if !self.tracksUserCourse || self.userAnchorPoint != userCourseView?.center ?? self.userAnchorPoint { | |||
There was a problem hiding this comment.
Why did this boolean check go away? (TBH I'm not sure I understand what it was doing in the first place)
There was a problem hiding this comment.
I think this check was in place so centering the puck only happened if it was not centred already (optimisation maybe). As the map is moving during navigation we need to position the puck anyway, thats why I removed it.
There was a problem hiding this comment.
The map stops moving if the user pans. Does that matter?
Is this change required for the "position camera in bottom 1/3" of the screen work, or is this some bonus cleanup?
There was a problem hiding this comment.
Yeah, that's the point, if I move the map I want to look at a specific point. The puck is continuously updated and I can tap the "Resume" button to get typical navigation camera movement back.
There was a problem hiding this comment.
But why did it change in this PR? Again, I can't tell if you are doing random cleanup to make things nicer, or if this is required due to other changes you made to get the puck to appear in the bottom 1/3.
There was a problem hiding this comment.
I did remove this because it makes no difference. This if clause resolved to true in all scenarios I've tested. I guess this was relevant when we manually posited the userCourseView.center to the screen's center. Now that this is gone it's not necessary anymore.
| @@ -567,6 +568,7 @@ extension RouteMapViewController: NavigationViewDelegate { | |||
| if userTrackingMode == .none, !self.isInOverviewMode { | |||
| self.navigationView.wayNameView.isHidden = true | |||
| } | |||
| mapView.userTrackingMode = userTrackingMode | |||
There was a problem hiding this comment.
Because otherwise I would need to set the userTracking mode in startNavigation(). This way is more reliable.
If you look at the whole function there seems to be something missing, userTrackingMode is read from the mapView, then updated but never written back to the mapView, so it's basically unused. I suspect this was the intent in the first place but could have changed in a previous PR (maybe on accident).
There was a problem hiding this comment.
I'm not understanding why this would need to change to address the "position camera in bottom 1/3" of the screen logic. Is it required or is this some bonus work?
There was a problem hiding this comment.
This is done as a precaution to sync the tracking mode when navigating. Why else would we read, then modify the trackingMode but not assign it to the mapView? You could even say it fixes something a previous PR missed, otherwise we could remove this function altogether as it does nothing.
There was a problem hiding this comment.
I'm not saying you're wrong, but to review your code I need to understand what you are trying to do.
You said this pr is about positioning the puck in the bottom 1/3 of the viewport.
But this change is about something else, right? Did you observe a broken behavior with this? Or is this just about fixing code that "looked wrong"?
There was a problem hiding this comment.
If you are bothered by this line and want me to remove it because you think it's unnecessary then please say that. I've explained to you twice why I think we need this.
|
I’m not bothered - just trying to understand so I can review your code. On Aug 1, 2024, at 12:13, Patrick Kladek ***@***.***> wrote:
@Patrick-Kladek commented on this pull request.
In MapboxNavigation/RouteMapViewController.swift:
@@ -567,6 +568,7 @@ extension RouteMapViewController: NavigationViewDelegate {
if userTrackingMode == .none, !self.isInOverviewMode {
self.navigationView.wayNameView.isHidden = true
}
+ mapView.userTrackingMode = userTrackingMode
If you are bothered by this line and want me to remove it because you think it's unnecessary then please say that. I've explained to you twice why I think we need this.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
…n-ios into user-puck-positioning # Conflicts: # MapboxNavigation/NavigationMapView.swift
Description
Warning: do not merge right away, this contains changes from #76 which needs to be merged first!For some reason, the user puck was centered on the map. This reduced the visible road ahead and made navigation quite bad.
Open Tasks
Infos for Reviewer
The trick was to create a padding close to the region where the user puck is positioned. This padding is then used when the camera is set for the MapView.
Here is a recording from our app (SwiftUI), while starting the navigation and transitioning between overhead mode. It's not perfect yet (the overhead move is not fully visible, need to investigate further) but it's a lot better than what we currently have. There is no workaround needed to get the puck down and it is also butter smooth.
Bildschirmaufnahme.Simulator.mov