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.

Sign up for GitHub

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

Add support for camera bounding box #23

Merged
merged 5 commits into from
Mar 13, 2024
Merged

Add support for camera bounding box #23

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1649514
proof of concept, adjust camera to bounding box
Patrick-Kladek Feb 25, 2024
2b72fe7
extened MLNMapViewCameraUpdating protocol
Patrick-Kladek Feb 26, 2024
9c78e13
add tests
Patrick-Kladek Feb 26, 2024
004ce94
Merge branch 'main' of https://github.com/hactar/maplibre-swiftui-dsl…
Patrick-Kladek Mar 12, 2024
dfc7526
run swiftformat
Patrick-Kladek Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Package.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
"kind" : "remoteSourceControl",
"location" : "https://github.com/maplibre/maplibre-gl-native-distribution.git",
"state" : {
"revision" : "3df876f8f2c6c591b0f66a29b3e216020afc885c",
"version" : "6.0.0"
"revision" : "818e1d6b83e4cbe8482eca3e6e57d3f560926157",
"version" : "6.1.1"
}
},
{
Expand Down
1 change: 1 addition & 0 deletions Sources/MapLibreSwiftUI/Extensions/MapLibre/MLNMapViewCameraUpdating.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ protocol MLNMapViewCameraUpdating: AnyObject {
direction: CLLocationDirection,
animated: Bool)
func setZoomLevel(_ zoomLevel: Double, animated: Bool)
func setVisibleCoordinateBounds(_ bounds: MLNCoordinateBounds, edgePadding: UIEdgeInsets, animated: Bool, completionHandler: (() -> Void)?)
}

extension MLNMapView: MLNMapViewCameraUpdating {
Expand Down
7 changes: 6 additions & 1 deletion Sources/MapLibreSwiftUI/MapViewCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,12 @@ public class MapViewCoordinator: NSObject {
case .trackingUserLocationWithCourse:
mapView.userTrackingMode = .followWithCourse
mapView.setZoomLevel(camera.zoom, animated: false)
case .rect, .showcase:
case let .rect(boundingBox, padding):
mapView.setVisibleCoordinateBounds(boundingBox,
edgePadding: padding,
animated: animated,
completionHandler: nil)
case .showcase:
// TODO: Need a method these/or to finalize a goal here.
break
}
Expand Down
28 changes: 25 additions & 3 deletions Sources/MapLibreSwiftUI/Models/MapCamera/CameraState.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


/// Showcasing GeoJSON, Polygons, etc.
case showcase(shapeCollection: MLNShapeCollection)
Expand All @@ -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, _):
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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!)

return "CameraState.rect(northeast: \(boundingBox.ne), southwest: \(boundingBox.sw))"
case .showcase(shapeCollection: let shapeCollection):
return "CameraState.showcase(shapeCollection: \(shapeCollection))"
}
}
}

extension MLNCoordinateBounds: Equatable, Hashable {

public func hash(into hasher: inout Hasher) {
hasher.combine(self.ne)
hasher.combine(self.sw)
}

public static func == (lhs: MLNCoordinateBounds, rhs: MLNCoordinateBounds) -> Bool {
return lhs.ne == rhs.ne && lhs.sw == rhs.sw
}
}

extension UIEdgeInsets: Hashable {

public func hash(into hasher: inout Hasher) {
hasher.combine(self.left)
hasher.combine(self.right)
hasher.combine(self.top)
hasher.combine(self.bottom)
}
}
19 changes: 17 additions & 2 deletions Sources/MapLibreSwiftUI/Models/MapCamera/MapViewCamera.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,21 @@ public struct MapViewCamera: Hashable {
direction: Defaults.direction,
lastReasonForChange: .programmatic)
}

// TODO: Create init methods for other camera states once supporting materials are understood (e.g. BoundingBox)

/// Positions the camera to show a specific region in the MapView.
///
/// - Parameters:
/// - box: Set the desired bounding box. This is a one time event and the user can manipulate by moving the map.
/// - edgePadding: Set the edge insets that should be applied before positioning the map.
/// - Returns: The MapViewCamera representing the scenario
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)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

EdgeInsets Padding

I think we're on the right track here, but this presents an interesting trade off we need to discuss:

  1. This may suggest we should remove Hashable from the MapViewCamera, CameraState, etc. The reason I wanted it was because it allows us to pass state containing a defined MapViewCamera through SwiftUI's NavigationStack. Basically meaning, you could inject a predefined MapViewCamera into a new app screen by passing it through a NavigationLink. 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.
  2. 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.
  3. 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 convert EdgeInsets to UIEdgeInsets down 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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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!


// TODO: Create init methods for other camera states once supporting materials are understood (e.g. BoundingBox)
}
4 changes: 2 additions & 2 deletions Tests/MapLibreSwiftUITests/Models/MapCamera/CameraStateTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ final class CameraStateTests: XCTestCase {
let northeast = CLLocationCoordinate2D(latitude: 12.3, longitude: 23.4)
let southwest = CLLocationCoordinate2D(latitude: 34.5, longitude: 45.6)

let state: CameraState = .rect(northeast: northeast, southwest: southwest)
XCTAssertEqual(state, .rect(northeast: northeast, southwest: southwest))
let state: CameraState = .rect(boundingBox: .init(sw: southwest, ne: northeast))
XCTAssertEqual(state, .rect(boundingBox: .init(sw: southwest, ne: northeast)))
XCTAssertEqual(
String(describing: state),
"CameraState.rect(northeast: CLLocationCoordinate2D(latitude: 12.3, longitude: 23.4), southwest: CLLocationCoordinate2D(latitude: 34.5, longitude: 45.6))")
Expand Down
15 changes: 14 additions & 1 deletion Tests/MapLibreSwiftUITests/Models/MapCamera/MapViewCameraTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import XCTest
import CoreLocation
import MapLibre
import XCTest
@testable import MapLibreSwiftUI

final class MapViewCameraTests: XCTestCase {
Expand Down Expand Up @@ -45,6 +46,18 @@ final class MapViewCameraTests: XCTestCase {
XCTAssertEqual(camera.pitch, .free)
XCTAssertEqual(camera.direction, 0)
}

func testBoundingBox() {
let southwest = CLLocationCoordinate2D(latitude: 24.6056011, longitude: 46.67369842529297)
let northeast = CLLocationCoordinate2D(latitude: 24.6993808, longitude: 46.7709285)
let bounds = MLNCoordinateBounds(sw: southwest, ne: northeast)
let camera = MapViewCamera.boundingBox(bounds)

XCTAssertEqual(camera.state, .rect(boundingBox: bounds, edgePadding: .init(top: 20, left: 20, bottom: 20, right: 20)))
XCTAssertEqual(camera.zoom, 1)
XCTAssertEqual(camera.pitch, .free)
XCTAssertEqual(camera.direction, 0)
}

// TODO: Add additional camera tests once behaviors are added (e.g. rect)

Expand Down
Loading