From 9bbb1b721e56e07666f6b0819594c4bfda84435e Mon Sep 17 00:00:00 2001 From: zmiao Date: Sat, 25 Apr 2020 01:12:45 +0300 Subject: [core] Distance expression: remove unit argument (#16434) --- include/mbgl/style/expression/distance.hpp | 3 +- src/mbgl/style/expression/distance.cpp | 117 ++++++----------------------- test/style/property_expression.test.cpp | 76 ------------------- 3 files changed, 26 insertions(+), 170 deletions(-) diff --git a/include/mbgl/style/expression/distance.hpp b/include/mbgl/style/expression/distance.hpp index e034b9452f..481162be03 100644 --- a/include/mbgl/style/expression/distance.hpp +++ b/include/mbgl/style/expression/distance.hpp @@ -10,7 +10,7 @@ namespace expression { class Distance final : public Expression { public: - Distance(GeoJSON geoJSONSource_, Feature::geometry_type geometries_, mapbox::cheap_ruler::CheapRuler::Unit unit_); + Distance(GeoJSON geoJSONSource_, Feature::geometry_type geometries_); ~Distance() override; @@ -30,7 +30,6 @@ public: private: GeoJSON geoJSONSource; Feature::geometry_type geometries; - mapbox::cheap_ruler::CheapRuler::Unit unit; }; } // namespace expression diff --git a/src/mbgl/style/expression/distance.cpp b/src/mbgl/style/expression/distance.cpp index 78d80dc649..9992ef5394 100644 --- a/src/mbgl/style/expression/distance.cpp +++ b/src/mbgl/style/expression/distance.cpp @@ -24,6 +24,7 @@ namespace { const std::size_t MinPointsSize = 100; const std::size_t MinLinePointsSize = 50; const double InvalidDistance = std::numeric_limits::quiet_NaN(); +const mapbox::cheap_ruler::CheapRuler::Unit UnitInMeters = mapbox::cheap_ruler::CheapRuler::Unit::Meters; // Inclusive index range for multipoint or linestring container using IndexRange = std::pair; @@ -327,9 +328,8 @@ double lineToLinesDistance(const mapbox::geometry::line_string& line, } double pointsToGeometryDistance(const mapbox::geometry::multi_point& points, - const Feature::geometry_type& geoSet, - mapbox::cheap_ruler::CheapRuler::Unit unit) { - mapbox::cheap_ruler::CheapRuler ruler(points.front().y, unit); + const Feature::geometry_type& geoSet) { + mapbox::cheap_ruler::CheapRuler ruler(points.front().y, UnitInMeters); return geoSet.match( [&points, &ruler](const mapbox::geometry::point& p) { return pointsToPointsDistance(mapbox::geometry::multi_point{p}, points, ruler); @@ -346,11 +346,9 @@ double pointsToGeometryDistance(const mapbox::geometry::multi_point& poi [](const auto&) { return InvalidDistance; }); } -double lineToGeometryDistance(const mapbox::geometry::line_string& line, - const Feature::geometry_type& geoSet, - mapbox::cheap_ruler::CheapRuler::Unit unit) { +double lineToGeometryDistance(const mapbox::geometry::line_string& line, const Feature::geometry_type& geoSet) { assert(!line.empty()); - mapbox::cheap_ruler::CheapRuler ruler(line.front().y, unit); + mapbox::cheap_ruler::CheapRuler ruler(line.front().y, UnitInMeters); return geoSet.match( [&line, &ruler](const mapbox::geometry::point& p) { return pointsToLineDistance(mapbox::geometry::multi_point{p}, line, ruler); @@ -369,23 +367,22 @@ double lineToGeometryDistance(const mapbox::geometry::line_string& line, double calculateDistance(const GeometryTileFeature& feature, const CanonicalTileID& canonical, - const Feature::geometry_type& geoSet, - mapbox::cheap_ruler::CheapRuler::Unit unit) { + const Feature::geometry_type& geoSet) { return convertGeometry(feature, canonical) .match( - [&geoSet, &unit](const mapbox::geometry::point& point) -> double { - return pointsToGeometryDistance(mapbox::geometry::multi_point{point}, geoSet, unit); + [&geoSet](const mapbox::geometry::point& point) -> double { + return pointsToGeometryDistance(mapbox::geometry::multi_point{point}, geoSet); }, - [&geoSet, &unit](const mapbox::geometry::multi_point& points) -> double { - return pointsToGeometryDistance(points, geoSet, unit); + [&geoSet](const mapbox::geometry::multi_point& points) -> double { + return pointsToGeometryDistance(points, geoSet); }, - [&geoSet, &unit](const mapbox::geometry::line_string& line) -> double { - return lineToGeometryDistance(line, geoSet, unit); + [&geoSet](const mapbox::geometry::line_string& line) -> double { + return lineToGeometryDistance(line, geoSet); }, - [&geoSet, &unit](const mapbox::geometry::multi_line_string& lines) -> double { + [&geoSet](const mapbox::geometry::multi_line_string& lines) -> double { double dist = std::numeric_limits::infinity(); for (const auto& line : lines) { - auto tempDist = lineToGeometryDistance(line, geoSet, unit); + auto tempDist = lineToGeometryDistance(line, geoSet); if (std::isnan(tempDist)) return tempDist; dist = std::min(dist, tempDist); if (dist == 0.0 || std::isnan(dist)) return dist; @@ -395,15 +392,7 @@ double calculateDistance(const GeometryTileFeature& feature, [](const auto&) -> double { return InvalidDistance; }); } -struct Arguments { - Arguments(GeoJSON& geojson_, mapbox::cheap_ruler::CheapRuler::Unit unit_) - : geojson(std::move(geojson_)), unit(unit_) {} - - GeoJSON geojson; - mapbox::cheap_ruler::CheapRuler::Unit unit; -}; - -optional parseValue(const style::conversion::Convertible& value, style::expression::ParsingContext& ctx) { +optional parseValue(const style::conversion::Convertible& value, style::expression::ParsingContext& ctx) { if (isArray(value)) { // object value, quoted with ["Distance", GeoJSONObj, "unit(optional)"] auto length = arrayLength(value); @@ -413,42 +402,13 @@ optional parseValue(const style::conversion::Convertible& value, styl return nullopt; } - // Parse Unit info for distance calculation, "Meters" by default - mapbox::cheap_ruler::CheapRuler::Unit unit = mapbox::cheap_ruler::CheapRuler::Unit::Meters; - if (length == 3) { - auto input = toString(arrayMember(value, 2)); - if (input == nullopt) { - ctx.error("Failed to parse unit argument from 'distance' expression"); - return nullopt; - } - if (*input == "meters" || *input == "metres") { - unit = mapbox::cheap_ruler::CheapRuler::Unit::Meters; - } else if (*input == "kilometers") { - unit = mapbox::cheap_ruler::CheapRuler::Unit::Kilometers; - } else if (*input == "miles") { - unit = mapbox::cheap_ruler::CheapRuler::Unit::Miles; - } else if (*input == "nauticalmiles") { - unit = mapbox::cheap_ruler::CheapRuler::Unit::NauticalMiles; - } else if (*input == "yards") { - unit = mapbox::cheap_ruler::CheapRuler::Unit::Yards; - } else if (*input == "feet") { - unit = mapbox::cheap_ruler::CheapRuler::Unit::Feet; - } else if (*input == "inches") { - unit = mapbox::cheap_ruler::CheapRuler::Unit::Inches; - } else { - ctx.error( - "'distance' expression only accepts following units: kilometers, miles, nauticalmiles, " - "meters, metres, yards, feet, inches."); - return nullopt; - } - } // Parse geometry info const auto& argument1 = arrayMember(value, 1); if (isObject(argument1)) { style::conversion::Error error; auto geojson = toGeoJSON(argument1, error); if (geojson && error.message.empty()) { - return Arguments(*geojson, unit); + return *geojson; } ctx.error(error.message); } @@ -472,11 +432,8 @@ optional getGeometry(const Feature& feature, mbgl::style namespace style { namespace expression { -Distance::Distance(GeoJSON geojson, Feature::geometry_type geometries_, mapbox::cheap_ruler::CheapRuler::Unit unit_) - : Expression(Kind::Distance, type::Number), - geoJSONSource(std::move(geojson)), - geometries(std::move(geometries_)), - unit(unit_) {} +Distance::Distance(GeoJSON geojson, Feature::geometry_type geometries_) + : Expression(Kind::Distance, type::Number), geoJSONSource(std::move(geojson)), geometries(std::move(geometries_)) {} Distance::~Distance() = default; @@ -488,7 +445,7 @@ EvaluationResult Distance::evaluate(const EvaluationContext& params) const { } auto geometryType = params.feature->getType(); if (geometryType == FeatureType::Point || geometryType == FeatureType::LineString) { - auto distance = calculateDistance(*params.feature, *params.canonical, geometries, unit); + auto distance = calculateDistance(*params.feature, *params.canonical, geometries); if (!std::isnan(distance)) { assert(distance >= 0.0); return distance; @@ -503,26 +460,23 @@ ParseResult Distance::parse(const Convertible& value, ParsingContext& ctx) { return ParseResult(); } - return parsedValue->geojson.match( + return parsedValue->match( [&parsedValue, &ctx](const mapbox::geometry::geometry& geometrySet) { if (auto ret = getGeometry(mbgl::Feature(geometrySet), ctx)) { - return ParseResult( - std::make_unique(parsedValue->geojson, std::move(*ret), parsedValue->unit)); + return ParseResult(std::make_unique(*parsedValue, std::move(*ret))); } return ParseResult(); }, [&parsedValue, &ctx](const mapbox::feature::feature& feature) { if (auto ret = getGeometry(mbgl::Feature(feature), ctx)) { - return ParseResult( - std::make_unique(parsedValue->geojson, std::move(*ret), parsedValue->unit)); + return ParseResult(std::make_unique(*parsedValue, std::move(*ret))); } return ParseResult(); }, [&parsedValue, &ctx](const mapbox::feature::feature_collection& features) { for (const auto& feature : features) { if (auto ret = getGeometry(mbgl::Feature(feature), ctx)) { - return ParseResult( - std::make_unique(parsedValue->geojson, std::move(*ret), parsedValue->unit)); + return ParseResult(std::make_unique(*parsedValue, std::move(*ret))); } } return ParseResult(); @@ -566,27 +520,6 @@ mbgl::Value convertValue(const mapbox::geojson::rapidjson_value& v) { return Null; } -std::string getUnits(const mapbox::cheap_ruler::CheapRuler::Unit& unit) { - switch (unit) { - case mapbox::cheap_ruler::CheapRuler::Kilometers: - return "kilometers"; - case mapbox::cheap_ruler::CheapRuler::Miles: - return "miles"; - case mapbox::cheap_ruler::CheapRuler::NauticalMiles: - return "nauticalmiles"; - case mapbox::cheap_ruler::CheapRuler::Meters: - return "meters"; - case mapbox::cheap_ruler::CheapRuler::Yards: - return "yards"; - case mapbox::cheap_ruler::CheapRuler::Feet: - return "feet"; - case mapbox::cheap_ruler::CheapRuler::Inches: - return "inches"; - default: - return "error"; - } -} - mbgl::Value Distance::serialize() const { std::unordered_map serialized; rapidjson::CrtAllocator allocator; @@ -599,13 +532,13 @@ mbgl::Value Distance::serialize() const { mbgl::Log::Error(mbgl::Event::General, "Failed to serialize 'distance' expression, converted rapidJSON is not an object"); } - return std::vector{{getOperator(), serialized, getUnits(unit)}}; + return std::vector{{getOperator(), serialized}}; } bool Distance::operator==(const Expression& e) const { if (e.getKind() == Kind::Distance) { auto rhs = static_cast(&e); - return geoJSONSource == rhs->geoJSONSource && geometries == rhs->geometries && unit == rhs->unit; + return geoJSONSource == rhs->geoJSONSource && geometries == rhs->geometries; } return false; } diff --git a/test/style/property_expression.test.cpp b/test/style/property_expression.test.cpp index 41f8238c70..924b0f91f6 100644 --- a/test/style/property_expression.test.cpp +++ b/test/style/property_expression.test.cpp @@ -550,82 +550,6 @@ TEST(PropertyExpression, DistanceExpression) { propExpr.evaluate(EvaluationContext(&pointFeature).withCanonicalTileID(&canonicalTileID), invalidResult); EXPECT_NEAR(491.307, evaluatedResult, 0.01); - // Unit: meters - ss.str(std::string()); - ss << std::string(R"(["distance", )") << pointGeoSource << std::string(R"(, "meters" ])"); - expression = createExpression(ss.str().c_str()); - ASSERT_TRUE(expression); - propExpr = std::move(expression); - - evaluatedResult = - propExpr.evaluate(EvaluationContext(&pointFeature).withCanonicalTileID(&canonicalTileID), invalidResult); - EXPECT_NEAR(491.307, evaluatedResult, 0.01); - - // Unit: kilometers - ss.str(std::string()); - ss << std::string(R"(["distance", )") << pointGeoSource << std::string(R"(, "kilometers" ])"); - expression = createExpression(ss.str().c_str()); - ASSERT_TRUE(expression); - propExpr = std::move(expression); - - evaluatedResult = - propExpr.evaluate(EvaluationContext(&pointFeature).withCanonicalTileID(&canonicalTileID), invalidResult); - EXPECT_NEAR(0.491307, evaluatedResult, 0.00001); - - // Unit: miles - ss.str(std::string()); - ss << std::string(R"(["distance", )") << pointGeoSource << std::string(R"(, "miles" ])"); - expression = createExpression(ss.str().c_str()); - ASSERT_TRUE(expression); - propExpr = std::move(expression); - - evaluatedResult = - propExpr.evaluate(EvaluationContext(&pointFeature).withCanonicalTileID(&canonicalTileID), invalidResult); - EXPECT_NEAR(0.305284, evaluatedResult, 0.00001); - - // Unit: nauticalmiles - ss.str(std::string()); - ss << std::string(R"(["distance", )") << pointGeoSource << std::string(R"(, "nauticalmiles" ])"); - expression = createExpression(ss.str().c_str()); - ASSERT_TRUE(expression); - propExpr = std::move(expression); - - evaluatedResult = - propExpr.evaluate(EvaluationContext(&pointFeature).withCanonicalTileID(&canonicalTileID), invalidResult); - EXPECT_NEAR(0.265284, evaluatedResult, 0.00001); - - // Unit: yards - ss.str(std::string()); - ss << std::string(R"(["distance", )") << pointGeoSource << std::string(R"(, "yards" ])"); - expression = createExpression(ss.str().c_str()); - ASSERT_TRUE(expression); - propExpr = std::move(expression); - - evaluatedResult = - propExpr.evaluate(EvaluationContext(&pointFeature).withCanonicalTileID(&canonicalTileID), invalidResult); - EXPECT_NEAR(537.299, evaluatedResult, 0.01); - - // Unit: feet - ss.str(std::string()); - ss << std::string(R"(["distance", )") << pointGeoSource << std::string(R"(, "feet" ])"); - expression = createExpression(ss.str().c_str()); - ASSERT_TRUE(expression); - propExpr = std::move(expression); - - evaluatedResult = - propExpr.evaluate(EvaluationContext(&pointFeature).withCanonicalTileID(&canonicalTileID), invalidResult); - EXPECT_NEAR(1611.898, evaluatedResult, 0.01); - - // Unit: inches - ss.str(std::string()); - ss << std::string(R"(["distance", )") << pointGeoSource << std::string(R"(, "inches" ])"); - expression = createExpression(ss.str().c_str()); - ASSERT_TRUE(expression); - propExpr = std::move(expression); - - evaluatedResult = - propExpr.evaluate(EvaluationContext(&pointFeature).withCanonicalTileID(&canonicalTileID), invalidResult); - EXPECT_NEAR(19342.781, evaluatedResult, 0.01); } // Evaluation test with Point to MultiPoint distance -- cgit v1.2.1