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

Support Navigation #39

Merged
merged 26 commits into from
Jul 15, 2024

Conversation

Patrick-Kladek
Copy link
Contributor

@Patrick-Kladek Patrick-Kladek commented Jun 7, 2024

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:

  • No dependency on maplibre-navigation-ios, so users without navigation are not increasing their app size.
  • Easy maintenance as changes are affecting both (maplibre & maplibre-navigation), eg. camera improvements

Tasks

  • duplicate MapView to NavigationMapView
  • add binding to a route and start/stopp navigation when route is available
  • extract common requirements in protocol
  • remove maplibre-navigation dependency
  • extract start/stop logic to consumer and test in existing app

Infos for Reviewer

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

Relevant changes are in files:

  • MapView.swift
  • MapViewController.swift
  • MapViewCoordinator.swift
  • MapViewModifiers.swift

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: maplibre/maplibre-navigation-ios#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)
}

hactar and others added 15 commits May 15, 2024 15:56
* updating tests

* adding pitch setup for initial load

* renaming CameraPitch

* pitch workaround for initial load

* format improvements

* fixing tests

* linting
commit 857c1ce
Author: PW <[email protected]>
Date:   Wed May 15 14:02:58 2024 +0200

    adding example

commit ea40ab1
Author: PW <[email protected]>
Date:   Wed May 15 13:49:24 2024 +0200

    adding expand clusters modifier
@Patrick-Kladek Patrick-Kladek marked this pull request as ready for review June 11, 2024 12:33
@ianthetechie
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

@ianthetechie just fixed the merge conflicts

@hactar hactar force-pushed the enhancement/unify-navigation branch from 5da79ef to 565da23 Compare July 8, 2024 11:16
@hactar
Copy link
Collaborator

hactar commented Jul 8, 2024

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? ☺️

Copy link
Collaborator

@Archdoog Archdoog left a comment

Choose a reason for hiding this comment

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

Generally looking pretty good 👍. Have a couple of easy questions and suggestions.

Sources/MapLibreSwiftUI/MapViewController.swift Outdated Show resolved Hide resolved
Sources/MapLibreSwiftUI/MapView.swift Show resolved Hide resolved
@ianthetechie ianthetechie merged commit 605d855 into maplibre:main Jul 15, 2024
2 checks passed
@Patrick-Kladek Patrick-Kladek deleted the enhancement/unify-navigation branch July 31, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants