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

Allow to use map SDK location layer during turn-by-turn navigation #2968

Merged
merged 20 commits into from Jun 11, 2021

Conversation

ShanMa1991
Copy link
Contributor

@ShanMa1991 ShanMa1991 commented Apr 30, 2021

Description

This pr is to allow developer choose between UserCourseView and map SDK location layer.

  • allow the developer to use map SDK
  • allow the map puck receiving location update
  • allow the map puck receiving heading update
  • update the vanishing route line synced with the map sdk puck.
  • allow the customized Puck update orientation under PassiveLocationManager
  • Fix UserPuckCourseView should be subclassable #3033. update the UserHaloCourseView and UserPuckCourseView to open access and subclassable.

Implementation

Added the ability to allow developers choose between NavigationMapView.userCourseView and map SDK location layer through the NavigationMapView.puckType . The default puck is the NavigationMapView.userCourseView. To configure a customized 2D Puck as following :

        var configuration = Puck2DConfiguration()
        if #available(iOS 13.0, *) {
            configuration.topImage = UIImage(systemName: "arrow.up")
            configuration.scale = .constant(2.0)
        }

To configure a customized 3D Puck as following:

        let uri = Bundle.main.url(forResource: "someModelFile", withExtension: "gltf")
        let myModel = Model(uri: uri,
                            position: [-122.396152, 37.79129], // Coordinates of the model in `[longitude, latitude]`format
                            orientation: [0, 0, 90])
        let scalingExpression = someExpression // Setting an expression to  scale the model based on camera zoom
        let configuration = Puck3DConfiguration(model: myModel, modelScale: .expression(scalingExpression))

To display the puck on NavigationMapView, use navigationMapView.puckType = .puck3D(configuration: configuration).
To display the puck during turn-by-turn navigation, use navigationViewController.navigationMapView.puckType = .puck3D(configuration: configuration)

To allow the puck rotate under PassiveLocationManager, developers need to receive Notification.passiveLocationDataSourceDidUpdate and call the navigationMapView.updateUserCourseView(location) when there's a new location to adjust the rotation of the customized puck.

Screenshots or Gifs

2D customized puck example during turn-by-turn navigation:
mapPuck

3D customized puck example during turn-by-turn navigation:
3DPuckExample

3D customized puck example under PassiveLocationManager:
passiveLocationPuck

@ShanMa1991 ShanMa1991 added the UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. label Apr 30, 2021
@ShanMa1991 ShanMa1991 added this to the v2.0.0 (RC) milestone Apr 30, 2021
@ShanMa1991 ShanMa1991 self-assigned this Apr 30, 2021
@ShanMa1991
Copy link
Contributor Author

When we call the mapView.location.locationProvider(_:didUpdateLocations:), it will automatically assign the latestLocation.heading to the puck. And the SimulatedLocationManager doesn't have a function to update a simulated CLHeading.

@ShanMa1991 ShanMa1991 marked this pull request as ready for review May 3, 2021 19:50
@ShanMa1991 ShanMa1991 requested a review from a team May 3, 2021 19:50
@ShanMa1991
Copy link
Contributor Author

Instead of creating a routeLine as a LocationConsumer to receive location update, the vanishing route line in navigationMapView also need routeProgress to calculate the fractionTraveled. So it keeps synced with the map SDK puck at the same rate. The map SDK puck also avoids receiving location update directly from the default AppleLocationProvider(), especially during simulation mode or PassiveLocationManager.

@ShanMa1991 ShanMa1991 force-pushed the shan/location-puck-902 branch 3 times, most recently from 2679994 to 273e4bc Compare May 12, 2021 18:14
@ShanMa1991
Copy link
Contributor Author

Now the camera is shaking stronger than #2998, when in simulation mode during turn-by-turn navigation, the camera should only receive the routeProgress for the location Update

@ShanMa1991 ShanMa1991 requested a review from 1ec5 May 13, 2021 18:06
@ShanMa1991 ShanMa1991 force-pushed the shan/location-puck-902 branch 2 times, most recently from 8623ae0 to eb4cd41 Compare May 17, 2021 20:32
@zugaldia
Copy link
Member

zugaldia commented May 27, 2021

Per team chat, we've decided that #2998 is not blocking this PR to land. However, we expect a fix from upstream dependencies ahead of the final v2 release. We will be using #2998 to track that work.

@ShanMa1991
Copy link
Contributor Author

ShanMa1991 commented May 28, 2021

Because right now the mapSDK doesn't support the puck layer bearing rotation based on course but on heading. Now when the CLHeading is missing from the Location information, the mapSDK puck would not rotate and will always point to one side.

To handle this issue, the following cases are considered:

  1. During free drive, we're using PassiveLocationManager, and call the navigationMapView.updateUserCourseView(location) to receive location update from NotificationCenter. instead of from the mapView.location.locationProvider.
  2. During the turn-by-turn navigation. If it's under simulation mode, the mapSDK puck will only be updated from navigationMapView.updateUserCourseView, to avoid the influence of the mapView.location.locationProvider, which may cause the shifting of puck location and bearing, we stop the location update from it.
  3. During turn-by-turn navigation. If it's not under simulation mode, we only receive location update to update the puck from the mapView.location.locationProvider, using the snapped location to indicate the true location and true heading.

Sources/MapboxNavigation/PuckType.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/PuckType.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/PuckType.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/PuckType.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/UserCourseView.swift Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Sources/MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
let modelSourceCode = [NavigationMapView.ModelKeyIdentifier.modelSouce: model]
if let data = try? JSONEncoder().encode(modelSourceCode),
let jsonDictionary = try? JSONSerialization.jsonObject(with: data, options: []) as? [String: Any] {
try? mapView.mapboxMap.style.setSourceProperty(for: NavigationMapView.SourceIdentifier.puck3DSource, property: "models", value: jsonDictionary)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use Model more directly, rather than serializing to JSON?

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 the source type of the 3D puck layer isModelSource, which is internal and could not be updated directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

ModelSource became internal in mapbox/mapbox-maps-ios#198. I don’t see any rationale associated with it, so it might have been just for lack of external demand. ModelSource would be more type-safe and forward-compatible than raw JSON usage. Since this is how the map SDK currently requires you to use the models property, let’s track ModelSource usage as tail work and also request an upstream change.

/cc @macdrevx

Comment on lines 218 to 234
do {
try self.mapView.mapboxMap.style.updateLayer(withId: mainRouteLayerIdentifier) { (lineLayer: inout LineLayer) throws in
let congestionSegments = routeProgress.route.congestionFeatures(legIndex: self.currentLegIndex, roadClassesWithOverriddenCongestionLevels: self.roadClassesWithOverriddenCongestionLevels)
let mainRouteLayerGradient = self.routeLineGradient(congestionSegments,
fractionTraveled: newFractionTraveled)

lineLayer.lineGradient = .expression(Expression.routeLineGradientExpression(mainRouteLayerGradient))
}

try self.mapView.mapboxMap.style.updateLayer(withId: mainRouteCasingLayerIdentifier) { (lineLayer: inout LineLayer) throws in
let mainRouteCasingLayerGradient = self.routeLineGradient(fractionTraveled: newFractionTraveled)

lineLayer.lineGradient = .expression(Expression.routeLineGradientExpression(mainRouteCasingLayerGradient))
}
} catch {
print("Failed to update main route line layer.")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is basically a duplicate of code in .default case (other than newFractionTraveled and fractionTraveled properties usage). Can it be extracted into separate method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I'll update the implementation of the vanishing route line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the comment below, the vanishing route line method now is still using the Timer incase of overwhelming calculation.

Sources/MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
@ShanMa1991
Copy link
Contributor Author

After test, the another constraint that block using the frame to update vanishing route line is based on the following reason.

  1. For example, we could implement the following way in the RouteLineController.init(_:) to replace the Timer:
        init(_ navigationViewData: NavigationViewData) {
            self.navigationViewData = navigationViewData
            routeProgress = navigationViewData.router.routeProgress
            let date = Date()
            self.navigationMapView.mapView.mapboxMap.onEvery(.renderFrameFinished) { [weak self] _ in
                print("timeInterval:\(abs(date.timeIntervalSinceNow))")
                guard let self = self,
                      let location = self.navigationMapView.mostRecentUserCourseViewLocation else { return }
                self.navigationMapView.updateUserCourseView(location)
                self.navigationMapView.updateTraveledRouteLine(location.coordinate)
                self.navigationMapView.updateRoute(self.routeProgress)
            }
        }

During the turn by turn navigation, if we zoom pretty close, and we have a following log of the time interval of the render frame finished:

timeInterval:6.394365072250366
timeInterval:6.417618036270142
timeInterval:6.426851034164429
timeInterval:6.450319051742554
//That's a long time interval where the frame will not be updated
timeInterval:19.978631019592285
timeInterval:19.996222019195557
timeInterval:20.01080596446991
timeInterval:20.027930974960327
timeInterval:20.043519020080566
timeInterval:20.059892058372498
timeInterval:20.076043009757996

That means that when we zoom pretty close and the route line would not be updated because the frame will not keep rendering update. There would show a tail of the route line behind the puck.

  1. And another way to replace the Timer is to use location Indicator callback similar as in Android(though we don't have it right now). But the current constraint is that the rotation of the mapSDK puck is only based on heading, not course. When in simulation mode, or other circumstances, the puck would always point to one side. To fix this issue, in this pr, we stop receiving the location update from mapView.locationProvider in these cases to update the puck, and use the navigationMapView.updateUserCourseView(_:animated:) to update the puck layer and source properties directly as a workaround for the course based rotation. Only after mapSDK could allow the puck rotation based on course, we could allow the puck receive location update from the locationProvider in these cases. And then we could use the locationConsumer of other way to update the vanishing route line.

So based on the above reasons, the tail work of replacing the Timer of vanishing route line would be resolved and addressed in another separate ticket with the render frame rate improvement, or the rotation improvement.

Sources/MapboxNavigation/UserLocationStyle.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/UserLocationStyle.swift Outdated Show resolved Hide resolved
docs/jazzy.yml Outdated Show resolved Hide resolved
Sources/MapboxNavigation/NavigationViewController.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Sources/MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
let modelSourceCode = [NavigationMapView.ModelKeyIdentifier.modelSouce: model]
if let data = try? JSONEncoder().encode(modelSourceCode),
let jsonDictionary = try? JSONSerialization.jsonObject(with: data, options: []) as? [String: Any] {
try? mapView.mapboxMap.style.setSourceProperty(for: NavigationMapView.SourceIdentifier.puck3DSource, property: "models", value: jsonDictionary)
Copy link
Contributor

Choose a reason for hiding this comment

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

ModelSource became internal in mapbox/mapbox-maps-ios#198. I don’t see any rationale associated with it, so it might have been just for lack of external demand. ModelSource would be more type-safe and forward-compatible than raw JSON usage. Since this is how the map SDK currently requires you to use the models property, let’s track ModelSource usage as tail work and also request an upstream change.

/cc @macdrevx

@ShanMa1991
Copy link
Contributor Author

The tail work of the vanishing route line implementation will be addressed in #3069 separately.

Example/ViewController+FreeDrive.swift Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Sources/MapboxNavigation/UserLocationStyle.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/UserLocationStyle.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/NavigationViewController.swift Outdated Show resolved Hide resolved
Sources/MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
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.

Overall, I’m concerned about special-casing specific location managers/providers to achieve the normal behavior, which could leave an application-defined location manager/provider with unexpected behavior despite conforming to the right protocols.

I’m approving this PR on the condition that simulatesLocation be based on a property of the location manager class (via the NavigationLocationManager class and any necessary overrides), as discussed in #2968 (comment). I think that’s the minimum necessary to ensure that custom subclasses still work correctly. But we should investigate #2968 (comment) as tail work soon while there’s still time: having NavigationLocationManager conform to LocationProvider would be pretty elegant compared to the confusing and limiting PassiveLocationDataSource/PassiveLocationManager split we have now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI Work related to visual components, Android Auto, Camera, 3D, voice, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants