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

Feature querying methods should return Turf.Feature, not MapboxCommon.Feature #526

Closed
1ec5 opened this issue Jul 8, 2021 · 5 comments
Closed
Assignees
Labels
breaking change ⚠️ If your pull request introduces a breaking change and updates are required when version is published
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Jul 8, 2021

MapboxMap’s feature querying methods’ callbacks should accept Turf.Features as arguments, not MapboxCommon.Features. Otherwise, there’s no way for the developer to work with the results in other Mapbox products such as MapboxDirections or MapboxNavigation.

// TODO: Turf feature property of QueriedFeature
extension MapboxMap: MapFeatureQueryable {
public func queryRenderedFeatures(for shape: [CGPoint], options: RenderedQueryOptions? = nil, completion: @escaping (Result<[QueriedFeature], Error>) -> Void) {

At a glance, it seems like the code would simply need to map the results to this convenience initializer:

internal init?(_ feature: MapboxCommon.Feature) {

/cc @mapbox/maps-ios @ZiZasaurus @mapbox/navigation-ios

@1ec5 1ec5 added the breaking change ⚠️ If your pull request introduces a breaking change and updates are required when version is published label Jul 8, 2021
@bmt2018
Copy link

bmt2018 commented Jul 9, 2021

I was confused by this, as well. My GeoJSON source is a feature collection created from an array of Turf.Feature objects. But when querying for the rendered feature that was tapped on, I'm getting back an array of QueriedFeature which use instances of MapboxCommon.Feature. I was trying to cast MapboxCommon.Feature to Turf.Feature or to be able to init Turf.Feature using MapboxCommon.Feature but couldn't get either to work.

@nkdem
Copy link

nkdem commented Jul 9, 2021

I was confused by this, as well. My GeoJSON source is a feature collection created from an array of Turf.Feature objects. But when querying for the rendered feature that was tapped on, I'm getting back an array of QueriedFeature which use instances of MapboxCommon.Feature. I was trying to cast MapboxCommon.Feature to Turf.Feature or to be able to init Turf.Feature using MapboxCommon.Feature but couldn't get either to work.

I ran into the same issue so a workaround I used was something Mapbox's 'internal' code does:

extension Turf.Geometry {
/// Allows a Turf object to be initialized with an internal `Geometry` object.
/// - Parameter geometry: The `Geometry` object to transform.
internal init?(_ geometry: MapboxCommon.Geometry) {
switch geometry.geometryType {
case GeometryType_Point:
guard let coordinate = geometry.extractLocations()?.coordinateValue() else {
return nil
}
self = Turf.Geometry.point(Point(coordinate))

So for example, in my application I need access to the feature's geometry type however, mapbox's feature only returns an integer and I'd prefer a more readable format i.e., a String which Turf provides. I created a function:

// Convert from Mapbox geometry type to Turfs
	static func convertGeometryType(geometry: MapboxMaps.GeometryType) -> Turf.GeometryType? {
		switch geometry {
		case GeometryType_Point:
			return Turf.GeometryType.Point
		// ... You get the gist
	}

Hope that helps @bmt2018


Anyway, I think there should be more consistency in the querying methods to prevent from having to do workarounds like this. And by consistency what I mean is that some functionality relies on Turf.Feature whereas some on Mapbox's feature.

@bmt2018
Copy link

bmt2018 commented Jul 12, 2021

@nbieniek Thank you for the suggestion! I ended up doing something very similar as a workaround for now. I also hope that they'll make this more consistent in a future build of the SDK.

@macdrevx
Copy link
Contributor

We plan to do this by refining FeatureExtensionValue.featureCollection and QueriedFeature.feature so that their types are Turf types instead of the current MBXFeature-based types.

@neelmistry94 neelmistry94 self-assigned this Sep 13, 2021
@knov knov added this to the ga milestone Sep 16, 2021
@macdrevx macdrevx assigned macdrevx and unassigned neelmistry94 and macdrevx Sep 20, 2021
@macdrevx
Copy link
Contributor

Resolved by #717 and #628

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ If your pull request introduces a breaking change and updates are required when version is published
Projects
None yet
Development

No branches or pull requests

6 participants