Allow predicates to test whether a feature lies within a given shape

[1ec5]

Added support for the NSPredicate syntax SELF IN shape or shape CONTAINS SELF, where shape is an MGLShape object or MGLFeature instance. These syntaxes are interpreted as within expressions in JSON format.

Fixed an issue where GeoJSON geometries were converted to MGLFeature instances instead of MGLShape objects. This issue probably hasn’t come up in the wild because the conversion code was previously only part of a feature querying code path, and feature querying methods return features anyways. But since it’s possible for the shape in SELF IN shape to be an MGLShape, that shape should get round-tripped as an MGLShape rather than an MGLFeature.

Fixes #175. Depends on #183, which in turn depends on #181. Retarget and rebase this PR onto master before merging.

/cc @mapbox/maps-ios

[1ec5]

This works, but there may be a more efficient way to convert this JSON-like NSDictionary structure into an MGLShape. Perhaps we could first convert the NSDictionary structure into a C++ representation of the GeoJSON object, then convert to an MGLShape.

[fabian-guerra]

Is there any performance considerations for bridging into an C++ object then back to MGLShape? Is this something that is going to be called frequently?

[1ec5]

In the current implementation, MGLStyleValueTransformer::PropertyExpressionEvaluator converts the C++ mapbox::geojson struct to a GeoJSON-formatted NSDictionary, then this method converts it to a string, then back to a C++ mapbox::geojson struct, then finally to an MGLShape object. It is more roundabout than it should be.

This conversion process happens once when setting a layer’s predicate or paint/layout property (which can happen automatically e.g. when localizing the style). mbgl will evaluate the already-converted expression on each feature, but no SDK-side code will run as part of the rendering loop.

Ideally, mbgl would preserve the mapbox::geojson struct so that MGLJSONObjectFromMBGLValue() could convert it directly to an MGLShape. Absent such a change in mbgl, there are two alternative, purely SDK-side approaches:

• Convert the NSDictionary directly to C++ mapbox::geojson structs then to MGLShape, skipping NSJSONSerialization
• Parse the NSDictionary into an MGLShape, skipping C++

but I’m inclined to treat either optimization as tail work for when we know more about how this expression operator will be used. The performance characteristics could differ depending on the complexity of the passed-in GeoJSON feature.

[chloekraw]

@1ec5 will there be a developer-facing API that references within in its name?

[1ec5]

will there be a developer-facing API that references within in its name?

The SELF IN syntax is available for developers to use like this (notice lovely highlighting of NSExpression format string syntax):

[NSPredicate predicateWithFormat:@"SELF IN %@", [MGLPolygon polygonWithCoodinates:coords count:4]];

But if you’re referring to something more reminiscent of the JSON expression format, it’s also possible to use within expressions directly, using either of these syntaxes:

•  [NSPredicate predicateWithFormat:@"MGL_FUNCTION('within', %@)", [MGLPolygon polygonWithCoodinates:coords count:4]];

•  [NSPredicate predicateWithMGLJSONObject:@[@"within", @{@"type": @"Polygon", @"coordinates": @[@[@[@0, @0], @[@0, @1], @[@1, @1], @[@1, @0]]]}]];

The latter syntax is already possible without this PR, now that #183 has landed.

I suppose we could install an aftermarket function like mgl_within: to avoid MGL_FUNCTION(), but I’m not sure of the use case given the IN and CONTAINS operators.

[1ec5]

As always, if a developer is already familiar with the style specification’s JSON expression format or a similar syntax on Android, they can consult the “Information for Style Authors” guide to learn the corresponding NSExpression syntax.

[1ec5]

Per #175 (comment), MGLConversion needs to convert from mbgl::GeoJSON to MGLShape:

mapbox-gl-native-ios/platform/darwin/src/MGLConversion.h

Lines 127 to 130 in 534d64c

	static optional<GeoJSON> toGeoJSON(const Holder&, Error& error) {
	error = { "toGeoJSON not implemented" };
	return {};
	}

This code is called as part of -[NSPredicate(MGLAdditions) mgl_filter]. The lack of a conversion probably explains why I was unable to preserve the filter roundtripping assertions as part of the unit tests in this PR.

[julianrex]

Thanks! Just the question from @fabian-guerra

[fabian-guerra]

I agree that we could treat optimizations as tail work. Thank you for explaining. LGTM in that case.

[1ec5]

As of 11424ded73745e734c2d1f58b9fad8d7bd8ae635, the Debug ‣ Manipulate Style command adds a circle layer that should mark each POI around here. However, nothing shows up because the within expression always evaluates to false. The conversion from NSExpression to mbgl::style::expression::Within is working correctly, as is the conversion from GeoJSON-formatted NSDictionary to mapbox::geometry. Here’s what the GeoJSON representation of the shape now that toGeoJSON() has been implemented:

{"type":"Polygon","coordinates":[[[-84.520899999999997,39.127899999999997],[-84.511200000000002,39.127299999999998],[-84.510199999999998,39.1355],[-84.521199999999993,39.136000000000003],[-84.520899999999997,39.127899999999997]]]}

Within::evaluate() returns false because pointsWithinPolygons() returns false because boxWithinBox() returns false because isBBoxValid() returns false because Within.polygonBBox is never set. Something in the call to convertGeometry() causes the polygon’s only ring to go missing. In this part of the method, pg is empty.

As far as I can tell, the SDK code is behaving correctly and the shape is valid, but the expression never evaluates to true. I’ve also tried reversing the polygon and removing the closing point to no avail.

/cc @zmiao

[1ec5]

MGLFeature support was added in mapbox/mapbox-gl-native#16232, which we haven’t pulled into this repository yet.

[1ec5]

However, nothing shows up because the within expression always evaluates to false.

I’ve confirmed that this will also be fixed by mapbox/mapbox-gl-native#16232 once we pull it in.

[1ec5]

However, nothing shows up because the within expression always evaluates to false.

I’ve confirmed that this will also be fixed by mapbox/mapbox-gl-native#16232 once we pull it in.

#210 will pull in the fix. No additional changes are needed for within support at the SDK level, but this PR depends on #183, which is waiting for #210.