Add support for camera bounding box #23
Conversation
|
@Archdoog thanks for the tipps. I'm quite surprised how easy it was to add this functionality. |
| public static func boundingBox(_ box: MLNCoordinateBounds, edgePadding: UIEdgeInsets = .init(top: 20, left: 20, bottom: 20, right: 20)) -> MapViewCamera { | ||
| // zoom, pitch & direction are ignored. | ||
| return MapViewCamera(state: .rect(boundingBox: box, edgePadding: edgePadding), | ||
| zoom: 1, | ||
| pitch: Defaults.pitch, | ||
| direction: Defaults.direction, | ||
| lastReasonForChange: .programmatic) | ||
| } |
There was a problem hiding this comment.
I'm a bit unsure about this:
1st how do we pass the edge padding to the camera? This works but I don't know if it's the best solution.
2nd and more importantly: zoom, pitch & direction are ignored for this camera setup. Wouldn't it make more sense to add these parameters as associated values to the respective (CameraState) enum cases? Otherwise adding parameters that are ignored might cause some confusion.
There was a problem hiding this comment.
EdgeInsets Padding
I think we're on the right track here, but this presents an interesting trade off we need to discuss:
- This may suggest we should remove
Hashablefrom the MapViewCamera, CameraState, etc. The reason I wanted it was because it allows us to pass state containing a definedMapViewCamerathrough SwiftUI'sNavigationStack. Basically meaning, you could inject a predefinedMapViewCamerainto a new app screen by passing it through aNavigationLink. If we do remove this, it's not the end of the world, it just makes it trickier for a developer to set their initial camera state on presenting a new screen in their app. - We could decide it's worth keeping
Hashablebecause of theNavigationLinkvalue support https://developer.apple.com/documentation/swiftui/navigationlink/init(_:value:)-kj9v. In this case, we might want to create our own padding struct (e.g.ShowcasePadding) that more easily conforms toHashable, then outputs/converts to a UIEdgeInsets once it's called on the map view. - We could keep it as is, in which case my only recommendation is we use SwiftUI's EdgeInsets on this top layer and move the hashable stuff to the right places in the
Sources/Extensions/folder. Then we'd need to convertEdgeInsetstoUIEdgeInsetsdown in the coordinator, but that would make it SwiftUI focused on top later/entry point.
This same discussion applies to the bounding box definition type as well MLNCoordinateBounds. A lot of the stuff in the underlying MapLibre code is older UIKit based code, so it's not unreasonable to wrap things in a SwiftUI focused top layer.
Zoom, Pitch & Direction
This is a good question. We had a similar issue deciding how to handle the .centered state's coordinates. In the end we decided if the value is only required for certain state(s) we should put it on the CameraState enum as a variable.
I think the boundingBox case suggests we should do the same for zoom as well. Since zoom only applies to the centered and tracking... states. Pitch and direction seem like they should also follow that path unless they actually can be manipulated and maintained when showcasing (e.g. not sure how maplibre handles centering on a bounding box with padding at different compass directions for example, it may always just revert the pitch and direction to default north + straight down?).
Any additional thoughts @ianthetechie?
There was a problem hiding this comment.
EdgeInsets
We could decide it's worth keeping Hashable because of the NavigationLink value support https://developer.apple.com/documentation/swiftui/navigationlink/init(_:value:)-kj9v. In this case, we might want to create our own padding struct (e.g. ShowcasePadding) that more easily conforms to Hashable, then outputs/converts to a UIEdgeInsets once it's called on the map view.
I tend to prefer this approach of writing our own padding struct which has a computed property for easy conversion to UIEdgeInsets (I don't have any great name ideas myself, but I think MapInsets or something makes sense; I understand the Showcase term but I'm not sure it's widely used). The reason being that it keeps our public API stable in the event that MapLibre does something else in the future (ex: modernizes) or Apple introduces surprise breakage by adding a protocol conformance.
This same discussion applies to the bounding box definition type as well MLNCoordinateBounds. A lot of the stuff in the underlying MapLibre code is older UIKit based code, so it's not unreasonable to wrap things in a SwiftUI focused top layer.
Agreed.
Zoom, Pitch & Direction
I think the boundingBox case suggests we should do the same for zoom as well. Since zoom only applies to the centered and tracking... states.
Agreed. I'm in favor of moving zoom to the CameraState as associated values for the relevant variants. In general, the principle we follow is "make illegal state unrepresentable" whenever possible, and you've found a solid shortcoming as it relates to bounding boxes.
Thanks!
| case rect(northeast: CLLocationCoordinate2D, southwest: CLLocationCoordinate2D) // TODO: make a bounding box? | ||
| case rect(boundingBox: MLNCoordinateBounds, edgePadding: UIEdgeInsets) |
There was a problem hiding this comment.
I can use MLNCoordinateBounds as an associated value in this enum, however I need to add Equatable & Hashable conformance.
Can this have any side effects? Let's say in a big project these extensions are already added, wouldn't this clash and result in build errors?
Patrick-Kladek
changed the title
|
@Archdoog @ianthetechie ready for review |
| public static func boundingBox(_ box: MLNCoordinateBounds, edgePadding: UIEdgeInsets = .init(top: 20, left: 20, bottom: 20, right: 20)) -> MapViewCamera { | ||
| // zoom, pitch & direction are ignored. | ||
| return MapViewCamera(state: .rect(boundingBox: box, edgePadding: edgePadding), | ||
| zoom: 1, | ||
| pitch: Defaults.pitch, | ||
| direction: Defaults.direction, | ||
| lastReasonForChange: .programmatic) | ||
| } |
There was a problem hiding this comment.
EdgeInsets Padding
I think we're on the right track here, but this presents an interesting trade off we need to discuss:
- This may suggest we should remove
Hashablefrom the MapViewCamera, CameraState, etc. The reason I wanted it was because it allows us to pass state containing a definedMapViewCamerathrough SwiftUI'sNavigationStack. Basically meaning, you could inject a predefinedMapViewCamerainto a new app screen by passing it through aNavigationLink. If we do remove this, it's not the end of the world, it just makes it trickier for a developer to set their initial camera state on presenting a new screen in their app. - We could decide it's worth keeping
Hashablebecause of theNavigationLinkvalue support https://developer.apple.com/documentation/swiftui/navigationlink/init(_:value:)-kj9v. In this case, we might want to create our own padding struct (e.g.ShowcasePadding) that more easily conforms toHashable, then outputs/converts to a UIEdgeInsets once it's called on the map view. - We could keep it as is, in which case my only recommendation is we use SwiftUI's EdgeInsets on this top layer and move the hashable stuff to the right places in the
Sources/Extensions/folder. Then we'd need to convertEdgeInsetstoUIEdgeInsetsdown in the coordinator, but that would make it SwiftUI focused on top later/entry point.
This same discussion applies to the bounding box definition type as well MLNCoordinateBounds. A lot of the stuff in the underlying MapLibre code is older UIKit based code, so it's not unreasonable to wrap things in a SwiftUI focused top layer.
Zoom, Pitch & Direction
This is a good question. We had a similar issue deciding how to handle the .centered state's coordinates. In the end we decided if the value is only required for certain state(s) we should put it on the CameraState enum as a variable.
I think the boundingBox case suggests we should do the same for zoom as well. Since zoom only applies to the centered and tracking... states. Pitch and direction seem like they should also follow that path unless they actually can be manipulated and maintained when showcasing (e.g. not sure how maplibre handles centering on a bounding box with padding at different compass directions for example, it may always just revert the pitch and direction to default north + straight down?).
Any additional thoughts @ianthetechie?
| @@ -44,10 +44,32 @@ extension CameraState: CustomDebugStringConvertible { | |||
| return "CameraState.trackingUserLocationWithHeading" | |||
| case .trackingUserLocationWithCourse: | |||
| return "CameraState.trackingUserLocationWithCourse" | |||
| case .rect(northeast: let northeast, southwest: let southwest): | |||
| return "CameraState.rect(northeast: \(northeast), southwest: \(southwest))" | |||
| case .rect(boundingBox: let boundingBox, _): | |||
There was a problem hiding this comment.
I've created a swiftformat ticket #27. We should wait for that to merge then we can let it sort out all the space/tab consistency.
There was a problem hiding this comment.
This is now merged @Patrick-Kladek. Can you merge with the main branch and then run swiftformat locally to clean up the formatting? (Thanks @Archdoog for doing that!)
| @@ -26,7 +26,7 @@ public enum CameraState: Hashable { | |||
| case trackingUserLocationWithCourse | |||
|
|
|||
| /// Centered on a bounding box/rectangle. | |||
| case rect(northeast: CLLocationCoordinate2D, southwest: CLLocationCoordinate2D) // TODO: make a bounding box? | |||
| case rect(boundingBox: MLNCoordinateBounds, edgePadding: UIEdgeInsets = .init(top: 20, left: 20, bottom: 20, right: 20)) | |||
There was a problem hiding this comment.
Once we decide on the right option for padding discussed in my large comment below, we can update this and put the defaults in the Defaults struct.
@Patrick-Kladek I added that main discussion/comment which we need to figure out more broadly. Let me know what you think and let's see what @ianthetechie has to say as well. Once we answer that, then we can jump into the smaller review items. |
|
Heads up: I've just pushed a change related to what was discussed in #16 which breaks some existing code and created a few merge conflicts. |
…-playground into enhancement/camera-bounding-box # Conflicts: # Package.resolved # Sources/MapLibreSwiftUI/Models/MapCamera/CameraState.swift # Sources/MapLibreSwiftUI/Models/MapCamera/MapViewCamera.swift # Tests/MapLibreSwiftUITests/Models/MapCamera/CameraStateTests.swift # Tests/MapLibreSwiftUITests/Models/MapCamera/MapViewCameraTests.swift
|
Thanks a bunch for your contribution @Patrick-Kladek! This looks mergeable to me now. I see a few minor things to clean up (like a completion handler which I don't think does anything and doesn't really belong), and of course the larger discussion of a breaking change to move zoom out into the states from the camera, but that can happen separately on our side. Your contribution is valuable as-is :) |
|
Okay that was fast, thanks. Yesterday before going to sleep I merged in main and then continued to weigh in the pros & cons. I guess as I only have insights into the bounding-box and less into the other camera options it's best if you decide how to proceed here. |
|
Yeah no worries. I actually had most of a branch finished where I was working on those issues anyways over the weekend to support an unrelated use case, so this was a timely discussion to get me thinking :) Thanks again and hope the changes are working well for you! |
Description
Allow showing map in a bounding box, therefore extend MapViewCamera.
Tasks
Infos for Reviewer
We might need to update MapViewCamera as zoom, pitch & direction are not needed for every camera configuration. We have the option to either make all of them optional, however then we lose swift compile checks, instead I would propose to move these properties to each enum case in CameraState.