Warn about attribute loss when using shape collection in shape source

[JacobJT]

Platform: iOS
Mapbox SDK version: 3.7

Steps to trigger behavior

1. Add MGLSymbolStyleLayer using MGLShapeSource with MGLShapeCollection as source, initialized with MGLPointFeatures
2. Use gesture recognizer calling [mapView visibleFeaturesAtPoint: inStyleLayersWithIdentifiers:] to get the features at the point tapped on map
3. Try to access attributes data of the features added

Expected behavior

Attributes of the MGLPointFeatures which were added to the MGLShapeCollection are accessible

Actual behavior

Attributes are not accessible

I have a handful of 'types' of markers I'm trying to display, and tapping them should open a new screen. In order to format the new screen, I'm adding some meta data to the attributes of each of these MGLPointFeatures. However, when I call visibleFeaturesAtPoint, the points exist but have no attributes.

When I'm adding points, I am accessing a reference to the MGLShapeCollection with the existing points, and creating a new NSArray of MGLPointFeatures using [[shapeCollection shapes] arrayByAddingObject:newPoint]; When I add a break point and view this new array, it contains all of my features with their full attributes, so the MGLShapeCollection has the appropriate data. I create a new MGLShapeCollection with this new array, and I call [source setShape:shapeCollection]; This works in the sense that it adds all of the MGLPointFeatures to the map. They'll be formatted according to the MGLSymbolStyleLayer that the source belongs to. But their attributes are gone when I try to access them when the map is tapped.

[JacobJT]

I've found the fix is simply to use an MGLShapeCollectionFeature instead of MGLShapeCollection. This wasn't clear in the documentation - the documentation made it appear that the difference was simply that the shape collection itself was also given an identifier and attributes. Especially considering that the MGLShapeCollection's shape property contained the proper feature objects with their attributes. It wasn't until adding the MGLShapeCollection to a source that the attributes were stripped from the features.

I found the solution by looking at the implementation of MGLShapeSource's initWithIdentifier:features:options method, which added the array of features to an MGLShapeCollectionFeature and assigned them to the source's shape.
https://github.com/mapbox/mapbox-gl-native/blob/master/platform/darwin/src/MGLShapeSource.mm

I recommend updating the documentation of MGLShapeCollection to point out that if you want to maintain the attributes of features added to use the MGLShapeCollectionFeature, and update the documentation of MGLShapeCollectionFeature to point out that it is recommended to use instead of MGLShapeCollection when you're adding features instead of shapes.

[1ec5]

Related to this confusing situation: #7454 #7453.

[JacobJT]

Ahh, I had done some searching but didn't see that. I see in one of them you even discuss either changing to MGLShapeCollectionFeature when detecting a feature in the shapes array, or turning the feature into a shape.

Personally, the current functionality would have been easy to work with if I had any understanding of what was going on. A debug log saying "Warning: One or more shapes added to this MGLShapeCollection were features. All metadata will be stripped. Please use MGLShapeCollectionFeature if you'd like to retain the metadata" or something similar would have made this a much smaller head ache.

[1ec5]

That’s a good point. Until we fix #7454, documentation can only mitigate this trap but not eliminate it. A one-time console warning is a good idea.

[JacobJT]

That's all I would have needed!

A note at the top of this documentation saying "If using MGLShapeFeatures, you should use an MGLShapeCollectionFeature instead."
https://www.mapbox.com/ios-sdk/api/3.7.5/Classes/MGLShapeCollection.html

And perhaps modify this MGLShapeCollectionFeature documentation to say it uses ShapeFeatures instead of Shapes? From the documentation, it says An MGLShapeCollectionFeature object associates a shape collection with an optional identifier and attributes., so even though I had seen this in the documentation before implementing what I needed, I was under the impression that the only difference between using the two would be that the ShapeCollection itself gets the identifier / attributes.
https://www.mapbox.com/ios-sdk/api/3.7.5/Style%20Primitives.html#/c:objc(cs)MGLShapeCollectionFeature

[JacobJT]

Is the documentation update considered enough to close this issue? I'd still think the one time console warning would be ideal. Are there any use cases where one would intentionally want to stick features into an MGLShapeCollection instead of MGLShapeCollectionFeature?

[1ec5]

I suppose that would be one way to erase feature attributes without having to create separate copies of everything, but I’m struggling to think of a use case for what’s essentially data loss. The one-time warning would be useful, so this issue remains open to track that improvement.

[1ec5]

Assuming there is a legitimate use case for stuffing a feature in a shape collection, we could instead warn when adding a shape collection to a shape source, as originally suggested above.

[captainbarbosa]

#12625 added a one-time warning when adding a shape collection to a shape source. Combined with the documentation improvements in #11462, this will hopefully make it more clear.