-
Notifications
You must be signed in to change notification settings - Fork 27
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
Usage as non navigation map #275
base: main
Are you sure you want to change the base?
Changes from 6 commits
5dca5ed
874deb7
5a2c28b
d97f21c
85f551d
e6eb881
8082fed
7d76294
1758ca2
fc4ac4b
cf9f2cf
9fe7d95
9afdad2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,7 +83,7 @@ public protocol FerrostarCoreDelegate: AnyObject { | |
|
||
private let networkSession: URLRequestLoading | ||
private let routeProvider: RouteProvider | ||
private let locationProvider: LocationProviding | ||
public let locationProvider: LocationProviding | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did this become public? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I needed access to this to help me with debugging, could be turned to private again, though one thing that might be useful is making this a public var, so that one can switch between the simulated location provider and the real location provider if needed (either for debugging, or for tunnel simulation, unless ferrostar core is planning to handle that internally. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm open to making this public so that it would be easier to switch (though I personally have a hard time thinking this will get any use outside debug builds, but I also know I lack imagination sometimes; call me out an whatever I'm missing :D). The main reason it's private right now is that it's not actually safe to just go resetting it at will. It would need setter logic to ensure that everything below is aware and subscribes to updates correctly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think 99% of the cases will be debug cases. That said, I do think its an important debug case:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those debug use cases make sense.
You might be righte about this... Some use cases will be better solved with a custom LocationProvider (ex: if you have wheel sensors, sophisticated inertial processing, etc.). I haven't yet examined the existing approaches closely enough to see how these work. One approach would be the developer using the simulated provider directly and replacing it as you're implying here. This would actually be pretty straightforward; it has some shortcomings right now like not being able to very easily set the speed, but those are easy to address. Another could be a "fallback" configuration where the dev doesn't have to reset the location provider directly but rather provides a fallback designed to simulate progress + parameters on when to switch back and forth, what speed to simulate progress at, how long (distance) to simulate progress, decay in speed as you get past some threshold, etc. This is all pretty half-baked, but the idea behind that all would be to let you do something like "my router tells me that this tunnel is 3km long. My average speed over 30 seconds leading up to the tunnel has been 70kph with minimal variance, so we'll simulate progress at 68kph and hopefully we get a smooth transition." And at the end of the tunnel idk just stop or slow way down? And kick back over to "live" location once we get a location update from CoreLocation with an accuracy of better than 50m. TL;DR though, yes, switching should be possible, both directly and maybe indirectly ;) |
||
private var navigationController: NavigationControllerProtocol? | ||
private var routeRequestInFlight = false | ||
private var lastAutomaticRecalculation: Date? = nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,13 +17,15 @@ public struct DynamicallyOrientingNavigationView: View, CustomizableNavigatingIn | |
let navigationCamera: MapViewCamera | ||
|
||
private var navigationState: NavigationState? | ||
private let userLayers: [StyleLayerDefinition] | ||
private let userLayers: () -> [StyleLayerDefinition] | ||
|
||
private let mapViewModifiersWhenNotNavigating: (MapView<MLNMapViewController>) -> AnyView | ||
|
||
public var topCenter: (() -> AnyView)? | ||
public var topTrailing: (() -> AnyView)? | ||
public var midLeading: (() -> AnyView)? | ||
public var bottomTrailing: (() -> AnyView)? | ||
|
||
var onTapExit: (() -> Void)? | ||
|
||
public var minimumSafeAreaInsets: EdgeInsets | ||
|
@@ -44,21 +46,26 @@ public struct DynamicallyOrientingNavigationView: View, CustomizableNavigatingIn | |
public init( | ||
styleURL: URL, | ||
camera: Binding<MapViewCamera>, | ||
navigationCamera: MapViewCamera = .automotiveNavigation(), | ||
navigationCamera: MapViewCamera = .automotiveNavigation() | ||
, | ||
navigationState: NavigationState?, | ||
minimumSafeAreaInsets: EdgeInsets = EdgeInsets(top: 16, leading: 16, bottom: 16, trailing: 16), | ||
onTapExit: (() -> Void)? = nil, | ||
@MapViewContentBuilder makeMapContent: () -> [StyleLayerDefinition] = { [] } | ||
@MapViewContentBuilder makeMapContent: @escaping () -> [StyleLayerDefinition] = { [] }, | ||
mapViewModifiersWhenNotNavigating: @escaping (MapView<MLNMapViewController>) -> AnyView = { transferView in | ||
AnyView(transferView) | ||
} | ||
) { | ||
self.styleURL = styleURL | ||
self.navigationState = navigationState | ||
self.minimumSafeAreaInsets = minimumSafeAreaInsets | ||
self.onTapExit = onTapExit | ||
|
||
userLayers = makeMapContent() | ||
userLayers = makeMapContent | ||
|
||
_camera = camera | ||
self.navigationCamera = navigationCamera | ||
self.mapViewModifiersWhenNotNavigating = mapViewModifiersWhenNotNavigating | ||
} | ||
|
||
public var body: some View { | ||
|
@@ -69,11 +76,14 @@ public struct DynamicallyOrientingNavigationView: View, CustomizableNavigatingIn | |
camera: $camera, | ||
navigationState: navigationState, | ||
onStyleLoaded: { _ in | ||
camera = navigationCamera | ||
} | ||
) { | ||
userLayers | ||
} | ||
if navigationState?.isNavigating == true { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this be called when the user calls There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope - unless start navigation is called before the style completes. But this call here should only be seen as a fallback, something else should set the camera to navigation camera, like startNavigation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idea for this concept generally. Could we extend the NavigationMapView with navigations state event view modifier(s) (e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might not actually need this if we go with your suggestion (which I'll elaborate on in my review comments), since the modifiers get applied any time state changes ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While we might not need it, it might still be convenient? one can already achieve this by a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, fair point ;) I'm open to draft implementations from either of you if you think it's helpful. I originally was super enthusiastic when @Archdoog described it on our call but then realized it might not be a hard requirement with the other proposed changes. |
||
camera = navigationCamera | ||
} | ||
|
||
}, | ||
makeMapContent: userLayers, | ||
mapViewModifiersWhenNotNavigating: mapViewModifiersWhenNotNavigating) | ||
|
||
.navigationMapViewContentInset(NavigationMapViewContentInsetMode( | ||
orientation: orientation, | ||
geometry: geometry | ||
|
@@ -84,7 +94,7 @@ public struct DynamicallyOrientingNavigationView: View, CustomizableNavigatingIn | |
LandscapeNavigationOverlayView( | ||
navigationState: navigationState, | ||
speedLimit: nil, | ||
showZoom: true, | ||
showZoom: navigationState?.isNavigating == true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I'd actually never like to show the zoom buttons, and rely only on pinch/tap to zoom in all cases. I wonder if a delegate query would be a better choice (or maybe init parameter is more conventional in SwiftUI? I'm still pretty new to SwiftUI and not confident on the conventions) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, I'm not a fan of zoom buttons either, but I wanted to keep it as close as possible to the current setup first :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All for that. The only reason I added the zoom buttons in the first place was to include them in the available UI components and to provide a simple verification for zoom behavior while navigating. Long term goal is the existing configurable static zoom as well as a fancier dynamic zoom per activity. E.g. zooms out when as route annotation speed increases. We're almost there on the "current annotation" state, so this probably won't be hard shortly. This can become as fancy as we want as long as various use cases are supported 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this SHOULD be configurable. Thanks for pointing it out! On zoom or no zoom, that's a decision that app developers can make. I don't like the screen real estate, but OTOH, 80% of the time pinch zoom makes the camera stop following me (in every nav app I can think of), and that's ALMOST never what I wanted, so I can appreciate the button option ;) |
||
onZoomIn: { camera.incrementZoom(by: 1) }, | ||
onZoomOut: { camera.incrementZoom(by: -1) }, | ||
showCentering: !camera.isTrackingUserLocationWithCourse, | ||
|
@@ -104,7 +114,7 @@ public struct DynamicallyOrientingNavigationView: View, CustomizableNavigatingIn | |
PortraitNavigationOverlayView( | ||
navigationState: navigationState, | ||
speedLimit: nil, | ||
showZoom: true, | ||
showZoom: navigationState?.isNavigating == true, | ||
onZoomIn: { camera.incrementZoom(by: 1) }, | ||
onZoomOut: { camera.incrementZoom(by: -1) }, | ||
showCentering: !camera.isTrackingUserLocationWithCourse, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,16 +17,22 @@ public struct NavigationMapView: View { | |
var mapViewContentInset: UIEdgeInsets = .zero | ||
var onStyleLoaded: (MLNStyle) -> Void | ||
let userLayers: [StyleLayerDefinition] | ||
|
||
let mapViewModifiersWhenNotNavigating: (MapView<MLNMapViewController>) -> AnyView | ||
|
||
// TODO: Configurable camera and user "puck" rotation modes | ||
|
||
private var navigationState: NavigationState? | ||
private var navigationState: NavigationState? | ||
|
||
@State private var locationManager = StaticLocationManager(initialLocation: CLLocation()) | ||
|
||
// MARK: Camera Settings | ||
|
||
@Binding var camera: MapViewCamera | ||
|
||
private var effectiveMapViewContentInset: UIEdgeInsets { | ||
return navigationState?.isNavigating == true ? mapViewContentInset : .zero | ||
} | ||
|
||
/// Initialize a map view tuned for turn by turn navigation. | ||
/// | ||
|
@@ -41,15 +47,20 @@ public struct NavigationMapView: View { | |
camera: Binding<MapViewCamera>, | ||
navigationState: NavigationState?, | ||
onStyleLoaded: @escaping ((MLNStyle) -> Void), | ||
@MapViewContentBuilder _ makeMapContent: () -> [StyleLayerDefinition] = { [] } | ||
@MapViewContentBuilder makeMapContent: () -> [StyleLayerDefinition] = { [] }, | ||
mapViewModifiersWhenNotNavigating: @escaping (MapView<MLNMapViewController>) -> AnyView = { transferView in | ||
AnyView(transferView) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wish we wouldn't have to type-erase here, but I didn't find a way to get this to work with it returning some View or a MapView as this closure may contain both MapView modifiers such as onMapTapGesture, but also View modifiers such as onTapGesture. |
||
} | ||
) { | ||
self.styleURL = styleURL | ||
_camera = camera | ||
self.navigationState = navigationState | ||
self.onStyleLoaded = onStyleLoaded | ||
userLayers = makeMapContent() | ||
self.userLayers = makeMapContent() | ||
self.mapViewModifiersWhenNotNavigating = mapViewModifiersWhenNotNavigating | ||
} | ||
|
||
@ViewBuilder | ||
public var body: some View { | ||
MapView( | ||
styleURL: styleURL, | ||
|
@@ -74,11 +85,12 @@ public struct NavigationMapView: View { | |
// Overlay any additional user layers. | ||
userLayers | ||
} | ||
.mapViewContentInset(mapViewContentInset) | ||
.mapViewContentInset(effectiveMapViewContentInset) | ||
.mapControls { | ||
// No controls | ||
} | ||
.onStyleLoaded(onStyleLoaded) | ||
.applyTransform(if: navigationState?.isNavigating != true, transform: mapViewModifiersWhenNotNavigating) | ||
.ignoresSafeArea(.all) | ||
} | ||
|
||
|
@@ -94,6 +106,21 @@ public struct NavigationMapView: View { | |
} | ||
} | ||
|
||
extension MapView<MLNMapViewController> { | ||
@ViewBuilder | ||
func applyTransform<Content: View>( | ||
if condition: Bool, | ||
transform: (MapView<MLNMapViewController>) -> Content | ||
) -> some View { | ||
if condition { | ||
transform(self) | ||
} else { | ||
self | ||
} | ||
} | ||
} | ||
|
||
|
||
#Preview("Navigation Map View") { | ||
// TODO: Make map URL configurable but gitignored | ||
let state = NavigationState.modifiedPedestrianExample(droppingNWaypoints: 4) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a weird entry with your local machine details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, yes I was referencing my local copy so that I could edit it directly in Xcode. Will fix in next commit.