[core] Fix within expression

CHANGELOG.md

@@ -26,6 +26,10 @@
 
   But `within` expression need to accept an Object and then convert to GeoJSON object, now `toGeoJSON` method can convert both string and Object to GeoJSON.
 
+- [core] Fix `within` expression algorithm so that `false` value will be returned when point is on the boundary.  Allow using different GeoJSON formats as arguments of `within` expression.([#16232](https://github.com/mapbox/mapbox-gl-native/pull/16232))
+
+  A valid GeoJSON argument should contain one of the following types:  `"Feature"`, `"FeatureCollection"`,`"Polygon"` or `"MultiPolygon"`. 
+
 ### 🧩  Architectural changes
 
 - Changes to `MapSnapshotter` threading model ([#16268](https://github.com/mapbox/mapbox-gl-native/pull/16268))

include/mbgl/style/expression/within.hpp

@@ -11,7 +11,7 @@ namespace expression {
 
 class Within final : public Expression {
 public:
-    explicit Within(GeoJSON geojson, Feature::geometry_type geometries_, WithinBBox polygonBBox_);
+    explicit Within(GeoJSON geojson, Feature::geometry_type geometries_, const WithinBBox& polygonBBox_);
 
     ~Within() override;
 

src/mbgl/style/expression/within.cpp

@@ -13,27 +13,6 @@
 namespace mbgl {
 namespace {
 
-class PolygonFeature : public GeometryTileFeature {
-public:
-    const Feature& feature;
-    mutable optional<GeometryCollection> geometry;
-
-    PolygonFeature(const Feature& feature_, const CanonicalTileID& canonical) : feature(feature_) {
-        geometry = convertGeometry(feature.geometry, canonical);
-        assert(geometry);
-        geometry = fixupPolygons(*geometry);
-    }
-
-    FeatureType getType() const override { return FeatureType::Polygon; }
-    const PropertyMap& getProperties() const override { return feature.properties; }
-    FeatureIdentifier getID() const override { return feature.id; }
-    optional<mbgl::Value> getValue(const std::string& /*key*/) const override { return optional<mbgl::Value>(); }
-    const GeometryCollection& getGeometries() const override {
-        assert(geometry);
-        return *geometry;
-    }
-};
-
 bool pointsWithinPolygons(const mbgl::GeometryTileFeature& feature,
                           const mbgl::CanonicalTileID& canonical,
                           const Feature::geometry_type& polygonGeoSet,
@@ -109,29 +88,38 @@ bool linesWithinPolygons(const mbgl::GeometryTileFeature& feature,
 mbgl::optional<mbgl::GeoJSON> parseValue(const mbgl::style::conversion::Convertible& value_,
                                          mbgl::style::expression::ParsingContext& ctx) {
     if (isObject(value_)) {
-        const auto geometryType = objectMember(value_, "type");
-        if (geometryType) {
-            const auto type = toString(*geometryType);
-            if (type && *type == "Polygon") {
-                mbgl::style::conversion::Error error;
-                auto geojson = toGeoJSON(value_, error);
-                if (geojson && error.message.empty()) {
-                    return geojson;
-                }
-                ctx.error(error.message);
-            }
+        mbgl::style::conversion::Error error;
+        auto geojson = toGeoJSON(value_, error);
+        if (geojson && error.message.empty()) {
+            return geojson;
         }
+        ctx.error(error.message);
     }
+
     ctx.error("'within' expression requires valid geojson source that contains polygon geometry type.");
     return nullopt;
 }
 
+struct PolygonInfo {
+    PolygonInfo(const Feature::geometry_type& geometry_) : geometry(geometry_), bbox(calculateBBox(geometry)){};
+    Feature::geometry_type geometry;
+    WithinBBox bbox;
+};
+
+mbgl::optional<PolygonInfo> getPolygonInfo(const Feature& polyFeature, mbgl::style::expression::ParsingContext& ctx) {
+    const auto type = apply_visitor(ToFeatureType(), polyFeature.geometry);
+    if (type == FeatureType::Polygon) {
+        return PolygonInfo(polyFeature.geometry);
+    }
+    ctx.error("'within' expression requires valid geojson source that contains polygon geometry type.");
+    return nullopt;
+}
 } // namespace
 
 namespace style {
 namespace expression {
 
-Within::Within(GeoJSON geojson, Feature::geometry_type geometries_, WithinBBox polygonBBox_)
+Within::Within(GeoJSON geojson, Feature::geometry_type geometries_, const WithinBBox& polygonBBox_)
     : Expression(Kind::Within, type::Boolean),
       geoJSONSource(std::move(geojson)),
       geometries(std::move(geometries_)),
@@ -173,15 +161,28 @@ ParseResult Within::parse(const Convertible& value, ParsingContext& ctx) {
         }
 
         return parsedValue->match(
-            [&parsedValue](const mapbox::geometry::geometry<double>& geometrySet) {
-                mbgl::Feature f(geometrySet);
-                PolygonFeature polyFeature(f, CanonicalTileID(0, 0, 0));
-                auto refinedGeoSet = convertGeometry(polyFeature, CanonicalTileID(0, 0, 0));
-                auto bbox = calculateBBox(refinedGeoSet);
-                return ParseResult(std::make_unique<Within>(*parsedValue, std::move(refinedGeoSet), bbox));
+            [&parsedValue, &ctx](const mapbox::geometry::geometry<double>& geometrySet) {
+                if (auto ret = getPolygonInfo(mbgl::Feature(geometrySet), ctx)) {
+                    return ParseResult(std::make_unique<Within>(*parsedValue, std::move(ret->geometry), ret->bbox));
+                }
+                return ParseResult();
+            },
+            [&parsedValue, &ctx](const mapbox::feature::feature<double>& feature) {
+                if (auto ret = getPolygonInfo(mbgl::Feature(feature), ctx)) {
+                    return ParseResult(std::make_unique<Within>(*parsedValue, std::move(ret->geometry), ret->bbox));
+                }
+                return ParseResult();
+            },
+            [&parsedValue, &ctx](const mapbox::feature::feature_collection<double>& features) {
+                for (const auto& feature : features) {
+                    if (auto ret = getPolygonInfo(mbgl::Feature(feature), ctx)) {
+                        return ParseResult(std::make_unique<Within>(*parsedValue, std::move(ret->geometry), ret->bbox));
+                    }
+                }
+                return ParseResult();
             },
             [&ctx](const auto&) {
-                ctx.error("'within' expression requires geojson source that contains valid geometry data.");
+                ctx.error("'within' expression requires valid geojson source that contains polygon geometry type.");
                 return ParseResult();
             });
     }
@@ -204,6 +205,13 @@ Value valueConverter(const mapbox::geojson::rapidjson_value& v) {
         }
         return result;
     }
+    if (v.IsObject()) {
+        std::unordered_map<std::string, Value> result;
+        for (const auto& m : v.GetObject()) {
+            result.emplace(m.name.GetString(), valueConverter(m.value));
+        }
+        return result;
+    }
     // Ignore other types as valid geojson only contains above types.
     return Null;
 }
@@ -226,7 +234,7 @@ mbgl::Value Within::serialize() const {
 bool Within::operator==(const Expression& e) const {
     if (e.getKind() == Kind::Within) {
         auto rhs = static_cast<const Within*>(&e);
-        return geoJSONSource == rhs->geoJSONSource;
+        return geoJSONSource == rhs->geoJSONSource && geometries == rhs->geometries && polygonBBox == rhs->polygonBBox;
     }
     return false;
 }

src/mbgl/util/geometry_within.cpp

@@ -9,6 +9,18 @@ bool rayIntersect(const Point<double>& p, const Point<double>& p1, const Point<d
     return ((p1.y > p.y) != (p2.y > p.y)) && (p.x < (p2.x - p1.x) * (p.y - p1.y) / (p2.y - p1.y) + p1.x);
 }
 
+// check if point p in on line segment with end points p1 and p2
+bool onBoundary(const Point<double>& p, const Point<double>& p1, const Point<double>& p2) {
+    // requirements of point p on line segment:
+    // 1. colinear: cross product of vector p->p1(x1, y1) and vector p->p2(x2, y2) equals to 0
+    // 2. p is between p1 and p2
+    const auto x1 = p.x - p1.x;
+    const auto y1 = p.y - p1.y;
+    const auto x2 = p.x - p2.x;
+    const auto y2 = p.y - p2.y;
+    return (x1 * y2 - x2 * y1 == 0) && (x1 * x2 <= 0) && (y1 * y2 <= 0);
+}
+
 // a, b are end points for line segment1, c and d are end points for line segment2
 bool lineIntersectLine(const Point<double>& a, const Point<double>& b, const Point<double>& c, const Point<double>& d) {
     const auto perp = [](const Point<double>& v1, const Point<double>& v2) { return (v1.x * v2.y - v1.y * v2.x); };
@@ -134,6 +146,7 @@ bool pointWithinPolygon(const Point<double>& point, const Polygon<double>& polyg
         const auto length = ring.size();
         // loop through every edge of the ring
         for (std::size_t i = 0; i < length - 1; ++i) {
+            if (onBoundary(point, ring[i], ring[i + 1])) return false;
             if (rayIntersect(point, ring[i], ring[i + 1])) {
                 within = !within;
             }

test/style/property_expression.test.cpp

@@ -334,5 +334,10 @@ TEST(PropertyExpression, WithinExpression) {
         pointFeature = getPointFeature(Point<double>(-15.5126953125, -11.73830237143684));
         evaluatedResult = propExpr.evaluate(EvaluationContext(&pointFeature).withCanonicalTileID(&canonicalTileID));
         EXPECT_TRUE(evaluatedResult);
+
+        // On the boundary
+        pointFeature = getPointFeature(Point<double>(3.076171875, -7.01366792756663));
+        evaluatedResult = propExpr.evaluate(EvaluationContext(&pointFeature).withCanonicalTileID(&canonicalTileID));
+        EXPECT_FALSE(evaluatedResult);
     }
 }