diff --git a/CHANGELOG.md b/CHANGELOG.md index 67206108103..ab95815b5b3 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/include/mbgl/style/expression/within.hpp b/include/mbgl/style/expression/within.hpp index 2bc59a05964..eb23c7d0e03 100644 --- a/include/mbgl/style/expression/within.hpp +++ b/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; diff --git a/src/mbgl/style/expression/within.cpp b/src/mbgl/style/expression/within.cpp index 9039ff2fe41..49ae8b54a8b 100644 --- a/src/mbgl/style/expression/within.cpp +++ b/src/mbgl/style/expression/within.cpp @@ -13,27 +13,6 @@ namespace mbgl { namespace { -class PolygonFeature : public GeometryTileFeature { -public: - const Feature& feature; - mutable optional 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 getValue(const std::string& /*key*/) const override { return optional(); } - 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 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 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& 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(*parsedValue, std::move(refinedGeoSet), bbox)); + [&parsedValue, &ctx](const mapbox::geometry::geometry& geometrySet) { + if (auto ret = getPolygonInfo(mbgl::Feature(geometrySet), ctx)) { + return ParseResult(std::make_unique(*parsedValue, std::move(ret->geometry), ret->bbox)); + } + return ParseResult(); + }, + [&parsedValue, &ctx](const mapbox::feature::feature& feature) { + if (auto ret = getPolygonInfo(mbgl::Feature(feature), ctx)) { + return ParseResult(std::make_unique(*parsedValue, std::move(ret->geometry), ret->bbox)); + } + return ParseResult(); + }, + [&parsedValue, &ctx](const mapbox::feature::feature_collection& features) { + for (const auto& feature : features) { + if (auto ret = getPolygonInfo(mbgl::Feature(feature), ctx)) { + return ParseResult(std::make_unique(*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 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(&e); - return geoJSONSource == rhs->geoJSONSource; + return geoJSONSource == rhs->geoJSONSource && geometries == rhs->geometries && polygonBBox == rhs->polygonBBox; } return false; } diff --git a/src/mbgl/util/geometry_within.cpp b/src/mbgl/util/geometry_within.cpp index c79952b8f27..4a0a5cce112 100644 --- a/src/mbgl/util/geometry_within.cpp +++ b/src/mbgl/util/geometry_within.cpp @@ -9,6 +9,18 @@ bool rayIntersect(const Point& p, const Point& p1, const Point 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& p, const Point& p1, const Point& 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& a, const Point& b, const Point& c, const Point& d) { const auto perp = [](const Point& v1, const Point& v2) { return (v1.x * v2.y - v1.y * v2.x); }; @@ -134,6 +146,7 @@ bool pointWithinPolygon(const Point& point, const Polygon& 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; } diff --git a/test/style/property_expression.test.cpp b/test/style/property_expression.test.cpp index 88096bab6b1..419aff30a2d 100644 --- a/test/style/property_expression.test.cpp +++ b/test/style/property_expression.test.cpp @@ -334,5 +334,10 @@ TEST(PropertyExpression, WithinExpression) { pointFeature = getPointFeature(Point(-15.5126953125, -11.73830237143684)); evaluatedResult = propExpr.evaluate(EvaluationContext(&pointFeature).withCanonicalTileID(&canonicalTileID)); EXPECT_TRUE(evaluatedResult); + + // On the boundary + pointFeature = getPointFeature(Point(3.076171875, -7.01366792756663)); + evaluatedResult = propExpr.evaluate(EvaluationContext(&pointFeature).withCanonicalTileID(&canonicalTileID)); + EXPECT_FALSE(evaluatedResult); } }