Allow initializing MGLGeoJSONSource from a shape by frederoni · Pull Request #7145 · mapbox/mapbox-gl-native

frederoni · 2016-11-22T15:07:52Z

First stab at #6986

I'm slightly hesitant about replacing -[MGLGeoJSONSource initWithIdentifier:features:options:] with feature because of the additional initialization of MGLShapeCollectionFeature devs has to do now compared to a concise array literal prior to this change.

@1ec5 👀

mention-bot · 2016-11-22T15:07:55Z

@frederoni, thanks for your PR! By analyzing the history of the files in this pull request, we identified @boundsj, @ituaijagbone and @1ec5 to be potential reviewers.

1ec5 · 2016-11-23T09:27:57Z

I share you hesitation about removing -[MGLGeoJSONSource initWithIdentifier:features:options:]. -[MGLShapeCollectionFeature shapeCollectionWithShapes:] is kind of unwieldy. Should we ease the pain by keeping -[MGLGeoJSONSource initWithIdentifier:features:options:] as yet another initializer on MGLGeoJSONSource?

1ec5 · 2016-11-23T09:31:14Z

+        id serialized = [NSJSONSerialization JSONObjectWithData:self.geoJSONData options:0 error:nil];
+        BOOL isFeatureCollection = [serialized isKindOfClass:NSArray.class] || ([serialized isKindOfClass:NSDictionary.class] && [[(NSDictionary *)serialized objectForKey:@"type"] isEqualToString:@"FeatureCollection"]);
+
+        if (isFeatureCollection) {


Why does this conditional exist? mbgl::style::Source::setGeoJSON() takes a mapbox::geojson::geojson, which is the type returned by mapbox::geojson::parse(), so no get() is needed for either case. _feature is of type id <MGLFeature>, so MGLFeatureFromMBGLFeature() works for both cases too.

1ec5 · 2016-11-23T09:37:30Z

-        for (id <MGLFeaturePrivate> feature in self.features) {
-            featureCollection.push_back([feature mbglFeature]);
+        // Feature collection
+        if ([self.feature isMemberOfClass:MGLShapeCollectionFeature.class]) {


MGLShapeCollectionFeature implements -mbglFeature, so this case is also unnecessary.

frederoni · 2016-11-24T16:13:59Z

Removed debug code and cleaned up.
Todo:

Support initializing MGLGeoJSONSource with a geometry?
-[MGLGeoJSONSourceTests testMGLGeoJSONSourceWithPolygonFeatures] is failing unexpectedly.
features are not set if MGLGeoJSONSource is initialized with data.

frederoni · 2016-11-28T14:35:57Z

We can't assign features here if we're gonna support plain geometries or a single feature. Would it make sense to rename features to geometries and write a GeoJSON evaluator which overloads featureCollection, feature and geometry?

We need a shape property that can be any MGLShape (or any MGLFeature). We can have a features property as a convenience for when shape happens to contain an MGLShapeCollectionFeature.

frederoni · 2016-11-28T14:40:10Z

Depending on what we want, I think this is good for another review and possibly also merge.
#6986 doesn't occur anymore but there's a drawback. Using -[MGLGeoJSONSource initWithIdentifier:geoJSONData:options:] will leave the features property unset because it can contain plain geometries.

1ec5 · 2016-11-28T17:50:32Z

Maybe that means it needs to be a shape property (normally holding a shape collection feature), with features being a convenience property based on it?

1ec5 · 2016-11-29T02:59:59Z

+ @param feature A feature that conform to the `MGLFeature` protocol.
+ @param options An `NSDictionary` of options for this source.
+ @return An initialized GeoJSON source.
+ */


Since mbgl::style::GeoJSONSource::setGeoJSON() takes an mbgl::GeoJSON (a variant of geometry, feature, or feature_collection), it would be better for this initializer to take any MGLShape. All MGLShape subclasses in turn have subclasses that conform to MGLFeature, so we wouldn’t be losing any expressiveness that way.

1ec5 · 2016-11-29T03:02:15Z

We need a shape property that can be any MGLShape (or any MGLFeature). We can have a features property as a convenience for when shape happens to contain an MGLShapeCollectionFeature.

1ec5 · 2016-11-30T16:36:57Z

 `-[MGLStyle addSource]`.
 */
-@property (nonatomic, nullable) NS_ARRAY_OF(id <MGLFeature>) *features;
+@property (nonatomic, nullable) MGLShape *shape;


Update the documentation for this property.

1ec5 · 2016-11-30T16:38:32Z

 @property (nonatomic, nullable) NSURL *URL;

+/**
+ Returns all objects from the shape that conforms to `MGLFeature`.


This makes it sound like it does a deep search through the shape collection feature for any shapes, whereas it's only set to the top-level features, and only if a shape collection feature or array of features was passed in on initialization.

1ec5 · 2016-11-30T16:42:16Z

+/**
+ Returns all objects from the shape that conforms to `MGLFeature`.
+ */
+- (NSArray<MGLFeature> *)features;


Use NS_ARRAY_OF() in public APIs for the benefit of developers compiling against the iOS 8 or OS X 10.10 SDKs.

Also, this method should be a property like before.

Finally, the return type should be an array of id <MGLFeature>, not an NSArray conforming to the MGLFeature protocol.

1ec5 · 2016-11-30T16:44:38Z

-    }

-    mbgl::FillAnnotation annotation { geometry };
+    auto polygon = mbgl::Polygon<double> { [self ring] };


What about interior rings?

1ec5 · 2016-12-01T16:26:03Z

+        MGLShapeCollection *collection = (MGLShapeCollection *)_shape;
+        for (MGLShape *shape in collection.shapes) {
+            if ([shape conformsToProtocol:@protocol(MGLFeature)]) {
+                [features addObject:(id <MGLFeature>)shape];


Would it be more efficient to compute the features property as soon as shape is set, or on demand when calling -features? Or perhaps a lazy-loading strategy is called for, similar to how I treated overlayBounds in #7251. Basically, if a features ivar is nil, -features would compute the features in the shape and store the results in that ivar. -initWithIdentifier:features:options: would go ahead and set that ivar to the features that are passed in, so a subsequent call to -features would require no computation.

1ec5 · 2016-12-01T16:30:27Z

    return self;
 }

+- (instancetype)initWithIdentifier:(NSString *)identifier feature:(id<MGLFeature>)feature options:(NSDictionary<MGLGeoJSONSourceOption,id> *)options


Any reason to require a feature instead of any ordinary shape? Looks like we’re only using this parameter for an MGLShape-typed property anyways.

Mostly not to break the API. Should we rename it to -[MGLGeoJSONSource initWithIdentifier:shape:options:] or keep feature: and features: while adding shape: ?

How about -initWithIdentifier:shape:options:, which would be expressive enough to take any MGLShape, including any MGLFeature. We’d want to treat the shape as a feature if possible and fall back to treating it as a non-feature, to avoid losing attribute information.

As for the other initializer, which is merely a convenience, how about -initWithIdentifier:features:options: initializer, as incongruous as it’d be? I don’t think we’d want to encourage developers to intermix non-features and features at the top level of the same source, because then we’d have to cater to the lowest common denominator and treat them all like ordinary shapes. (They can still do so by passing an MGLShapeCollection into -initWithIdentifier:shape:options:.)

I’m starting to think we should omit the features property just to keep things simple for us. The developer can still get the features out of shape as long as they cast shape to an MGLShapeCollectionFeature. Maybe @incanus or @ericrwolfe have opinions here, given that they’ve been working with this API at the application level recently.

1ec5

One question and one minor issue, but otherwise good to go.

1ec5 · 2016-12-02T16:45:18Z

 @return An initialized GeoJSON source.
 */
- (instancetype)initWithIdentifier:(NSString *)identifier features:(NSArray<id<MGLFeature>> *)features options:(nullable NS_DICTIONARY_OF(MGLGeoJSONSourceOption, id) *)options NS_DESIGNATED_INITIALIZER;
+- (instancetype)initWithIdentifier:(NSString *)identifier shape:(nullable MGLShape *)shape options:(nullable NS_DICTIONARY_OF(MGLGeoJSONSourceOption, id) *)options NS_DESIGNATED_INITIALIZER;


What does it mean to create a source from no shapes at all? It does work in the current release branch, using an empty array of features, but we should make sure it works when shape is nil.

nil shape works but I can't think of any case where you'd want to do that.

1ec5 · 2016-12-02T16:45:45Z

    self.rawSource->setGeoJSON(geojson);

-    _features = MGLFeaturesFromMBGLFeatures(featureCollection);
+    [self willChangeValueForKey:@"shape"];


These change notifications are unnecessary. Since setShape: is a standard KVC selector, these change notifications will be generated for you implicitly.

1ec5 · 2016-12-05T04:48:44Z

The Coveralls failure is caused by an unrelated change in test coverage in mbgl. You can get around it by pushing another change, so that coverage will be even.

frederoni · 2016-12-05T11:02:34Z

@1ec5 what was the purpose of this test?

Looks like it's intended to trip an exception in -[MGLShapeCollectionFeature mbglFeature], which is unimplemented in the previous code.

boundsj · 2016-12-05T23:44:22Z

The iOS release branch now has c8e34d5 so if this branch is rebased on top of that, the node tests will pass.

frederoni added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS release blocker Blocks the next final release ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Nov 22, 2016

frederoni added this to the ios-v3.4.0 milestone Nov 22, 2016

frederoni self-assigned this Nov 22, 2016

This was referenced Nov 22, 2016

Computed (on-demand) shape source #6940

Closed

rename MGLGeoJSONSource to MGLFeatureSource #7160

Closed

1ec5 suggested changes Nov 23, 2016

View reviewed changes

frederoni force-pushed the fred-conform-geojson-6986 branch 4 times, most recently from daa94bc to 7b32cd4 Compare November 24, 2016 16:02

frederoni force-pushed the fred-conform-geojson-6986 branch 3 times, most recently from 40df884 to 4eb7c7b Compare November 28, 2016 13:15

frederoni changed the title ~~GeoJSON support for feature or collection~~ GeoJSON support Nov 28, 2016

frederoni commented Nov 28, 2016

View reviewed changes

frederoni removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Nov 28, 2016

frederoni force-pushed the fred-conform-geojson-6986 branch from 4eb7c7b to 339c774 Compare November 28, 2016 15:15

1ec5 suggested changes Nov 29, 2016

View reviewed changes

frederoni force-pushed the fred-conform-geojson-6986 branch 2 times, most recently from 7a85f4b to 0172790 Compare November 30, 2016 16:18

1ec5 suggested changes Nov 30, 2016

View reviewed changes

1ec5 changed the title ~~GeoJSON support~~ Allow initializing MGLGeoJSONSource from a shape Nov 30, 2016

1ec5 reviewed Dec 1, 2016

View reviewed changes

frederoni force-pushed the fred-conform-geojson-6986 branch from 2ad126b to 52695cf Compare December 2, 2016 15:20

1ec5 approved these changes Dec 2, 2016

View reviewed changes

frederoni force-pushed the fred-conform-geojson-6986 branch from 52695cf to bec9b4c Compare December 5, 2016 11:01

frederoni commented Dec 5, 2016

View reviewed changes

frederoni force-pushed the fred-conform-geojson-6986 branch from bec9b4c to 3e2cf4e Compare December 5, 2016 11:31

boundsj approved these changes Dec 5, 2016

View reviewed changes

frederoni force-pushed the fred-conform-geojson-6986 branch 2 times, most recently from b78e8aa to 47fd189 Compare December 6, 2016 12:04

[ios, macos] MGLGeoJSONSource can now be initialized with any shape

fdd3c40

frederoni force-pushed the fred-conform-geojson-6986 branch from 47fd189 to fdd3c40 Compare December 6, 2016 12:24

frederoni merged commit dc2dc31 into release-ios-v3.4.0 Dec 6, 2016

frederoni mentioned this pull request Dec 6, 2016

[MGLGeoJSONSource initWithIdentifier:geoJSONData] crashes if data isn't a feature collection #6986

Closed

boundsj mentioned this pull request Dec 7, 2016

GeoJSON (Shape) sources initialized with MGLShape objects should work with predicate filters #7336

Closed

1ec5 mentioned this pull request Dec 8, 2016

[ios] Add inline examples in documentation #7337

Merged

jfirebaugh deleted the fred-conform-geojson-6986 branch December 12, 2016 18:47

Conversation

frederoni commented Nov 22, 2016

Uh oh!

mention-bot commented Nov 22, 2016

Uh oh!

1ec5 commented Nov 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frederoni commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frederoni Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frederoni commented Nov 28, 2016

Uh oh!

1ec5 commented Nov 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 commented Dec 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boundsj commented Dec 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

frederoni commented Nov 24, 2016 •

edited

Loading

frederoni Nov 28, 2016 •

edited

Loading

1ec5 Nov 30, 2016 •

edited

Loading

1ec5 Dec 1, 2016 •

edited

Loading