[ios, macos] Load features into shape source if possible #7339

boundsj · 2016-12-08T01:59:54Z

mention-bot · 2016-12-08T01:59:57Z

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

boundsj · 2016-12-08T02:02:44Z

This attempts to keep the features object intact if that is what we actually get in a "shape". Features have identifer and attributes (aka properties in GeoJSON) properties that are required for some styling and filtering operations to work.

The WIP commit so far breaks a GeoJSON source test where a shape collection feature is nested in a shape collection feature.

frederoni · 2016-12-08T08:49:55Z

platform/darwin/src/MGLFeature.mm

+        if ([feature isMemberOfClass:[MGLShapeCollectionFeature class]]) {
+            featureCollection.push_back([self mbglFeatureCollection]);
+        }
+        featureCollection.push_back([feature mbglFeature]);


MGLShapeCollectionFeature raises an exception upon -mbglFeature so this would result in either a crash or a duplicate. I guess this should be in an else statement? Sorry for overlooking this. I should've added regression tests.

That was the intent of the if statement just above here. However I don't think we need to support this case (see comment)

1ec5 · 2016-12-08T09:15:37Z

platform/darwin/src/MGLFeature.mm

+    mbgl::FeatureCollection featureCollection;
+    featureCollection.reserve(self.shapes.count);
+    for (id <MGLFeaturePrivate> feature in self.shapes) {
+        if ([feature isMemberOfClass:[MGLShapeCollectionFeature class]]) {


-isKindOfClass: is more forward-compatible than -isMemberOfClass:. Use the latter only if you want to avoid subclasses, for instance if you want to check for a shape collection that isn’t a shape collection feature.

Good point. We won't ever get here if the super class, MGLShapeCollection, is used since that is a geometry that'll be handled on a different track than what we are doing here for features.

I don't think we actually need to support nested feature collections since it does not look like they are supported in geometry.hpp

1ec5 · 2016-12-08T09:19:34Z

platform/darwin/src/MGLFeature_Private.h

@@ -42,4 +42,10 @@ NS_DICTIONARY_OF(NSString *, id) *NSDictionaryFeatureForGeometry(NSDictionary *g

 @end

+@protocol MGLFeatureCollectionPrivate <NSObject>


Hidden protocols are dangerous because they make it easy to write code that crashes on unrecognized selectors. MGLFeaturePrivate is necessary because MGLFeature is a protocol, but if MGLShapeCollectionFeature is the only class that would conform to this hidden protocol, it’d be safer to declare a Private extension on MGLShapeCollectionFeature specifically.

Sounds good.

MGLShapeCollectionFeature declares conformance to MGLFeaturePrivate (and raises an exception) if you call the only method in that protocol. I just realized / remembered that this allows for the transformation in GeometryEvaluator.

boundsj · 2016-12-09T00:25:44Z

Adjusted the test in 24f8abb since feature collection objects should not be nested (ref https://github.com/mapbox/geometry.hpp/blob/master/include/mapbox/geometry/feature.hpp#L72)

boundsj · 2016-12-09T01:29:12Z

It looks like plain old shapes (i.e. MGLPolygon instead of MGLPolygonFeature) still don't get drawn. I'm going to try and fix that, if possible, before this lands.

boundsj · 2016-12-09T18:26:26Z

It looks like plain old shapes (i.e. MGLPolygon instead of MGLPolygonFeature) still don't get drawn.

Actually this works fine. Not sure what I was seeing before, probably crossed lat/lng in my test data.

@1ec5 @frederoni

boundsj · 2016-12-10T00:29:38Z

Squashed and rebased on top of #7334

1ec5

Two more changes to make, but otherwise this looks good.

1ec5 · 2016-12-10T01:10:20Z

platform/darwin/src/MGLShapeSource.mm

@@ -99,8 +100,15 @@ - (void)commonInit
        source->setGeoJSON(geojson);
        _shape = MGLShapeFromGeoJSON(geojson);
    } else {
-        const auto geojson = mbgl::GeoJSON{self.shape.geometryObject};
-        source->setGeoJSON(geojson);
+        if ([self.shape isMemberOfClass:[MGLShapeCollectionFeature class]]) {


Using -isKindOfClass: will also avoid latent bugs if we ever end up needing to subclass MGLShapeCollectionFeature ourselves.

1ec5 · 2016-12-10T01:11:12Z

platform/ios/ios.xcodeproj/project.pbxproj

@@ -592,6 +592,7 @@
 		4085AF081D933DEA00F11B22 /* MGLTileSetTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = MGLTileSetTests.mm; path = ../../darwin/test/MGLTileSetTests.mm; sourceTree = "<group>"; };
 		408AA8551DAEDA0800022900 /* NSDictionary+MGLAdditions.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "NSDictionary+MGLAdditions.h"; sourceTree = "<group>"; };
 		408AA8561DAEDA0800022900 /* NSDictionary+MGLAdditions.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = "NSDictionary+MGLAdditions.mm"; sourceTree = "<group>"; };
+		40986FE51DFA2B5C004A7E6E /* MGLShapeCollectionFeature_Private.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = MGLShapeCollectionFeature_Private.h; sourceTree = "<group>"; };


Add this file to macos.xcodeproj too.

That's strange... it is added locally. I'll try to figure out why it is not in this changeset

Never mind! Fixed https://github.com/mapbox/mapbox-gl-native/pull/7339/files#diff-9276451d661540f77943125827376c97R303

1ec5 · 2016-12-12T01:17:57Z

This PR blocks #7377, where the conversion from an MGLFeature to an mbgl feature will be used as part of a general-purpose GeoJSON (de)serialization utility.

This checks the kind of MGLShape passed into the source and, if it is a feature, it mbgl feature objects to pass to core. This keeps the feature id and attributes data intact. If the shape is a `MGLShapeCollectionFeature` it creates an `mbgl::FeatureCollection` object (also to keep feature properties). If the shape is not any sort of feature, it passes along just the geometry. This also uses the MGLFeatureFromMBGLFeature converter for the case where GeoJSON data passed in to a source contains a single feature. The converter has logic to keep the id and attributes properties intact. Before, these properties were lost because only geometry conversion was done. Finally, logic to handle (and associated tests) of nested shape collection features is removed since that is not supported by the GeoJSON spec or the core implementation. [ios] Add test of drawing plain shape to iosapp

* release-ios-v3.4.0: [ios] Fixed crash on launch in iosapp [ios] Fix iosapp runtime styling examples (#7395) [ios, macos] handle duplicate layer error [core] guard against duplicate layer ids [core] use raii to guard backend deactivation [ios, macos] Override references to property names in property requirements lists (#7391) [ios, macos] Load features into shape source if possible (#7339) [ios, macos] Expanded source documentation [ios, macos] Rename MGLGeoJSONSource to MGLShapeSource (#7334) [ios, macos] Silence -Wc++11-narrowing warning in open gl layer (#7355) [ios, macos] Source-driven attribution (#5999) [ios, macos] renamed raster-hue-rotate # Conflicts: # platform/darwin/src/MGLRasterSource.h # platform/darwin/src/MGLShapeSource.h # platform/darwin/src/MGLSource.h # platform/darwin/src/MGLVectorSource.h

* release-ios-v3.4.0: [ios] Fixed crash on launch in iosapp [ios] Fix iosapp runtime styling examples (#7395) [ios, macos] handle duplicate layer error [core] guard against duplicate layer ids [core] use raii to guard backend deactivation [ios, macos] Override references to property names in property requirements lists (#7391) [ios, macos] Load features into shape source if possible (#7339) [ios, macos] Expanded source documentation [ios, macos] Rename MGLGeoJSONSource to MGLShapeSource (#7334) [ios, macos] Silence -Wc++11-narrowing warning in open gl layer (#7355) [ios, macos] Source-driven attribution (#5999) [ios, macos] renamed raster-hue-rotate

* release-ios-v3.4.0: [ios] Fixed crash on launch in iosapp [ios] Fix iosapp runtime styling examples (#7395) [ios, macos] handle duplicate layer error [core] guard against duplicate layer ids [core] use raii to guard backend deactivation [ios, macos] Override references to property names in property requirements lists (#7391) [ios, macos] Load features into shape source if possible (#7339) [ios, macos] Expanded source documentation [ios, macos] Rename MGLGeoJSONSource to MGLShapeSource (#7334) [ios, macos] Silence -Wc++11-narrowing warning in open gl layer (#7355) [ios, macos] Source-driven attribution (#5999) [ios, macos] renamed raster-hue-rotate # Conflicts: # platform/ios/jazzy.yml

boundsj added bug iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS release blocker Blocks the next final release labels Dec 8, 2016

boundsj added this to the ios-v3.4.0 milestone Dec 8, 2016

boundsj self-assigned this Dec 8, 2016

boundsj added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Dec 8, 2016

frederoni reviewed Dec 8, 2016

View reviewed changes

1ec5 suggested changes Dec 8, 2016

View reviewed changes

1ec5 added the runtime styling label Dec 8, 2016

boundsj force-pushed the boundsj-use-features-if-possible branch from b245a63 to 24f8abb Compare December 9, 2016 00:23

boundsj added ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Dec 9, 2016

boundsj force-pushed the boundsj-use-features-if-possible branch from cfed7a8 to 4ad4f8d Compare December 10, 2016 00:29

boundsj changed the title ~~[ios, macos] Load features into GeoJSON source if possible~~ [ios, macos] Load features into shape source if possible Dec 10, 2016

boundsj force-pushed the boundsj-use-features-if-possible branch from 4ad4f8d to a2a66bf Compare December 10, 2016 00:54

1ec5 approved these changes Dec 10, 2016

View reviewed changes

1ec5 mentioned this pull request Dec 12, 2016

Simplify MGLSource and subclasses #7377

Merged

14 tasks

boundsj force-pushed the boundsj-use-features-if-possible branch from a2a66bf to 219aea8 Compare December 12, 2016 17:31

boundsj merged commit e38b4b5 into release-ios-v3.4.0 Dec 12, 2016

boundsj deleted the boundsj-use-features-if-possible branch December 12, 2016 18:38

boundsj mentioned this pull request Dec 12, 2016

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

Closed

1ec5 removed the ✓ ready for review label Dec 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios, macos] Load features into shape source if possible #7339

[ios, macos] Load features into shape source if possible #7339

boundsj commented Dec 8, 2016 •

edited

Loading

mention-bot commented Dec 8, 2016

boundsj commented Dec 8, 2016

frederoni Dec 8, 2016 •

edited

Loading

boundsj Dec 8, 2016

1ec5 Dec 8, 2016

boundsj Dec 8, 2016

1ec5 Dec 8, 2016 •

edited

Loading

boundsj Dec 8, 2016

boundsj commented Dec 9, 2016

boundsj commented Dec 9, 2016

boundsj commented Dec 9, 2016

boundsj commented Dec 10, 2016

1ec5 left a comment

1ec5 Dec 10, 2016

1ec5 Dec 10, 2016

boundsj Dec 12, 2016

boundsj Dec 12, 2016

1ec5 commented Dec 12, 2016

		@@ -42,4 +42,10 @@ NS_DICTIONARY_OF(NSString , id) NSDictionaryFeatureForGeometry(NSDictionary *g

		@end

		@protocol MGLFeatureCollectionPrivate <NSObject>

[ios, macos] Load features into shape source if possible #7339

[ios, macos] Load features into shape source if possible #7339

Conversation

boundsj commented Dec 8, 2016 • edited Loading

mention-bot commented Dec 8, 2016

boundsj commented Dec 8, 2016

frederoni Dec 8, 2016 • 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 Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boundsj commented Dec 9, 2016

boundsj commented Dec 9, 2016

boundsj commented Dec 9, 2016

boundsj commented Dec 10, 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

1ec5 commented Dec 12, 2016

boundsj commented Dec 8, 2016 •

edited

Loading

frederoni Dec 8, 2016 •

edited

Loading

1ec5 Dec 8, 2016 •

edited

Loading