From 7fda6ecad3e1119995b1bb23257995f730ac2a23 Mon Sep 17 00:00:00 2001 From: zmiao Date: Tue, 11 Feb 2020 14:40:51 +0200 Subject: reserve vecotr capacity, fix for points within polygon Address review findings/nit --- expression-test/expression_test_parser.cpp | 15 +++++----- include/mbgl/style/expression/within.hpp | 2 +- src/mbgl/style/expression/within.cpp | 36 +++++++++--------------- src/mbgl/tile/geometry_tile_data.cpp | 45 +++++++++++++++++------------- 4 files changed, 46 insertions(+), 52 deletions(-) diff --git a/expression-test/expression_test_parser.cpp b/expression-test/expression_test_parser.cpp index 0d414cc2c0..9edc3b9866 100644 --- a/expression-test/expression_test_parser.cpp +++ b/expression-test/expression_test_parser.cpp @@ -259,14 +259,13 @@ bool parseInputs(const JSValue& inputsValue, TestData& data) { // Parse canonicalID optional canonical; if (evaluationContext.HasMember("canonicalID")) { - assert(evaluationContext["canonicalID"].IsObject()); - assert( - evaluationContext["canonicalID"].HasMember("z") && evaluationContext["canonicalID"]["z"].IsNumber() && - evaluationContext["canonicalID"].HasMember("x") && evaluationContext["canonicalID"]["x"].IsNumber() && - evaluationContext["canonicalID"].HasMember("y") && evaluationContext["canonicalID"]["y"].IsNumber()); - canonical = CanonicalTileID(evaluationContext["canonicalID"]["z"].GetUint(), - evaluationContext["canonicalID"]["x"].GetUint(), - evaluationContext["canonicalID"]["y"].GetUint()); + const auto& canonicalIDObject = evaluationContext["canonicalID"]; + assert(canonicalIDObject.IsObject()); + assert(canonicalIDObject.HasMember("z") && canonicalIDObject["z"].IsNumber()); + assert(canonicalIDObject.HasMember("x") && canonicalIDObject["x"].IsNumber()); + assert(canonicalIDObject.HasMember("y") && canonicalIDObject["y"].IsNumber()); + canonical = CanonicalTileID( + canonicalIDObject["z"].GetUint(), canonicalIDObject["x"].GetUint(), canonicalIDObject["y"].GetUint()); } // Parse availableImages diff --git a/include/mbgl/style/expression/within.hpp b/include/mbgl/style/expression/within.hpp index 5cc36ab19c..7e4050509d 100644 --- a/include/mbgl/style/expression/within.hpp +++ b/include/mbgl/style/expression/within.hpp @@ -15,7 +15,7 @@ class Within final : public Expression { public: explicit Within(GeoJSON geojson); - ~Within() final; + ~Within() override; EvaluationResult evaluate(const EvaluationContext&) const override; diff --git a/src/mbgl/style/expression/within.cpp b/src/mbgl/style/expression/within.cpp index a26f6fb7c8..0df0bd86ce 100644 --- a/src/mbgl/style/expression/within.cpp +++ b/src/mbgl/style/expression/within.cpp @@ -44,7 +44,7 @@ bool pointWithinPolygon(const mbgl::Point& point, const mapbox::geometry for (auto ring : polygon) { auto size = ring.size(); // loop through every edge of the ring - for (decltype(size) i = 0; i < size - 1; ++i) { + for (std::size_t i = 0; i < size - 1; ++i) { if (rayIntersect(point, ring[i], ring[i + 1])) { within = !within; } @@ -55,8 +55,7 @@ bool pointWithinPolygon(const mbgl::Point& point, const mapbox::geometry bool pointWithinPolygons(const mbgl::Point& point, const mapbox::geometry::multi_polygon& polygons) { for (auto polygon : polygons) { - auto within = pointWithinPolygon(point, polygon); - if (within) return true; + if (pointWithinPolygon(point, polygon)) return true; } return false; } @@ -77,14 +76,9 @@ bool pointsWithinPolygons(const mbgl::GeometryTileFeature& feature, return pointWithinPolygons(point, polygons); }, [&polygons](const mapbox::geometry::multi_point& points) -> bool { - auto result = false; - for (const auto& p : points) { - result = pointWithinPolygons(p, polygons); - if (!result) { - return result; - } - } - return result; + return std::all_of(points.begin(), points.end(), [&polygons](const auto& p) { + return pointWithinPolygons(p, polygons); + }); }, [](const auto&) -> bool { return false; }); }, @@ -95,14 +89,9 @@ bool pointsWithinPolygons(const mbgl::GeometryTileFeature& feature, return pointWithinPolygon(point, polygon); }, [&polygon](const mapbox::geometry::multi_point& points) -> bool { - auto result = false; - for (const auto& p : points) { - result = pointWithinPolygon(p, polygon); - if (!result) { - return result; - } - } - return result; + return std::all_of(points.begin(), points.end(), [&polygon](const auto& p) { + return pointWithinPolygon(p, polygon); + }); }, [](const auto&) -> bool { return false; }); }, @@ -138,7 +127,7 @@ namespace expression { Within::Within(GeoJSON geojson) : Expression(Kind::Within, type::Boolean), geoJSONSource(std::move(geojson)) {} -Within::~Within() {} +Within::~Within() = default; using namespace mbgl::style::conversion; @@ -150,15 +139,15 @@ EvaluationResult Within::evaluate(const EvaluationContext& params) const { // Currently only support Point/Points in Polygon/Polygons if (geometryType == FeatureType::Point) { return pointsWithinPolygons(*params.feature, *params.canonical, geoJSONSource); - } else { - mbgl::Log::Warning(mbgl::Event::General, "within expression currently only support 'Point' geometry type"); } + mbgl::Log::Warning(mbgl::Event::General, "within expression currently only support 'Point' geometry type"); + return false; } ParseResult Within::parse(const Convertible& value, ParsingContext& ctx) { if (isArray(value)) { - // object value, quoted with ["Within", value] + // object value, quoted with ["within", value] if (arrayLength(value) != 2) { ctx.error("'within' expression requires exactly one argument, but found " + util::toString(arrayLength(value) - 1) + " instead."); @@ -183,6 +172,7 @@ Value valueConverter(const mapbox::geojson::rapidjson_value& v) { } if (v.IsArray()) { std::vector result; + result.reserve(v.Size()); for (const auto& m : v.GetArray()) { result.push_back(valueConverter(m)); } diff --git a/src/mbgl/tile/geometry_tile_data.cpp b/src/mbgl/tile/geometry_tile_data.cpp index 56bef66c62..2f1c9e92fd 100644 --- a/src/mbgl/tile/geometry_tile_data.cpp +++ b/src/mbgl/tile/geometry_tile_data.cpp @@ -196,27 +196,27 @@ GeometryCollection convertGeometry(const Feature::geometry_type& geometryTileFea return geometryTileFeature.match( [&](const Point& point) -> GeometryCollection { return {{latLonToTileCoodinates(point)}}; }, [&](const MultiPoint& points) -> GeometryCollection { - GeometryCollection result; - std::vector> temp; + MultiPoint result; + result.reserve(points.size()); for (const auto p : points) { - temp.emplace_back(latLonToTileCoodinates(p)); + result.emplace_back(latLonToTileCoodinates(p)); } - result = {temp}; - return result; + return {std::move(result)}; }, [&](const LineString& lineString) -> GeometryCollection { - GeometryCollection result; - std::vector> temp; + LineString result; + result.reserve(lineString.size()); for (const auto p : lineString) { - temp.emplace_back(latLonToTileCoodinates(p)); + result.emplace_back(latLonToTileCoodinates(p)); } - result = {temp}; - return result; + return {std::move(result)}; }, [&](const MultiLineString& lineStrings) -> GeometryCollection { GeometryCollection result; + result.reserve(lineStrings.size()); for (const auto line : lineStrings) { - std::vector> temp; + LineString temp; + temp.reserve(line.size()); for (const auto p : line) { temp.emplace_back(latLonToTileCoodinates(p)); } @@ -226,9 +226,11 @@ GeometryCollection convertGeometry(const Feature::geometry_type& geometryTileFea }, [&](const Polygon polygon) -> GeometryCollection { GeometryCollection result; - for (const auto line : polygon) { - std::vector> temp; - for (const auto p : line) { + result.reserve(polygon.size()); + for (const auto ring : polygon) { + LinearRing temp; + temp.reserve(ring.size()); + for (const auto p : ring) { temp.emplace_back(latLonToTileCoodinates(p)); } result.emplace_back(temp); @@ -237,14 +239,17 @@ GeometryCollection convertGeometry(const Feature::geometry_type& geometryTileFea }, [&](const MultiPolygon polygons) -> GeometryCollection { GeometryCollection result; - for (const auto polygon : polygons) - for (const auto line : polygon) { - LineString temp; - for (const auto p : line) { - temp.emplace_back(latLonToTileCoodinates(p)); + result.reserve(polygons.size()); + for (const auto pg : polygons) { + for (const auto r : pg) { + LinearRing ring; + ring.reserve(r.size()); + for (const auto p : r) { + ring.emplace_back(latLonToTileCoodinates(p)); } - result.emplace_back(temp); + result.emplace_back(ring); } + } return result; }, [](const auto&) -> GeometryCollection { return GeometryCollection(); }); -- cgit v1.2.1