From b9c99180740951885c7ea74d5625af5c1c6febf7 Mon Sep 17 00:00:00 2001 From: zmiao Date: Wed, 5 Feb 2020 12:27:09 +0200 Subject: [PATCH] fix review findings --- include/mbgl/style/expression/expression.hpp | 4 +- include/mbgl/style/expression/within.hpp | 6 +- src/mbgl/layout/pattern_layout.hpp | 4 +- src/mbgl/style/expression/within.cpp | 58 +++++++++++--------- 4 files changed, 40 insertions(+), 32 deletions(-) diff --git a/include/mbgl/style/expression/expression.hpp b/include/mbgl/style/expression/expression.hpp index 1b339fa2946..1e34a8bd387 100644 --- a/include/mbgl/style/expression/expression.hpp +++ b/include/mbgl/style/expression/expression.hpp @@ -61,13 +61,13 @@ class EvaluationContext { optional zoom; optional accumulated; - const mbgl::CanonicalTileID* canonical = nullptr; - GeometryTileFeature const * feature = nullptr; + GeometryTileFeature const* feature = nullptr; optional colorRampParameter; // Contains formatted section object, std::unordered_map. const Value* formattedSection = nullptr; const FeatureState* featureState = nullptr; const std::set* availableImages = nullptr; + const mbgl::CanonicalTileID* canonical = nullptr; }; template diff --git a/include/mbgl/style/expression/within.hpp b/include/mbgl/style/expression/within.hpp index 5e2b5e06719..88e9cc56b89 100644 --- a/include/mbgl/style/expression/within.hpp +++ b/include/mbgl/style/expression/within.hpp @@ -11,9 +11,11 @@ namespace mbgl { namespace style { namespace expression { -class Within : public Expression { +class Within final : public Expression { public: - Within(GeoJSON& geojson) : Expression(Kind::Within, type::Boolean), geoJSONSource(geojson) {} + explicit Within(GeoJSON geojson); + + ~Within() final; EvaluationResult evaluate(const EvaluationContext&) const override; diff --git a/src/mbgl/layout/pattern_layout.hpp b/src/mbgl/layout/pattern_layout.hpp index 61128610d6d..81ff3996dbe 100644 --- a/src/mbgl/layout/pattern_layout.hpp +++ b/src/mbgl/layout/pattern_layout.hpp @@ -157,8 +157,8 @@ class PatternLayout : public Layout { void createBucket(const ImagePositions& patternPositions, std::unique_ptr& featureIndex, std::unordered_map& renderData, - const bool, - const bool, + const bool /*firstLoad*/, + const bool /*showCollisionBoxes*/, const CanonicalTileID& canonical) override { auto bucket = std::make_shared(layout, layerPropertiesMap, zoom, overscaling); for (auto & patternFeature : features) { diff --git a/src/mbgl/style/expression/within.cpp b/src/mbgl/style/expression/within.cpp index 1b1b8b11b98..d02ea47cb22 100644 --- a/src/mbgl/style/expression/within.cpp +++ b/src/mbgl/style/expression/within.cpp @@ -1,10 +1,13 @@ +#include + #include #include #include -#include #include +#include #include +namespace mbgl { namespace { double isLeft(mbgl::Point P0, mbgl::Point P1, mbgl::Point P2) { @@ -13,14 +16,13 @@ double isLeft(mbgl::Point P0, mbgl::Point P1, mbgl::Point point, const mapbox::geometry::polygon& polys) { +bool pointWithinPolygons(mbgl::Point point, const mapbox::geometry::polygon& polys) { // wn = the winding number (=0 only when point is outside) int wn = 0; for (auto poly : polys) { auto size = poly.size(); - auto i = size; // loop through every edge of the polygon - for (i = 0; i < size - 1; ++i) { + for (decltype(size) i = 0; i < size - 1; ++i) { if (poly[i].y <= point.y) { if (poly[i + 1].y > point.y) { // upward horizontal crossing from point to edge E(poly[i], poly[i+1]) if (isLeft(poly[i], poly[i + 1], point) > 0) { @@ -42,34 +44,33 @@ bool pointWithinPoly(mbgl::Point point, const mapbox::geometry::polygon< return wn != 0; } -bool pointsWithinPoly(const mbgl::GeometryTileFeature& feature, - const mbgl::CanonicalTileID& canonical, - const mbgl::GeoJSON& geoJson) { - bool result = false; - geoJson.match( - [&](const mapbox::geometry::geometry& geometrySet) { - geometrySet.match( - [&](const mapbox::geometry::polygon& polygons) { - convertGeometry(feature, canonical) +bool pointsWithinPolygons(const mbgl::GeometryTileFeature& feature, + const mbgl::CanonicalTileID& canonical, + const mbgl::GeoJSON& geoJson) { + return geoJson.match( + [&feature, &canonical](const mapbox::geometry::geometry& geometrySet) -> bool { + return geometrySet.match( + [&feature, &canonical](const mapbox::geometry::polygon& polygons) -> bool { + return convertGeometry(feature, canonical) .match( - [&](const mapbox::geometry::point& point) { - result = pointWithinPoly(point, polygons); + [&polygons](const mapbox::geometry::point& point) -> bool { + return pointWithinPolygons(point, polygons); }, - [&](const mapbox::geometry::multi_point& points) { + [&polygons](const mapbox::geometry::multi_point& points) -> bool { + auto result = false; for (const auto& p : points) { - result = pointWithinPoly(p, polygons); + result = pointWithinPolygons(p, polygons); if (!result) { - return; + return result; } } + return result; }, - [&](const auto&) { return; }); + [](const auto&) -> bool { return false; }); }, - [&](const auto&) { return; }); + [](const auto&) -> bool { return false; }); }, - [&](const auto&) { return; }); - - return result; + [](const auto&) -> bool { return false; }); } mbgl::optional parseValue(const mbgl::style::conversion::Convertible& value_, @@ -89,15 +90,18 @@ mbgl::optional parseValue(const mbgl::style::conversion::Converti } } ctx.error("'Within' expression requires valid geojson source that contains polygon geometry type."); - return mbgl::optional(); + return nullopt; } } // namespace -namespace mbgl { namespace style { namespace expression { +Within::Within(GeoJSON geojson) : Expression(Kind::Within, type::Boolean), geoJSONSource(std::move(geojson)) {} + +Within::~Within() {} + using namespace mbgl::style::conversion; EvaluationResult Within::evaluate(const EvaluationContext& params) const { @@ -107,7 +111,9 @@ EvaluationResult Within::evaluate(const EvaluationContext& params) const { auto geometryType = params.feature->getType(); // Currently only support Point/Points in polygon if (geometryType == FeatureType::Point) { - return pointsWithinPoly(*params.feature, *params.canonical, geoJSONSource); + return pointsWithinPolygons(*params.feature, *params.canonical, geoJSONSource); + } else { + mbgl::Log::Warning(mbgl::Event::General, "Within expression currently only support 'Point' geometry type"); } return false; }