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.

Sign up for GitHub

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

Fix puck location during navigation #82

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix puck location during navigation #82

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b0c1397
prevent code from running again after setting the same property again
Patrick-Kladek Jul 1, 2024
4111671
reset LocationManager on endNavigation
Patrick-Kladek Jul 1, 2024
427721a
hide endOfRoute UI on dismiss button tap
Patrick-Kladek Jul 1, 2024
62f8f45
fix user puck location during navigation
Patrick-Kladek Jul 12, 2024
089dfb9
Merge branch 'maplibre:main' into user-puck-positioning
Patrick-Kladek Jul 14, 2024
b4741b0
rename parameter
Patrick-Kladek Jul 15, 2024
bd0bfc1
Merge branch 'main' of https://github.com/maplibre/maplibre-navigatio…
Patrick-Kladek Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions Example/example/SceneDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,24 @@ private extension SceneDelegate {
@objc
func cameraButtonTapped() {
guard let waypoint = self.waypoints.randomElement() else { return }

let distance = CLLocationDistance.random(in: 10 ... 100_000)
self.viewController.mapView.camera = .init(lookingAtCenter: waypoint.coordinate,
acrossDistance: distance,
pitch: 0,
heading: 0)

let userAnchorPoint = self.viewController.mapView.userCourseView!.center
Patrick-Kladek marked this conversation as resolved.
Show resolved Hide resolved
let padding = UIEdgeInsets(top: floor(userAnchorPoint.y),
left: floor(userAnchorPoint.x),
bottom: floor(self.viewController.mapView.frame.height - userAnchorPoint.y),
right: floor(self.viewController.mapView.frame.width - userAnchorPoint.x))

let camera = MLNMapCamera(lookingAtCenter: waypoint.coordinate,
acrossDistance: distance,
pitch: 0,
heading: 0)

self.viewController.mapView.setCamera(camera,
withDuration: 1,
animationTimingFunction: CAMediaTimingFunction(name: CAMediaTimingFunctionName.linear),
edgePadding: padding)
}

@objc
Expand Down
2 changes: 1 addition & 1 deletion MapboxNavigation/CarPlayNavigationViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@
return
}
self.mapView?.enableFrameByFrameCourseViewTracking(for: 3)
self.mapView?.setOverheadCameraView(from: userLocation, along: self.routeController.routeProgress.route.coordinates!, for: self.edgePadding)
self.mapView?.setOverheadCameraView(from: userLocation, along: self.routeController.routeProgress.route.coordinates!, insets: self.edgePadding)
Patrick-Kladek marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down Expand Up @@ -264,7 +264,7 @@

var maneuvers: [CPManeuver] = [primaryManeuver]

// Add tertiary text if available. TODO: handle lanes.

Check warning on line 267 in MapboxNavigation/CarPlayNavigationViewController.swift

View workflow job for this annotation

GitHub Actions / Code Style 

TODOs should be resolved (handle lanes.) (todo)
if let tertiaryInstruction = visualInstruction.tertiaryInstruction, !tertiaryInstruction.containsLaneIndications {
let tertiaryManeuver = CPManeuver()
tertiaryManeuver.symbolSet = tertiaryInstruction.maneuverImageSet(side: visualInstruction.drivingSide)
Expand All @@ -291,7 +291,7 @@

func endOfRouteFeedbackTemplate() -> CPGridTemplate {
let buttonHandler: (_: CPGridButton) -> Void = { [weak self] _ in
// TODO: no such method exists, and the replacement candidate ignores the feedback sent, so ... ?

Check warning on line 294 in MapboxNavigation/CarPlayNavigationViewController.swift

View workflow job for this annotation

GitHub Actions / Code Style 

TODOs should be resolved (no such method exists, and the...) (todo)
// self?.routeController.setEndOfRoute(rating: Int(button.titleVariants.first!.components(separatedBy: CharacterSet.decimalDigits.inverted).joined())!, comment: nil)
self?.carInterfaceController.popTemplate(animated: true)
self?.exitNavigation()
Expand Down
34 changes: 20 additions & 14 deletions MapboxNavigation/NavigationMapView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@
return anchorPoint
}

let contentFrame = bounds.inset(by: safeAreaInsets)
let contentFrame = self.bounds.inset(by: self.safeAreaInsets)
let courseViewWidth = self.userCourseView?.frame.width ?? 0
let courseViewHeight = self.userCourseView?.frame.height ?? 0
let edgePadding = UIEdgeInsets(top: 50 + courseViewHeight / 2,
Expand Down Expand Up @@ -293,7 +293,15 @@
if !cameraUpdated {
let newCamera = MLNMapCamera(lookingAtCenter: location.coordinate, acrossDistance: self.altitude, pitch: 45, heading: location.course)
let function = CAMediaTimingFunction(name: CAMediaTimingFunctionName.linear)
setCamera(newCamera, withDuration: 1, animationTimingFunction: function, edgePadding: UIEdgeInsets.zero, completionHandler: nil)

let userAnchorPoint = self.userAnchorPoint
let padding = UIEdgeInsets(top: floor(userAnchorPoint.y),
left: floor(userAnchorPoint.x),
bottom: floor(self.frame.height - userAnchorPoint.y),
right: floor(self.frame.width - userAnchorPoint.x))

// NOTE: When Camera is set without edge padding, user puck location can be out of sync
self.setCamera(newCamera, withDuration: 1, animationTimingFunction: function, edgePadding: padding, completionHandler: nil)
}
}

Expand Down Expand Up @@ -347,20 +355,18 @@
return
}

if !self.tracksUserCourse || self.userAnchorPoint != userCourseView?.center ?? self.userAnchorPoint {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did this boolean check go away? (TBH I'm not sure I understand what it was doing in the first place)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

UIView.animate(withDuration: duration, delay: 0, options: [.curveLinear, .beginFromCurrentState], animations: {
self.userCourseView?.center = self.convert(location.coordinate, toPointTo: self)
})
}
UIView.animate(withDuration: duration, delay: 0, options: [.curveLinear, .beginFromCurrentState], animations: {
self.userCourseView?.center = self.convert(location.coordinate, toPointTo: self)
})

if let userCourseView = userCourseView as? UserCourseView {
if let userCourseView = self.userCourseView as? UserCourseView {
if let customTransformation = userCourseView.update?(location: location, pitch: self.camera.pitch, direction: direction, animated: animated, tracksUserCourse: tracksUserCourse) {
customTransformation
} else {
self.userCourseView?.applyDefaultUserPuckTransformation(location: location, pitch: self.camera.pitch, direction: direction, animated: animated, tracksUserCourse: self.tracksUserCourse)
}
} else {
userCourseView?.applyDefaultUserPuckTransformation(location: location, pitch: self.camera.pitch, direction: direction, animated: animated, tracksUserCourse: self.tracksUserCourse)
self.userCourseView?.applyDefaultUserPuckTransformation(location: location, pitch: self.camera.pitch, direction: direction, animated: animated, tracksUserCourse: self.tracksUserCourse)
}
}

Expand Down Expand Up @@ -443,7 +449,7 @@
let line = MLNPolyline(coordinates: coords, count: UInt(coords.count))
let camera = cameraThatFitsShape(line, direction: direction, edgePadding: padding)

setCamera(camera, animated: false)
self.setCamera(camera, animated: false)
}

/**
Expand Down Expand Up @@ -743,7 +749,7 @@
}
}

// TODO: Change to point-based distance calculation

Check warning on line 752 in MapboxNavigation/NavigationMapView.swift

View workflow job for this annotation

GitHub Actions / Code Style 

TODOs should be resolved (Change to point-based distance...) (todo)
private func waypoints(on routes: [Route], closeTo point: CGPoint) -> [Waypoint]? {
let tapCoordinate = convert(point, toCoordinateFrom: self)
let multipointRoutes = routes.filter { $0.routeOptions.waypoints.count >= 3 }
Expand Down Expand Up @@ -1069,7 +1075,7 @@
/**
Sets the camera directly over a series of coordinates.
*/
@objc public func setOverheadCameraView(from userLocation: CLLocationCoordinate2D, along coordinates: [CLLocationCoordinate2D], for bounds: UIEdgeInsets) {
@objc public func setOverheadCameraView(from userLocation: CLLocationCoordinate2D, along coordinates: [CLLocationCoordinate2D], insets: UIEdgeInsets) {
self.isAnimatingToOverheadMode = true
let slicedLine = Polyline(coordinates).sliced(from: userLocation).coordinates
let line = MLNPolyline(coordinates: slicedLine, count: UInt(slicedLine.count))
Expand All @@ -1084,7 +1090,7 @@
camera.heading = 0
camera.centerCoordinate = userLocation
camera.altitude = self.defaultAltitude
setCamera(camera, withDuration: 1, animationTimingFunction: nil) { [weak self] in
self.setCamera(camera, withDuration: 1, animationTimingFunction: nil) { [weak self] in
self?.isAnimatingToOverheadMode = false
}
return
Expand All @@ -1094,8 +1100,8 @@
cam.pitch = 0
cam.heading = 0

let cameraForLine = camera(cam, fitting: line, edgePadding: bounds)
setCamera(cameraForLine, withDuration: 1, animationTimingFunction: nil) { [weak self] in
let cameraForLine = camera(cam, fitting: line, edgePadding: insets)
self.setCamera(cameraForLine, withDuration: 1, animationTimingFunction: nil) { [weak self] in
self?.isAnimatingToOverheadMode = false
}
}
Expand Down
6 changes: 4 additions & 2 deletions MapboxNavigation/RouteMapViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@
self.init()
self.routeController = routeController
self.delegate = delegate
self.automaticallyAdjustsScrollViewInsets = false

Check warning on line 137 in MapboxNavigation/RouteMapViewController.swift

View workflow job for this annotation

GitHub Actions / Build and Test 

'automaticallyAdjustsScrollViewInsets' was deprecated in iOS 11.0: Use UIScrollView's contentInsetAdjustmentBehavior instead
}

deinit {
Expand Down Expand Up @@ -207,6 +207,7 @@
self.navigationView.muteButton.isSelected = NavigationSettings.shared.voiceMuted
self.mapView.compassView.isHidden = true
self.mapView.tracksUserCourse = true
self.mapView.userTrackingMode = .followWithHeading

if let camera = self.pendingCamera {
self.mapView.camera = camera
Expand Down Expand Up @@ -259,7 +260,7 @@

if self.isInOverviewMode {
if let coordinates = routeController.routeProgress.route.coordinates, let userLocation = routeController.locationManager.location?.coordinate {
self.mapView.setOverheadCameraView(from: userLocation, along: coordinates, for: self.overheadInsets)
self.mapView.setOverheadCameraView(from: userLocation, along: coordinates, insets: self.overheadInsets)
}
} else {
self.mapView.tracksUserCourse = true
Expand Down Expand Up @@ -567,6 +568,7 @@
if userTrackingMode == .none, !self.isInOverviewMode {
self.navigationView.wayNameView.isHidden = true
}
mapView.userTrackingMode = userTrackingMode
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Bildschirmfoto 2024-08-01 um 14 22 26

Copy link
Collaborator

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

func mapView(_ mapView: MLNMapView, didFinishLoading style: MLNStyle) {
Expand Down Expand Up @@ -754,7 +756,7 @@
self.mapView.enableFrameByFrameCourseViewTracking(for: 3)
if let coordinates = self.routeController?.routeProgress.route.coordinates,
let userLocation = self.routeController?.locationManager.location?.coordinate {
self.mapView.setOverheadCameraView(from: userLocation, along: coordinates, for: self.overheadInsets)
self.mapView.setOverheadCameraView(from: userLocation, along: coordinates, insets: self.overheadInsets)
}
self.isInOverviewMode = true
}
Expand Down
Loading