stadiamaps / maplibre-swiftui-dsl-playground

DSL and SwiftUI integration for MapLibre
BSD 3-Clause "New" or "Revised" License
20 stars 6 forks source link

Support Navigation #39

Closed Patrick-Kladek closed 2 months ago

Patrick-Kladek commented 3 months ago

Description

Allows to use maplibre-navigation-ios with SwiftUI without duplicating targets or source code. We think this is the best way given the following requirements:

Tasks

Infos for Reviewer

Note #37 should be merged first as this PR contains the changes already.

Relevant changes are in files:

To make this work we are not using UIViewRepresentable, instead we are using UIViewControllerRepresentable and providing a default ViewController that has a mapView. This requirement is extracted into a protocol WrappedViewController and allows to instantiate a MapView specifying the ViewController via Generics:

@ViewBuilder
var mapView: some View {
    MapView<MapViewController>(styleURL: self.styleURL, camera: self.$camera) {
...
    }
}

Thats the only change needed if you want to continue using this Package without navigation support. However if you want to support navigation the following changes are needed:

Add this branch as a dependency to your project: https://github.com/maplibre/maplibre-navigation-ios/pull/54

import MapboxCoreNavigation
import MapboxNavigation

extension NavigationViewController: WrappedViewController {
    public typealias MapType = NavigationMapView
}

@State var route: Route?
@State var navigationInProgress: Bool = false

@ViewBuilder
var mapView: some View {
    MapView<NavigationViewController>(makeViewController: NavigationViewController(dayStyleURL: self.styleURL), styleURL: self.styleURL, camera: self.$mapStore.camera) {

    }
    .unsafeMapViewControllerModifier { navigationViewController in
        navigationViewController.delegate = self.mapStore
        if let route = self.route, self.navigationInProgress == false {
            let locationManager = SimulatedLocationManager(route: route)
            navigationViewController.startNavigation(with: route, locationManager: locationManager)
            self.navigationInProgress = true
        } else if self.route == nil, self.navigationInProgress == true {
            navigationViewController.endNavigation()
            self.navigationInProgress = false
        }

        navigationViewController.mapView.showsUserLocation = self.showUserLocation && self.mapStore.streetView == .disabled
    }
    .cameraModifierDisabled(self.route != nil)
}
ianthetechie commented 3 months ago

Looks like there's a merge conflict now. Could you fix that real quick for me make it easier to review? GitHub appears to just throw up its hands and gives a useless diff now 😂

Archdoog commented 3 months ago

Thanks for the PR @Patrick-Kladek!

Taking a look over this concept, it looks like the generalized goal is to introduce the add the convenience of maplibre/mapbox’s NavigationViewController to the SwiftUI MapView?

While there’s no harm in this approach, I think it’s a specialization that doesn’t align with the goals of this project. I’d propose this alternative:

  1. Create the navigation route style needed using the Symbol DSL in this repo. Here’s how we did it in ferrostar RouteStyleLayer.swift.
  2. That way you can just display the route like any other symbol on the map based on what the navigation repo has as the current route in state. This could easily be a @State driven variable from the navigation controller delegate updates.
  3. At this point, you could easily add the SwiftUI MapView to the maplibre-navigation UIKit implementation, replacing the NavigationMapView. OR better, why not just drive a full SwiftUI view w/ the navigation repo’s controller/core state, wrapping whatever UIKit stuff you need in their own simple representables?

I realize this may not be as convenient as just swapping out the MLNMapView, but having used Mapbox’s navigation without the full view controller in my own iOS app, I think it would actually be relatively reasonable to follow that process, even if it was just in your app implementation.

Feel free to reach out over Slack, happy to dive in, discuss in more detail and provide some examples of how I've done the above myself before.

Patrick-Kladek commented 3 months ago

@ianthetechie just fixed the merge conflicts

hactar commented 2 months ago

Thanks for the PR @Patrick-Kladek!

Taking a look over this concept, it looks like the generalized goal is to introduce the add the convenience of maplibre/mapbox’s NavigationViewController to the SwiftUI MapView?

While there’s no harm in this approach, I think it’s a specialization that doesn’t align with the goals of this project. I’d propose this alternative:

  1. Create the navigation route style needed using the Symbol DSL in this repo. Here’s how we did it in ferrostar RouteStyleLayer.swift.
  2. That way you can just display the route like any other symbol on the map based on what the navigation repo has as the current route in state. This could easily be a @State driven variable from the navigation controller delegate updates.
  3. At this point, you could easily add the SwiftUI MapView to the maplibre-navigation UIKit implementation, replacing the NavigationMapView. OR better, why not just drive a full SwiftUI view w/ the navigation repo’s controller/core state, wrapping whatever UIKit stuff you need in their own simple representables?

I realize this may not be as convenient as just swapping out the MLNMapView, but having used Mapbox’s navigation without the full view controller in my own iOS app, I think it would actually be relatively reasonable to follow that process, even if it was just in your app implementation.

Feel free to reach out over Slack, happy to dive in, discuss in more detail and provide some examples of how I've done the above myself before.

I have adjusted the PR to include a convenience init so that the call site no longer changes if you do not need a UIViewController backing, so the old MapView(...) init returns, which I think is much cleaner and I hope resolves some potential reservations.

As for aligning with project goals: I do agree that a specialization towards MapLibre Navigation is not a project goal. But I don't think this is was this PR is.

In this revised PR, the call site for all existing users does not change - for anyone who does not need a UIViewController based backing, nothing changes. But for anyone who does, for example to include a UIViewController based navigation system such as MapLibre Navigation, this PR opens up this possibility. This is also not the only reason someone might need a UIViewController backed system: UIViewController offers all that UIView does, plus more, and UIViewRepresentable is just a convenience for UIViewControllerRespresentable I would argue - which is more powerful.

In other words, personally I do not see this PR as a specialization, but as a generalization, and now swiftly hidden behind a convenience init :).

We thought of some other alternatives and tried discussing them here: https://osmus.slack.com/archives/C06U5MM097B/p1717157153623649

Maybe we can discuss this in one of the calls on Wednesday? ☺️