Adding queryRenderedFeatures on iOS #419

tsemerad · 2016-09-21T19:14:56Z

I'm trying to make a PR adding queryRenderedFeatures support (or visibleFeaturesAtPoint and visibleFeaturesInRect on iOS), and I need some feedback.

I have the bridge set up to properly send the screen coordinate from JS to Obj C. However, it's failing on line 547 of RCTMapboxGL.m with Exception thrown while executing UI block: -[MGLMapView visibleFeaturesAtPoint:]: unrecognized selector sent to instance. I can't figure out why it's giving me this issue. I apologize if this is a simple issue I've overlooked - this is my first time writing React Native bridge code.

I also want feedback on the name and signature on the method. On iOS, it's called visibleFeaturesAtPoint and visibleFeaturesInRect, and on Android it's called queryRenderedFeatures. I'm planning on calling it queryRenderedFeatures, and allow passing in either a point or a rect.

…t to instance'

1ec5 · 2016-09-21T22:59:42Z

I have the bridge set up to properly send the screen coordinate from JS to Obj C. However, it's failing on line 547 of RCTMapboxGL.m with Exception thrown while executing UI block: -[MGLMapView visibleFeaturesAtPoint:]: unrecognized selector sent to instance.

That’s strange. The method definitely is implemented. What version of Xcode are you using? This is the SDK’s first method that uses the new NS_SWIFT_NAME attribute, so if you’re on Xcode 7.2 or lower, it might not be possible to call this method directly.

I'm planning on calling it queryRenderedFeatures, and allow passing in either a point or a rect.

That’s fine. The iOS naming is specific to Cocoa naming conventions, where a getter method without side effects would to be named without a verb.

tsemerad · 2016-09-21T23:57:20Z

I'm using XCode 7.3.1. I tested accessing other attributes and methods on _map, and they all worked.

tsemerad · 2016-09-22T17:19:34Z

I got it working now. I just had to remove and re-add Mapbox.framework from the XCode project, just how I originally had according to the manual installation instructions. Now it's recognizing the method properly. Is this something we'll need to warn people about?

…ints, polylines, and polygons

tsemerad · 2016-09-23T02:28:01Z

I have it working on iOS by passing in an options dictionary with a point parameter screen coordinate and optional layers array of layer ids. Converting the queried features to GeoJSON might need a little more work - it's working for Point, LineString, and Polygon, but haven't implemented MultiLineString or MultiPolygon. If there's already a known way to get GeoJSON from MGLFeatures, let me know. I see that mapbox-gl-native #6302 would provide a convenient method.

The question is, what do we want the queryRenderedFeatures signature to look like?

This is what I currently have, which I think needs some improvement:

map.queryRenderedFeatures(options, callback)
// example
map.queryRenderedFeatures({
    point: {
      screenCoordX: 212,
      screenCoordY: 111
    },
    layers: ['building']
  },
  (features) => console.log('do something with GeoJSON features')
)

I'm thinking of passing in a coordinate, rather than a screen point, to be more in line with Mapbox GL JS (I'm not sure if that's a concern). I started with a screen point since iOS uses visibleFeaturesAtPoint, and Android appears to use a screen point as well, but Mapbox GL JS doesn't. It accepts PointLike input. I'm not sure whether we want to add point-geometry as a dependency just to make it very similar to Mapbox GL JS.

Inspired by Mapbox GL JS:

map.queryRenderedFeatures(geometry, parameters, callback)
// example with point
map.queryRenderedFeatures([44.57, -123.42], {
    layers: ['building']
  },
  (features) => console.log('do something with GeoJSON features')
)

// example with bounding box
map.queryRenderedFeatures([[44.574, -123.426], [44.576, -123.424]], {
    layers: ['building']
  },
  (features) => console.log('do something with GeoJSON features')
)

Another alternative to passing [[latitudeSW, longitudeSW], [latitudeNE, longitudeNE]] would be [latitudeSW, longitudeSW, latitudeNE, longitudeNE] which matches bounds that we get from map.getBounds.

One other option, that might be more than we need (but is a bit more explicit):

map.queryRenderedFeatures(options, callback)
// example
// one of 'screenCoord', 'mapCoord', 'rect', and 'boundingBox' are required
map.queryRenderedFeatures({
    screenCoord: {
      x: 212,
      y: 111
    },
    mapCoord: {
      latitude: 44.57,
      longitude: -123.43
    },
    rect: {
      left: 0,
      top: 0,
      right: 10,
      bottom: 10
    },
    boundingBox: {
      southWest: {
        latitude: 44.574,
        longitude: -123.426
      },
      northEast: {
        latitude: 44.576,
        longitude: -123.424
      }
    },
    layers: ['building']
  },
  (features) => console.log('do something with GeoJSON features')
)

What do you think? Should we accept screen points, lat long coords, or both?

cc @bsudekum, @dapetcu21, @tmcw

1ec5 · 2016-09-23T05:44:52Z

If there's already a known way to get GeoJSON from MGLFeatures, let me know. I see that mapbox-gl-native #6302 would provide a convenient method.

mapbox/mapbox-gl-native#6302 would make it possible to create an MGLGeoJSONSource from an array of MGLFeatures, which is essentially opposite the direction you’d like to go. With an MGLGeoJSONSource, you could grab the geoJSONData, but round-tripping through MGLGeoJSONSource would be a pretty circuitous route to take for feature querying. mapbox/mapbox-gl-native#5623 (or the SDK-level wrapper around it) would ultimately expose a way to convert an MGLFeature to GeoJSON directly.

In the meantime, you’re on the right track with your conversion implementation. For collection types like MGLMultiPolyline and especially MGLShapeCollection, you’ll want to do the conversion recursively.

I'm thinking of passing in a coordinate, rather than a screen point, to be more in line with Mapbox GL JS (I'm not sure if that's a concern). I started with a screen point since iOS uses visibleFeaturesAtPoint, and Android appears to use a screen point as well, but Mapbox GL JS doesn't. It accepts PointLike input.

Based on GL JS’s documentation and examples, it’s clear queryRenderedFeatures() actually takes a screen coordinate or screen bounding box as well. PointLike is used for coordinates that aren’t geographic coordinates. You could accept geographic coordinates and convert them to screen coordinates before querying them, but that’ll make less sense once mapbox/mapbox-gl-js#3122 is implemented.

1ec5 · 2016-09-23T05:47:11Z

ios/RCTMapboxGL/RCTMapboxGLManager.m

+            NSSet<NSString *> *styleLayerIdentifiers = options[@"layers"];
+
+            CLLocationCoordinate2D location = [_map convertPoint:[sender locationInView:_map] toCoordinateFromView:_map];
+            CGPoint screenCoord = [sender locationInView:_map];


These two variables don’t appear to be used anywhere at the moment.

1ec5 · 2016-09-23T05:50:32Z

ios/RCTMapboxGL/RCTMapboxGLManager.m

+                }
+                // TODO: checks for MGLMultiPolyline and MGLMultiPolygon
+
+                NSDictionary *geoJSON = @{


Also set id to the value of the MGLFeature’s identifier property. (We don’t say “ID” much in Objective-C since id is a keyword.)

1ec5 · 2016-09-23T05:52:20Z

ios/RCTMapboxGL/RCTMapboxGLManager.m

+                    }
+                    geometryType = @"Polygon";
+                }
+                // TODO: checks for MGLMultiPolyline and MGLMultiPolygon


Also MGLMultiPoint and MGLShapeCollection. Note that MGLPolyline and MGLPolygon currently inherit from MGLMultiPoint (mapbox/mapbox-gl-native#5201), so only handle MGLMultiPoint after those two classes.

1ec5 · 2016-09-23T05:53:18Z

ios/RCTMapboxGL/RCTMapboxGLManager.m

+                if ([feature isKindOfClass:[MGLPointFeature class]]) {
+                    geometryType = @"Point";
+                    coordinates = (NSMutableArray *) @[@(feature.coordinate.longitude), @(feature.coordinate.latitude)];
+                } else if ([feature isKindOfClass:[MGLPolyline class]]) {


Since you’re checking for MGLPointFeature rather than MGLPointAnnotation in the first case above, it’d be more consistent to check for MGLPolylineFeature, MGLPolygonFeature, etc.

1ec5 · 2016-09-23T05:56:29Z

ios/RCTMapboxGL/RCTMapboxGLManager.m

+                        CLLocationCoordinate2D coord = multiPoint.coordinates[index];
+                        [coordinates addObject:@[@(coord.longitude), @(coord.latitude)]];
+                    }
+                    geometryType = @"Polygon";


A polygon can have multiple holes, represented as MGLPolygons inside MGLPolygon’s interiorPolygons property.

1ec5 · 2016-09-23T05:58:11Z

ios/RCTMapboxGL/RCTMapboxGLManager.m


            // TODO: also accept Rect to use [mapView visibleFeaturesInRect:]

-            // TODO: convert features to JS friendly objects
+            // TODO: switch to using MGLFeature's geoJSONFeature method if implemented - https://github.com/mapbox/mapbox-gl-native/issues/6302
+            NSMutableArray *geoJSONFeatures = [[NSMutableArray alloc] init];


Nit: you can also write this more succinctly as [NSMutableArray array]. As a possibly premature but reasonable optimization, you can also specify the size upfront: [NSMutableArray arrayWithCapacity:features.count].

1ec5 · 2016-09-23T05:59:39Z

ios/RCTMapboxGL/RCTMapboxGLManager.m

+            if (!styleLayerIdentifiers) {
+                features = [mapView visibleFeaturesAtPoint:point];
+            } else {
+                features = [mapView visibleFeaturesAtPoint:point inStyleLayersWithIdentifiers:styleLayerIdentifiers];


You can call this method for both cases. Pass in nil as the second parameter to get the same behavior as -visibleFeaturesAtPoint:.

1ec5 · 2016-09-23T06:03:48Z

ios/RCTMapboxGL/RCTMapboxGLManager.m

+            }
+            NSNumber *screenCoordX = pointDict[@"screenCoordX"];
+            NSNumber *screenCoordY = pointDict[@"screenCoordY"];
+            NSSet<NSString *> *styleLayerIdentifiers = options[@"layers"];


Wouldn’t this be an NSArray rather than an NSSet, given the JavaScript representation? If this works, it’s only because the SDK happens to be calling methods common to both classes. Probably better to convert it into an NSArray using allObjects.

You're right. I should be pulling it out of options as an NSArray, then convert it to an NSSet. I just pushed some commits with that change.

Probably better to convert it into an NSArray using allObjects.

You mean convert the NSArray into an NSSet as expected by visibleFeaturesAtPoint? Let me know if my most recent change to it looks good. I really appreciate your feedback!

…ng Android support

tsemerad · 2016-09-25T18:32:34Z

I think the PR is ready, at least on iOS. I've made all of the requested changes, got queryRenderedFeatures working with both point and rect input, convert the MGLFeatures into the 7 various GeoJSON feature types, and updated the API docs. Let me know what you think!

For Android, I've started looking into implementing it. Since queryRenderedFeatures is available in the latest Android SDK beta, I'm assuming we want to wait until it lands in a production release first, right? I can make a separate PR then.

1ec5

This is looking pretty good. A couple code style issues and a missing case.

1ec5 · 2016-09-26T15:26:07Z

ios/RCTMapboxGL/RCTMapboxGLManager.m

+              @"coordinates": coordinates };
+}
+
+- (NSMutableArray *)getMGLPolylineCoordinates:(MGLPolylineFeature *)polyline


An Objective-C method name should only begin with "get" if the method fetches a resource from the network or something of that nature.

A cleaner approach would be to add a category to MGLPolylineFeature with a method called -coordinateArray.

Thanks. I've created a category for MGLPolylineFeature and MGLPolygonFeature, each in their own files. Do you have a preferred naming convention? Should I call it MGLPolylineFeature+coordinateArray.m, or maybe MGLPolylineFeature+RCTMapboxGL.m?

How about: MGL{Polyline,Polygon}Feature+RCTAdditions.{h,m}

1ec5 · 2016-09-26T15:32:22Z

ios/RCTMapboxGL/RCTMapboxGLManager.m

+
+    if ([feature isKindOfClass:[MGLPointFeature class]]) {
+        geometryType = @"Point";
+        coordinates = (NSMutableArray *) @[@(feature.coordinate.longitude), @(feature.coordinate.latitude)];


It isn't causing any problems now, but it's unsafe to cast this NSArray to an NSMutableArray. You could declare coordinates as an NSArray without initializing it, then initialize a local NSMutableArray in each if block below where you need one. Not quite as concise, but also reduces the risk of a crash when someone later tries to treat the mutable array as an actual mutable array down below.

How about not creating an NSArray in the first place? I'm thinking replacing line 680 with the following:

coordinates = [[NSMutableArray alloc] initWithArray:@[@(feature.coordinate.longitude), @(feature.coordinate.latitude)]];

That works too. (@[] creates an NSArray, after all.)

1ec5 · 2016-09-26T15:33:54Z

ios/RCTMapboxGL/RCTMapboxGLManager.m

+            CLLocationCoordinate2D coord = multiPoint.coordinates[index];
+            [coordinates addObject:@[@(coord.longitude), @(coord.latitude)]];
+        }
+    }


What about MGLShapeCollectionFeature (FeatureCollection)?

Yikes, never mind, I missed that it's right up there at the top.

1ec5 · 2016-09-26T15:34:54Z

ios/RCTMapboxGL/RCTMapboxGLManager.m

+    [coordinates addObject:outerRingCoordinates];
+
+    for (MGLPolygonFeature *interiorRing in polygon.interiorPolygons) {
+        NSMutableArray *interiorRingCoordinates = [[NSMutableArray alloc] init];


This is fine, although you could also call the current method recursively here.

Wouldn't calling it recursively nest the arrays too much?

[outerRing, interiorRing1, interiorRing2, ...]

vs

[outerRing, [interiorRing1], [interiorRing2], ...]

I've left it as is in my refactoring to using a category.

…categories

tsemerad · 2016-09-26T22:56:24Z

I pushed a new commit addressing your concerns. I put the category on MGLPolyline and MGLPolygon, rather than their "Feature" counterparts, since MGLMultiPolyline's polylines attribute (and MGLMultiPolygon's polygons attribute) return them. I had overlooked that originally. Thanks again for the great feedback.

1ec5

This looks 👌. There are some additions opportunities for refactoring, for example moving all the GeoJSON conversion logic into more MGLFeature categories, but that can always happen down the line.

Thanks again for taking up this task. @bsudekum, good to merge?

lyzidiamond · 2016-09-27T00:02:58Z

HI TANNER!!!!!

1ec5 · 2016-09-27T00:43:19Z

Only thing left to do is to resolve the conflicts.

bsudekum · 2016-09-27T15:14:42Z

@tsemerad want to fix the conflicts? then we can merge

bsudekum · 2016-09-27T15:15:04Z

heh, didn't reload the page, 🎉

tsemerad · 2016-09-27T17:21:23Z

Glad I could help. Haha, hi @lyzidiamond! Good to hear from you! You probably didn't expect to see my name pop up in your Github feed.

1ec5 · 2017-03-22T14:57:21Z

mapbox/mapbox-gl-native#5623 (or the SDK-level wrapper around it) would ultimately expose a way to convert an MGLFeature to GeoJSON directly.

We ultimately implemented this in the iOS SDK as -[MGLShape geoJSONDataUsingEncoding:]. #8493 will add a similar API to the Android SDK.

WIP. Attempt at querying features, getting 'unrecognized selector sen…

d92b823

…t to instance'

1ec5 added the ios label Sep 22, 2016

queryRenderedFeatures working with screen point, returning GeoJSON po…

15d2dd4

…ints, polylines, and polygons

1ec5 suggested changes Sep 23, 2016

View reviewed changes

tsemerad added 9 commits September 23, 2016 11:59

properly grab 'layers' as NSArray, then convert to NSSet if not nil

0c05d43

fixed polygon to add interior rings

0ea8fec

added MGLMultiPointFeature check

668000e

now supporting all GeoJSON geometry types

28d192a

added 'id' attribute to GeoJSON

d1f7a1e

returning a Promise rather than using a callback

00ab190

now accept rect for query

cd1a26f

queryRenderedFeatures now optionally accepts callback, warns of missi…

a580abe

…ng Android support

updated API with queryRenderedFeatures on iOS

2bfa635

tsemerad changed the title ~~Adding queryRenderedFeatures, need feedback~~ Adding queryRenderedFeatures on iOS Sep 25, 2016

1ec5 suggested changes Sep 26, 2016

View reviewed changes

1ec5 mentioned this pull request Sep 26, 2016

MGLGeoJSONSource should have a features initializer mapbox/mapbox-gl-native#6302

Closed

tsemerad force-pushed the queryRenderedFeatures branch from 89617c3 to 97f2bb3 Compare September 26, 2016 22:46

refactored getting polygon and polyline coordinate arrays into 2 new …

d4cf5d5

…categories

tsemerad force-pushed the queryRenderedFeatures branch from 97f2bb3 to d4cf5d5 Compare September 26, 2016 22:52

1ec5 approved these changes Sep 27, 2016

View reviewed changes

bsudekum approved these changes Sep 27, 2016

View reviewed changes

1ec5 merged commit d4cf5d5 into nitaliano:master Sep 27, 2016

tsemerad mentioned this pull request Mar 18, 2017

Adding queryRenderedFeatures to Android #519

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding queryRenderedFeatures on iOS #419

Adding queryRenderedFeatures on iOS #419

tsemerad commented Sep 21, 2016

1ec5 commented Sep 21, 2016

tsemerad commented Sep 21, 2016

tsemerad commented Sep 22, 2016

tsemerad commented Sep 23, 2016

1ec5 commented Sep 23, 2016

1ec5 Sep 23, 2016

1ec5 Sep 23, 2016

1ec5 Sep 23, 2016

1ec5 Sep 23, 2016

1ec5 Sep 23, 2016

1ec5 Sep 23, 2016

1ec5 Sep 23, 2016

1ec5 Sep 23, 2016

tsemerad Sep 24, 2016

tsemerad commented Sep 25, 2016

1ec5 left a comment

1ec5 Sep 26, 2016

tsemerad Sep 26, 2016

1ec5 Sep 26, 2016

1ec5 Sep 26, 2016

tsemerad Sep 26, 2016

1ec5 Sep 26, 2016

1ec5 Sep 26, 2016

1ec5 Sep 26, 2016

1ec5 Sep 26, 2016

tsemerad Sep 26, 2016

tsemerad commented Sep 26, 2016

1ec5 left a comment

lyzidiamond commented Sep 27, 2016

1ec5 commented Sep 27, 2016

bsudekum commented Sep 27, 2016

bsudekum commented Sep 27, 2016

tsemerad commented Sep 27, 2016

1ec5 commented Mar 22, 2017

Adding queryRenderedFeatures on iOS #419

Adding queryRenderedFeatures on iOS #419

Conversation

tsemerad commented Sep 21, 2016

1ec5 commented Sep 21, 2016

tsemerad commented Sep 21, 2016

tsemerad commented Sep 22, 2016

tsemerad commented Sep 23, 2016

1ec5 commented Sep 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsemerad commented Sep 25, 2016

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsemerad commented Sep 26, 2016

1ec5 left a comment

Choose a reason for hiding this comment

lyzidiamond commented Sep 27, 2016

1ec5 commented Sep 27, 2016

bsudekum commented Sep 27, 2016

bsudekum commented Sep 27, 2016

tsemerad commented Sep 27, 2016

1ec5 commented Mar 22, 2017