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

Navigation camera viewport API. #2826

Merged
merged 72 commits into from
Apr 13, 2021
Merged

Conversation

MaximAlien
Copy link
Contributor

@MaximAlien MaximAlien commented Feb 22, 2021

Description

NavigationCamera viewport APIs bring the ability to control camera related behavior, which is specific for navigation (mostly free-drive and active guidance navigation).

NavigationCamera API introduces such components:

  • NavigationCamera class, which works as a manager for all camera related operations (allows to move camera to following, overview and idle states).
  • ViewportDataSource protocol.
  • NavigationViewportDataSource class, which conforms to ViewportDataSource and listens to location changes in free drive and active guidance modes and prepares CameraOptions. This class prepares CameraOptions for both iOS and CarPlay, as they're different. Can be overridden by the end user to provide custom data source.
  • CameraView class, which allows to run UIViewPropertyAnimator animations. Will be removed when such feature appears in Maps SDK.
  • CameraStateTransition protocol.
  • NavigationCameraStateTransition class, which conforms to CameraStateTransition and provides set of specific animated transitions. Can be overridden by the end user.

Current PR also contains an attempt to decouple location related APIs (e.g. UserCourseView) and camera related APIs in NavigationMapView, this leads to removal/replacement of certain properties (defaultAltitude, zoomedOutMotorwayAltitude etc).

Example application with custom camera implementation can be found here: mapbox/mapbox-navigation-ios-examples#92.

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

Initial results

camera_maneuver_behavior_building_highlighting.mov
navigation_camera_zoom_out_behavior.mov
overview_camera_behavior.mov
transitions_behavior.mov
car_play_behavior.mov

Known issues

@MaximAlien MaximAlien requested a review from a team February 22, 2021 19:39
@MaximAlien MaximAlien self-assigned this Feb 22, 2021
@MaximAlien MaximAlien marked this pull request as draft February 22, 2021 19:39
@MaximAlien MaximAlien changed the base branch from release-v2.0 to vk/430-tile-manager February 22, 2021 21:36
@1ec5 1ec5 modified the milestones: v2.0.0 (Public Preview), v2.0.0 (GA) Feb 23, 2021
@1ec5 1ec5 added feature New feature request. op-ex Refactoring, Tech Debt or any other operational excellence work. labels Feb 23, 2021
@MaximAlien MaximAlien force-pushed the maxim/camera-viewport-api branch 3 times, most recently from 4ab3cab to 92e7897 Compare March 2, 2021 21:13
Base automatically changed from vk/430-tile-manager to release-v2.0 March 2, 2021 21:30
@MaximAlien MaximAlien force-pushed the maxim/camera-viewport-api branch 3 times, most recently from f0853c7 to f4ede64 Compare March 4, 2021 18:00
@truburt truburt modified the milestones: v2.0.0 (GA) (iOS), v2.0.0 (GA) Mar 5, 2021
@1ec5 1ec5 modified the milestones: v2.0.0 (GA), v2.0.0 (RC) Mar 5, 2021
@MaximAlien MaximAlien force-pushed the maxim/camera-viewport-api branch 2 times, most recently from 7ae6b7b to 861f66f Compare March 5, 2021 23:26
@MaximAlien MaximAlien force-pushed the maxim/camera-viewport-api branch 2 times, most recently from 335fda5 to 0d079f9 Compare March 9, 2021 20:13
Sources/MapboxNavigation/BoundingBox.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/CLLocation.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/CLLocationDegrees.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/CameraOptions.swift Outdated Show resolved Hide resolved
*/
open func updatePreferredFrameRate(for routeProgress: RouteProgress) {
guard tracksUserCourse else { return }
public func updatePreferredFrameRate(for routeProgress: RouteProgress) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that was supposed to be fixed as part of #2643?

CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@avi-c avi-c left a comment

Choose a reason for hiding this comment

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

Thank you. This looks good.

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.

I think we should try to get some more certainty on #2826 (comment) and #2826 (comment) before landing, but otherwise the code looks to be in good shape.

Comment on lines +56 to +72
/**
Controls the distance on route after the current maneuver to include in the frame.

Defaults to `100.0` meters.
*/
public var distanceToFrameAfterManeuver: CLLocationDistance = 100.0

/**
Controls how much the bearing can deviate from the location's bearing, in degrees.

In case if set, the `bearing` property of `CameraOptions` during active guidance navigation
won't exactly reflect the bearing returned by the location, but will also be affected by the
direction to the upcoming framed geometry, to maximize the viewable area.

Defaults to `20.0` degrees.
*/
public var maximumBearingSmoothingAngle: CLLocationDirection? = 20.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per our discussion with @d-prukop distanceToFrameAfterManeuver and maximumBearingSmoothingAngle were made publicly available to provide more control over default camera behavior. There are several other properties, which are exposed in Mapbox Navigation for Android, but haven't made their way into main branch yet (and not yet present on iOS). We can add those in scope of tail-work. iOS implementation also doesn't use boolean flags to control usage of such properties (to simplify API), instead certain parameters provided as optionals.

@MaximAlien MaximAlien merged commit c28e1e7 into release-v2.0 Apr 13, 2021
@MaximAlien MaximAlien deleted the maxim/camera-viewport-api branch April 13, 2021 19:34
Delegate, which is used to notify `NavigationCamera` regarding upcoming `CameraOptions`
related changes.
*/
public var delegate: ViewportDataSourceDelegate?
Copy link
Contributor

@bamx23 bamx23 Apr 17, 2021

Choose a reason for hiding this comment

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

@MaximAlien Should we use weak here? I see a possible retain loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, will be addressed in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, SwiftLint produces a warning for such issues.

@@ -116,7 +122,6 @@ extension ViewController {
let branchNames = branchEdgeIdentifiers.flatMap { edgeNames(identifier: $0) }
statusString += " at \(branchNames.joined(separator: ", "))"
}
print(statusString)
Copy link
Contributor

Choose a reason for hiding this comment

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

This print statement was intentional: otherwise, all the electronic horizon work above is for naught. Ideally there would be some UI associated with this output, but a print statement was more expedient at the time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in ca8be54.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request. op-ex Refactoring, Tech Debt or any other operational excellence work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants