[core] query source features #8263

ivovandongen · 2017-03-02T23:48:35Z

mention-bot · 2017-03-02T23:48:37Z

@ivovandongen, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @1ec5 and @incanus to be potential reviewers.

jfirebaugh · 2017-03-06T17:30:07Z

include/mbgl/map/map.hpp

@@ -3,6 +3,7 @@
 #include <mbgl/util/optional.hpp>
 #include <mbgl/util/chrono.hpp>
 #include <mbgl/map/mode.hpp>
+#include <mbgl/map/query.hpp>


This is already included below.

jfirebaugh · 2017-03-06T17:38:28Z

include/mbgl/map/query.hpp

+class SourceQueryOptions {
+public:
+ // Required for VectorSource, ignored for GeoJSONSource
+ optional<std::string> sourceLayer;


Let's make this a std::vector<std::string> like RenderedQueryOptions. I don't think there's a good reason that the GL JS API is inconsistent (mapbox/mapbox-gl-js#4372).

jfirebaugh · 2017-03-06T17:41:00Z

include/mbgl/map/map.hpp

 AnnotationIDs queryPointAnnotations(const ScreenBox&);
+ std::vector<Feature> querySourceFeatures(const std::string&, const SourceQueryOptions& options = {});


GL native exposes an object-oriented API for sources -- move this method to mbgl::style::Source, rename it to queryFeatures, and drop the first parameter.

@jfirebaugh This does mean that you have to retrieve the source in the SDKs before querying. Small overhead, but still. Is this what we want?

@jfirebaugh In this case; shall I move SourceQueryOptions to a separate header in the mbgl::style namespace? or keep it here?

This does mean that you have to retrieve the source in the SDKs before querying. Small overhead, but still. Is this what we want?

Yes, I think so -- by eliminating the map middleman in map.querySourceFeatures(source), source.queryFeatures() becomes conceptually simpler and possibly actually more efficient in the case of repeated queries (though I would indeed expect the benefit to be negligible).

shall I move SourceQueryOptions to a separate header in the mbgl::style namespace?

Yeah, I think that makes sense.

1ec5 · 2017-03-07T00:44:57Z

This does mean that you have to retrieve the source in the SDKs before querying. Small overhead, but still. Is this what we want?

I've been thinking about making the same change to the iOS/macOS SDKs' public API for queryRenderedFeatures: accept an array of MGLStyleLayers instead of an array of identifiers in an awkwardly named parameter. The overhead is negligible for moderately complex styles. macosapp, for instance, retrieves and wraps every style layer on load, and any performance impact seems reasonable.

1ec5

Sweet – this will unblock plenty of use cases, plus some improvements I've been meaning to make around VoiceOver accessibility support.

In addition to the comments below, please add an entry to the iOS and macOS changelogs. Thanks!

1ec5 · 2017-03-07T00:52:25Z

platform/ios/src/MGLMapView.h

+ @return An array of objects conforming to the `MGLFeature` protocol that
+ represent features in the sources used by the current style.
+ */
+- (NS_ARRAY_OF(id <MGLFeature>) *)featuresWithSourceIdentifier:(NSString *)sourceIdentifier sourceLayerIdentifier:(nullable NSString *)sourceLayerIdentifier predicate:(nullable NSPredicate *)predicate NS_SWIFT_NAME(features(sourceIdentifier:sourceLayerIdentifier:predicate:));


How about -featuresLoadedInSource:sourceLayerIdentifier:predicate:?

Especially if this method only works with a single source at a time, I think it would be desirable to require the developer to pass in an MGLSource object instead of a source identifier. That encourages the developer to obtain an existing source and helps to clarify the distinction between a source identifier and a source layer identifier.

Additionally, we probably want to emphasize that this method returns the features that have already loaded, not all the possible features in the source (which for a vector source could be quite unwieldy). This isn't quite a reverse geocoding API, after all.

-featuresLoadedBySource:sourceLayerIdentifier:predicate: would avoid overusing "in", especially if we later add a bounding box option.

Swift can probably come up with a decent name for this method without the NS_SWIFT_NAME attribute. (Try it out locally by trying to call the method in one of our Swift unit test files.) If not, try providing a Swift name of features(loadedBy:sourceLayerIdentifier:predicate:).

We'll want to push the change suggested above (moving the query method to the Source object itself) out to the SDK level, making this -[MGLSource featuresLoaded:inLayerWithIdentifier:predicate:] or similar.

1ec5 · 2017-03-07T00:53:25Z

platform/ios/src/MGLMapView.h

+ Features come from tiled vector data or GeoJSON data that is converted to tiles
+ internally, so feature geometries are clipped at tile boundaries and features
+ may appear duplicated across tiles. For example, suppose the specified
+ rectangle intersects with a road that spans the screen. The resulting array


Some of this explanation is specific to -visibleFeaturesInRect:. This method by contrast accepts no rectangle parameter.

1ec5 · 2017-03-08T08:26:24Z

platform/darwin/src/MGLShapeSource.h

+ Features come from tiled vector data or GeoJSON data that is converted to tiles
+ internally, so feature geometries are clipped at tile boundaries and features
+ may appear duplicated across tiles. For example, suppose the specified
+ rectangle intersects with a road that spans the screen. The resulting array


No rectangle is specified as part of this API.

1ec5 · 2017-03-08T08:29:25Z

platform/darwin/src/MGLVectorSource.h

 - (instancetype)initWithIdentifier:(NSString *)identifier configurationURL:(NSURL *)configurationURL NS_DESIGNATED_INITIALIZER;

 - (instancetype)initWithIdentifier:(NSString *)identifier tileURLTemplates:(NS_ARRAY_OF(NSString *) *)tileURLTemplates options:(nullable NS_DICTIONARY_OF(MGLTileSourceOption, id) *)options NS_DESIGNATED_INITIALIZER;

+#pragma mark Querying the source's features


Since jazzy uses marks to generate section headings, it’s important to keep them consistent (title case, describes a task, uses the most commonplace terms possible). How about “Accessing a Source’s Content”, which is the same mark we use in MGLShapeSource?

1ec5 · 2017-03-08T08:30:59Z

platform/darwin/src/MGLShapeSource.h

@@ -210,6 +210,31 @@ MGL_EXPORT
 */
 @property (nonatomic, copy, nullable) NSURL *URL;

+#pragma mark Querying the source's features


I think -featuresMatchingPredicate: falls neatly under the existing “Accessing a Source’s Content” section heading.

1ec5 · 2017-03-08T08:33:55Z

platform/darwin/src/MGLShapeSource.h

+ @return An array of objects conforming to the `MGLFeature` protocol that
+ represent features in the sources used by the current style.
+ */
+- (NS_ARRAY_OF(id <MGLFeature>) *)featuresWithPredicate:(nullable NSPredicate *)predicate NS_SWIFT_NAME(features(predicate:));


Call this method -featuresMatchingPredicate: and remove the NS_SWIFT_NAME attribute. Swift will automatically bridge it as features(matching:).

The convention in Swift 3 is for argument labels to be prepositions or participles like “matching” instead of prepositional objects like “predicate”. matching: would be more intuitive argument label than with: in this case. (It’d also be consistent with methods like -[CNContactStore groupsMatchingPredicate:error:], -[EKEventStore eventsMatchingPredicate:], and -[XCUIElementQuery matchingPredicate:] in standard Apple libraries.)

1ec5 · 2017-03-08T08:35:57Z

platform/darwin/src/MGLShapeSource.h

+ current style and provides access to attributes specified by the relevant
+ <a href="https://www.mapbox.com/mapbox-gl-style-spec/#sources">tile sources</a>.
+ The returned array includes features specified in vector and GeoJSON tile
+ sources but does not include anything from raster, image, or video sources.


Now that this method lives on MGLShapeSource, it’s unnecessary to point out that it won’t return features from non-shape sources. It’s also important to note that this method may return features that exist in the source but are not currently drawn by a style layer.

1ec5 · 2017-03-08T08:40:01Z

platform/darwin/src/MGLVectorSource.h

+ current style and provides access to attributes specified by the relevant
+ <a href="https://www.mapbox.com/mapbox-gl-style-spec/#sources">tile sources</a>.
+ The returned array includes features specified in vector and GeoJSON tile
+ sources but does not include anything from raster, image, or video sources.


Similarly, there’s no need here to talk about other sources.

1ec5 · 2017-03-08T08:40:31Z

platform/darwin/src/MGLVectorSource.h

+ the road within each map tile is included individually.
+
+ @param sourceLayerIdentifiers The source layers to include in the query. Only the
+ features contained in this source layers are included in the returned array. At


s/this source layers/these source layers/

1ec5 · 2017-03-08T08:41:15Z

platform/darwin/src/MGLVectorSource.h

+ least one source layer is required.
+ @param predicate A predicate to filter the returned features.
+ @return An array of objects conforming to the `MGLFeature` protocol that 
+ represent features in the sources used by the current style.


Is there any significance to the order in which the features are returned? (It’s fine if there isn’t; we may even want to leave the order unmentioned as wiggle room for future improvements.)

No, not any order in particular and I don't think we should make any guarantees for now. Right now the order is sourceLayer, feature index.

1ec5 · 2017-03-08T08:43:36Z

platform/darwin/src/MGLVectorSource.h

+#pragma mark Querying the source's features
+
+/**
+ Returns an array of map features for this source, restricted to the given


s/for this source/loaded by this source/

1ec5 · 2017-03-08T08:49:14Z

platform/darwin/test/MGLSourceQueryTests.m

+ XCTAssertNotNil(mapView.style);
+ XCTAssertEqual(mapView.style, style);
+
+ [_styleLoadingExpectation fulfill];


In case you’re wondering why this works for shape sources but not vector sources, -mapView:didFinishLoadingStyle: gets called as soon as we’re done loading and parsing the style itself. It isn’t guaranteed that all sources load by this point. If you wanted to query a vector source too, you’d need to wait until -mapViewDidFinishLoadingMap: is called. But this is perfectly fine for the inline GeoJSON sources in query-style.json, since they load as part of the style.

If you wanted to query a vector source too, you’d need to wait until -mapViewDidFinishLoadingMap: is called

I've tried this, but this callback is never fired in this setup.

this is perfectly fine for the inline GeoJSON sources in query-style.json, since they load as part of the style.

I expected the same, but the inline geojson sources also don't seem to load this way.

ivovandongen · 2017-03-08T23:33:43Z

@jfirebaugh Made the changes you requested and adapted the SDK bindings. Adding support for multiple sourceLayers resulted in some code duplication in: GeoJSONTile:: querySourceFeatures. Could extract the common functionality, not sure if it's worth it.

tobrun

Tested the Android sample and getting 4 features when clicking the map. Android side of things looks good to 🚢

1ec5

A couple documentation nits, but the iOS/macOS changes are 👌 otherwise.

1ec5 · 2017-03-09T01:30:30Z

platform/darwin/src/MGLShapeSource.h

+
+ Each object in the returned array represents a feature for the
+ current style and provides access to attributes specified by the
+ <a href="https://www.mapbox.com/mapbox-gl-style-spec/#sources">source</a>.


There’s also no need to link “source”, since this method lives inside an MGLSource subclass.

1ec5 · 2017-03-09T01:31:38Z

platform/darwin/src/MGLVectorSource.h

+#pragma mark Accessing a Source’s Content
+
+/**
+ Returns an array of map features for this source, restricted to the given


Reminder: s/for this source/loaded by this source/

1ec5 · 2017-03-09T01:32:12Z

platform/darwin/src/MGLVectorSource.h

+ may appear duplicated across tiles.
+
+ @param sourceLayerIdentifiers The source layers to include in the query. Only the
+ features contained in this source layers are included in the returned array. At


Reminder: s/this source layers/these source layers/

ivovandongen added Core The cross-platform C++ core, aka mbgl feature ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Mar 2, 2017

ivovandongen self-assigned this Mar 2, 2017

ivovandongen force-pushed the 5792-query-source-features branch 3 times, most recently from bc88add to d3616d2 Compare March 5, 2017 03:39

jfirebaugh suggested changes Mar 6, 2017

View reviewed changes

ivovandongen force-pushed the 5792-query-source-features branch from fa9c52e to 46c655e Compare March 6, 2017 19:57

1ec5 reviewed Mar 7, 2017

View reviewed changes

ivovandongen force-pushed the 5792-query-source-features branch 2 times, most recently from 2b1e017 to 3f742a9 Compare March 8, 2017 06:03

1ec5 reviewed Mar 8, 2017

View reviewed changes

ivovandongen force-pushed the 5792-query-source-features branch 2 times, most recently from 4c1aadc to 4219e26 Compare March 8, 2017 23:29

ivovandongen added Android Mapbox Maps SDK for Android iOS Mapbox Maps SDK for iOS labels Mar 8, 2017

ivovandongen requested a review from tobrun March 8, 2017 23:29

ivovandongen force-pushed the 5792-query-source-features branch from 4219e26 to 5e8da5e Compare March 8, 2017 23:40

tobrun approved these changes Mar 8, 2017

View reviewed changes

1ec5 approved these changes Mar 9, 2017

View reviewed changes

jfirebaugh approved these changes Mar 9, 2017

View reviewed changes

ivovandongen added 3 commits March 9, 2017 11:44

[core] rename query options for query rendered features

388e628

[core] query source features

23ea254

[macos, ios] query source features

103fbd0

ivovandongen force-pushed the 5792-query-source-features branch from 5e8da5e to 3f72b72 Compare March 9, 2017 19:44

[android] query source features

42b76cd

ivovandongen force-pushed the 5792-query-source-features branch from 3f72b72 to 42b76cd Compare March 9, 2017 20:22

ivovandongen removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Mar 9, 2017

ivovandongen merged commit b2b5f9e into master Mar 9, 2017

ivovandongen deleted the 5792-query-source-features branch March 9, 2017 21:11

jradasaurus mentioned this pull request May 13, 2020

iOS missing querySourceFeatures #16467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] query source features #8263

[core] query source features #8263

ivovandongen commented Mar 2, 2017

mention-bot commented Mar 2, 2017

jfirebaugh Mar 6, 2017

jfirebaugh Mar 6, 2017

jfirebaugh Mar 6, 2017 •

edited

Loading

ivovandongen Mar 6, 2017

ivovandongen Mar 6, 2017

jfirebaugh Mar 7, 2017

1ec5 commented Mar 7, 2017

1ec5 left a comment

1ec5 Mar 7, 2017

1ec5 Mar 7, 2017

jfirebaugh Mar 7, 2017

1ec5 Mar 7, 2017

1ec5 Mar 8, 2017

1ec5 Mar 8, 2017

1ec5 Mar 8, 2017

1ec5 Mar 8, 2017

1ec5 Mar 8, 2017

1ec5 Mar 8, 2017

1ec5 Mar 8, 2017

1ec5 Mar 8, 2017

ivovandongen Mar 8, 2017

1ec5 Mar 8, 2017

1ec5 Mar 8, 2017

ivovandongen Mar 8, 2017

ivovandongen commented Mar 8, 2017

tobrun left a comment

1ec5 left a comment

1ec5 Mar 9, 2017

1ec5 Mar 9, 2017

1ec5 Mar 9, 2017

		AnnotationIDs queryPointAnnotations(const ScreenBox&);
		std::vector<Feature> querySourceFeatures(const std::string&, const SourceQueryOptions& options = {});

[core] query source features #8263

[core] query source features #8263

Conversation

ivovandongen commented Mar 2, 2017

mention-bot commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh Mar 6, 2017 • edited Loading

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

1ec5 commented Mar 7, 2017

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

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

ivovandongen commented Mar 8, 2017

tobrun left a comment

Choose a reason for hiding this comment

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

jfirebaugh Mar 6, 2017 •

edited

Loading