Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Feature querying; more geometry types #5110

Merged
merged 7 commits into from
May 28, 2016
Merged

Feature querying; more geometry types #5110

merged 7 commits into from
May 28, 2016

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented May 23, 2016

Added methods that return the visible features at a given point or in a given rectangle on the map. Each feature is represented by a class conforming to the MGLFeature protocol and, conveniently enough, the MGLAnnotation protocol. Yup, that means you can feed the features right back to the map in the form of point and shape annotations. These classes derive from the usual suspects (MGLPointAnnotation, MGLPolyline, etc.), but there are several new classes to represent geometry types supported by geometry.hpp but not by MapKit (MGLFeatureCollection, MGLMultiPolyline, etc.). Complex geometry types like multipolygon cannot yet be added as overlays, but this PR does add support for holes in simple polygons.

In addition to the usual annotation properties, each feature object has an identifier and a dictionary of attributes (properties) that come from the source. The dictionary contains boxed Foundation types such as NSString and NSNumber, as appropriate. Identifiers are also represented as NSNumbers: mbgl allows features to have IDs within the entire uint64_t space and the Mapbox GL Vector Tile Spec doesn’t specify the ID data type, so there’s no room for magic numbers.

In iosapp and osxapp, dropping a pin (by long-pressing or via the context menu) shows the usual callout view, but now its title is the name of a feature under the cursor, if available, rather than “Dropped Pin”. Choose “Select Features” from the context menu to highlight all the features under the cursor.

scpa
downtown
jungle-jims

More to come:

  • Convert mbgl::geometry::values to Cocoa types
  • Specialize SDK shape classes for feature querying results
  • Write SDK types for multipolylines, multipolygons, feature collections
  • Convert mbgl::Features to MGLFeatures
  • Query rendered features at an NSPoint on OS X
  • Support all feature shape classes in the annotation layer
  • Move SDK feature types into their own files
  • Make all SDK feature types inherit from MGLShape
  • Move feature Cocoa-ization into factory method on MGLShape in darwin/
  • Add unit tests for feature Cocoa-ization
  • Query rendered features at a CGPoint on iOS
  • Add a way to filter by layer
  • Support interior rings (holes) in polygons (fixes the iOS/OS X side of add support for multipolygons and polygon interior holes #1729)
  • Migrate mbgl::AnnotationManager to mbgl::geometry types? (split out into mbgl::AnnotationManager should use mapbox::geometry types #5158)
  • Fix crash querying features in the presence of mbgl::AnnotationManager-managed features
  • Query rendered features within NSRect/CGRect

Fixes the iOS/OS X side of #352.

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold annotations Annotations on iOS and macOS or markers on Android labels May 23, 2016
@1ec5 1ec5 added this to the ios-v3.3.0 milestone May 23, 2016
@1ec5 1ec5 self-assigned this May 23, 2016
@jfirebaugh
Copy link
Contributor

the Mapbox GL Vector Tile Spec doesn’t specify the ID data type

It specifies them as uint64. However, I might switch the mbgl core type to mbgl::Value, to accommodate all possible GeoJSON "id" values.

Dropping a pin via the menu or context menu item shows the usual dropped pin, but the pin’s title is now the name of a feature under the cursor, if available

Nice!

@jfirebaugh
Copy link
Contributor

👍 These two would go well together.

@1ec5
Copy link
Contributor Author

1ec5 commented May 23, 2016

the Mapbox GL Vector Tile Spec doesn’t specify the ID data type

It specifies them as uint64. However, I might switch the mbgl core type to mbgl::Value, to accommodate all possible GeoJSON "id" values.

Ah, I mistakenly thought the prose readme was the canonical specification. 👍 With mbgl::Value as the underlying id type, I think the most appropriate type for MGLFeature.tag would be id, confusingly enough!

@1ec5
Copy link
Contributor Author

1ec5 commented May 25, 2016

I need to investigate a crash querying features in a tile that also has annotations. It has to do with the indexed feature having no name but the annotation layers having names like com.mapbox.annotations.points:

#0  0x0000000100170774 in mbgl::AnnotationTile::getLayer(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) const at /path/to/mapbox-gl-native-2/src/mbgl/annotation/annotation_tile.cpp:29
#1  0x000000010018f2c6 in mbgl::FeatureIndex::addFeature(std::__1::unordered_map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::vector<mbgl::Feature, std::__1::allocator<mbgl::Feature> >, std::__1::hash<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::equal_to<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const, std::__1::vector<mbgl::Feature, std::__1::allocator<mbgl::Feature> > > > >&, mbgl::IndexedSubfeature const&, mbgl::GeometryCollection const&, std::experimental::fundamentals_v1::optional<std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > > const&, mbgl::GeometryTile const&, mbgl::CanonicalTileID const&, mbgl::Style const&, float, float) const at /path/to/mapbox-gl-native-2/src/mbgl/geometry/feature_index.cpp:105
#2  0x000000010018f0b2 in mbgl::FeatureIndex::query(std::__1::unordered_map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::vector<mbgl::Feature, std::__1::allocator<mbgl::Feature> >, std::__1::hash<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::equal_to<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const, std::__1::vector<mbgl::Feature, std::__1::allocator<mbgl::Feature> > > > >&, mbgl::GeometryCollection const&, float, double, double, std::experimental::fundamentals_v1::optional<std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > > const&, mbgl::GeometryTile const&, mbgl::CanonicalTileID const&, mbgl::Style const&) const at /path/to/mapbox-gl-native-2/src/mbgl/geometry/feature_index.cpp:85
#3  0x000000010044ce0d in mbgl::VectorTileData::queryRenderedFeatures(std::__1::unordered_map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::vector<mbgl::Feature, std::__1::allocator<mbgl::Feature> >, std::__1::hash<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::equal_to<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const, std::__1::vector<mbgl::Feature, std::__1::allocator<mbgl::Feature> > > > >&, mbgl::GeometryCoordinates const&, mbgl::TransformState const&, std::experimental::fundamentals_v1::optional<std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > > const&) at /path/to/mapbox-gl-native-2/src/mbgl/tile/vector_tile_data.cpp:202
#4  0x000000010030e9d7 in mbgl::Source::queryRenderedFeatures(mbgl::StyleQueryParameters const&) const at /path/to/mapbox-gl-native-2/src/mbgl/source/source.cpp:367
#5  0x000000010036bcea in mbgl::Style::queryRenderedFeatures(mbgl::StyleQueryParameters const&) const at /path/to/mapbox-gl-native-2/src/mbgl/style/style.cpp:324
#6  0x00000001002511d0 in mbgl::Map::queryRenderedFeatures(mapbox::geometry::point<double> const&, std::experimental::fundamentals_v1::optional<std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > > const&) at /path/to/mapbox-gl-native-2/src/mbgl/map/map.cpp:739
#7  0x0000000100069e75 in -[MGLMapView visibleFeaturesAtPoint:inStyleLayersWithIdentifiers:] at /path/to/mapbox-gl-native-2/platform/osx/src/MGLMapView.mm:2280
#8  0x0000000100069a03 in -[MGLMapView visibleFeaturesAtPoint:] at /path/to/mapbox-gl-native-2/platform/osx/src/MGLMapView.mm:2263
#9  0x00000001000065a8 in -[MapDocument handlePressGesture:] at /path/to/mapbox-gl-native-2/platform/osx/app/MapDocument.m:411

@1ec5
Copy link
Contributor Author

1ec5 commented May 26, 2016

Split out #5159 for the crash querying annotation tiles.

@1ec5 1ec5 changed the title [WIP] Feature querying; more geometry types Feature querying; more geometry types May 27, 2016
@1ec5 1ec5 force-pushed the 1ec5-features-352 branch 2 times, most recently from aa1c9a1 to 354396a Compare May 27, 2016 07:04
@1ec5 1ec5 added ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels May 27, 2016
@1ec5
Copy link
Contributor Author

1ec5 commented May 27, 2016

@jfirebaugh, would you mind reviewing the Objective-C++ changes in MGLFeature.mm, MGLFeatureTests.mm, and MGLMapView.mm, plus the C++ changes in shape_annotation_impl.cpp? Thanks!

@boundsj, would you mind reviewing the rest of the geometry-related changes in platform/darwin/? Thanks!

*/
class PropertyValueEvaluator {
public:
PropertyValueEvaluator() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This default constructor can be removed; the compiler will generate it automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the vestigial constructor and force-pushed.

@1ec5
Copy link
Contributor Author

1ec5 commented May 27, 2016

Here’s the new feature’s documentation:

mglmapview

mglfeature

@jfirebaugh
Copy link
Contributor

How would the API change if we added the the filter option supported by gl-js? Or other options? Is it extensible without breaking backward compatibility?

@1ec5
Copy link
Contributor Author

1ec5 commented May 27, 2016

How would the API change if we added the the filter option supported by gl-js? Or other options? Is it extensible without breaking backward compatibility?

We’d add additional variations of this method, like -visibleFeaturesInRect:inStyleLayersWithIdentifiers:filteredWithPredicates: (or something to that effect). That won’t scale too well if we have more than 3–4 options, but there aren’t any good alternatives in Objective-C.

The typical solution in C++ would be to pass in a struct representing the query, but structs in Objective-C (really, C structs) don’t support ARC and thus can’t hold Objective-C objects like NSString. (This is fixed in Swift, but without any backwards compatibility for Objective-C.) We went down this road in the past with -setCenterCoordinate:… and umpteen variations before we introduced -setCamera: to keep things tidy. But MGLMapCamera is a full-fledged class, with all the cognitive and syntactic overhead that entails.

I don’t think we should introduce a class with only one property. If lots more options become available down the road, or if MGLStyleLayer becomes a type the developer can interact with, we can do the deprecation dance with these methods at that point.

@1ec5
Copy link
Contributor Author

1ec5 commented May 27, 2016

I’ve tweaked the Swift names of these MGLMapView methods to be briefer and accommodate additional options in the future. For example, visibleFeaturesAtPoint(_:inStyleLayersWithIdentifiers:) becomes visibleFeatures(at:styleLayerIdentifiers:).

Hopefully it won’t be too confusing for the Swift names to be slightly different than the Objective-C names, now that realm/jazzy#136 has made it into our build system. Eventually, we can also refine the declarations for Swift, marking each labeled parameter as optional so you can specify any permutation of these parameters.


The keys and values of this dictionary are determined by the vector tile
source. Vector tiles are stored in <a href="http://geojson.org/">GeoJSON</a>
format; they additionally conform to the
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean to store a vector tile in GeoJSON format?

Vector tiles are stored in GeoJSON format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that’s incorrect and it’s the next part, about the vector tile spec, that we should be emphasizing anyways.

@1ec5 1ec5 mentioned this pull request May 27, 2016
On the other hand, if the style does include bus stops, an `MGLFeature` object
representing that bus stop is returned and its `featureAttributes` dictionary
has the `maki` key set to `bus` (along with other attributes). The dictionary
does not indicate how the feature is rendered by the current style.
Copy link
Contributor

Choose a reason for hiding this comment

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

This explanation is good and correct. As a thought exercise, I wonder what it'd look like to frame this description more in terms of what the API does rather than what it doesn't do.

This API returns the set of features that are rendered on the map at a point.

Copy link
Contributor Author

@1ec5 1ec5 May 27, 2016

Choose a reason for hiding this comment

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

I think all but the last sentence in this paragraph explains what the API does do – or at least, that’s what I was trying to do. The point I was trying to make at the end is that you shouldn’t expect paint properties to be included in the attribute dictionary.

1ec5 added 7 commits May 27, 2016 21:42
Added methods to MGLMapView that return the rendered features at a visible point or within a visible rectangle on the map, optionally restricted to a set of layers, plus voluminous documentation.

Added several new geometry classes corresponding to distinct geometry types supported by geometry.hpp. Added parallel “feature” classes to represent these geometries along with tags (IDs) and attributes (properties) from the source.

Grouped classes in the Foundation and SDK groups by theme.

In iosapp, dropped pins’ callout views now display the name of the topmost named feature at that point.

In osxapp, a long press on the map highlights the features under the cursor. Dropping a pin via the menu or context menu item shows the usual dropped pin, but the pin’s title is now the name of a feature under the cursor, if available, rather than “Dropped Pin”.

Fixes the iOS/OS X side of #352.
Made writability of MGLFeature properties private, to avoid having to awkwardly explain that changes to the properties don’t change the map (maybe someday they will). This allows the identifier and attributes to be set in only one place, avoiding copy-pasta.
MGLPolygon (and by extension MGLMultiPolygon) now supports interior rings. The data is preserved in feature querying results, and interior rings are respected when adding polygon overlays to the map.

Fixes #1729.

[ios, osx] Updated changelog
Removed a level of indirection when creating Objective-C collections from C++ collections. Adopted mbgl typedefs for geometry.hpp types. Factored out MGLPolygon construction.
Ignore any multipolyline, multipolygon, or shape collection object passed into -addAnnotation: or -addAnnotations:. Previously, these methods broke apart the compound shape into its constituent shapes in order to recursively add them to the map. But that broke assumptions about a one-to-one correspondence between annotations and their contexts during selection and deletion.
Moved feature selection to the context menu and restored the press gesture for dropping a pin. Bust complex features into simple shapes to feed into MGLMapView as overlays.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
annotations Annotations on iOS and macOS or markers on Android iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants