From 0f34eb7b253e83b8c4aef7ed6c83cd7b3801fa4c Mon Sep 17 00:00:00 2001 From: zmiao Date: Fri, 16 Aug 2019 15:03:48 +0300 Subject: Indroduce clusterProperty option for aggregation (#15287) * indroduce clusterProperty option for aggregation * remove unnecessary codes * update geojson_option conversion * fix reviewing findings --- include/mbgl/annotation/annotation.hpp | 1 - include/mbgl/style/expression/dsl.hpp | 29 +++++----- include/mbgl/style/expression/expression.hpp | 6 +- include/mbgl/style/sources/geojson_source.hpp | 13 ++++- platform/node/test/ignores.json | 1 - src/mbgl/style/conversion/geojson_options.cpp | 69 ++++++++++++++++++++++- src/mbgl/style/expression/compound_expression.cpp | 13 +++++ src/mbgl/style/expression/dsl.cpp | 23 ++++++++ src/mbgl/style/expression/expression.cpp | 8 ++- src/mbgl/style/expression/parsing_context.cpp | 3 +- src/mbgl/style/sources/geojson_source.cpp | 2 +- src/mbgl/style/sources/geojson_source_impl.cpp | 69 ++++++++++++++++------- src/mbgl/style/sources/geojson_source_impl.hpp | 2 +- test/style/conversion/geojson_options.test.cpp | 12 +++- test/style/expression/expression.test.cpp | 3 +- vendor/supercluster.hpp | 2 +- 16 files changed, 205 insertions(+), 51 deletions(-) diff --git a/include/mbgl/annotation/annotation.hpp b/include/mbgl/annotation/annotation.hpp index fb9ea5eba2..17728741bb 100644 --- a/include/mbgl/annotation/annotation.hpp +++ b/include/mbgl/annotation/annotation.hpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include diff --git a/include/mbgl/style/expression/dsl.hpp b/include/mbgl/style/expression/dsl.hpp index bcab999ab2..4abeac7989 100644 --- a/include/mbgl/style/expression/dsl.hpp +++ b/include/mbgl/style/expression/dsl.hpp @@ -1,13 +1,13 @@ #pragma once -#include #include #include +#include #include +#include #include #include -#include namespace mbgl { namespace style { @@ -24,15 +24,18 @@ std::vector> vec(Args... args) { return result; } +std::unique_ptr createExpression(const char* expr); +std::unique_ptr createExpression(const mbgl::style::conversion::Convertible& expr); std::unique_ptr error(std::string); std::unique_ptr literal(const char* value); std::unique_ptr literal(Value value); std::unique_ptr literal(std::initializer_list value); -std::unique_ptr literal(std::initializer_list value); +std::unique_ptr literal(std::initializer_list value); -std::unique_ptr assertion(type::Type, std::unique_ptr, - std::unique_ptr def = nullptr); +std::unique_ptr +assertion(type::Type, std::unique_ptr, + std::unique_ptr def = nullptr); std::unique_ptr number(std::unique_ptr, std::unique_ptr def = nullptr); std::unique_ptr string(std::unique_ptr, @@ -46,21 +49,17 @@ std::unique_ptr toString(std::unique_ptr, std::unique_ptr def = nullptr); std::unique_ptr toFormatted(std::unique_ptr, std::unique_ptr def = nullptr); - + std::unique_ptr get(const char* value); std::unique_ptr get(std::unique_ptr); std::unique_ptr id(); std::unique_ptr zoom(); -std::unique_ptr eq(std::unique_ptr, - std::unique_ptr); -std::unique_ptr ne(std::unique_ptr, - std::unique_ptr); -std::unique_ptr gt(std::unique_ptr, - std::unique_ptr); -std::unique_ptr lt(std::unique_ptr, - std::unique_ptr); +std::unique_ptr eq(std::unique_ptr, std::unique_ptr); +std::unique_ptr ne(std::unique_ptr, std::unique_ptr); +std::unique_ptr gt(std::unique_ptr, std::unique_ptr); +std::unique_ptr lt(std::unique_ptr, std::unique_ptr); std::unique_ptr step(std::unique_ptr input, std::unique_ptr output0, @@ -86,7 +85,7 @@ std::unique_ptr interpolate(Interpolator interpolator, double input3, std::unique_ptr output3); std::unique_ptr concat(std::vector> inputs); - + std::unique_ptr format(const char* value); std::unique_ptr format(std::unique_ptr); diff --git a/include/mbgl/style/expression/expression.hpp b/include/mbgl/style/expression/expression.hpp index 5f66fc6dc7..ad57748677 100644 --- a/include/mbgl/style/expression/expression.hpp +++ b/include/mbgl/style/expression/expression.hpp @@ -31,6 +31,9 @@ public: EvaluationContext(float zoom_, GeometryTileFeature const * feature_) : zoom(zoom_), feature(feature_) {} + EvaluationContext(optional accumulated_, GeometryTileFeature const * feature_) : + accumulated(std::move(accumulated_)), feature(feature_) + {} EvaluationContext(optional zoom_, GeometryTileFeature const * feature_, optional colorRampParameter_) : zoom(std::move(zoom_)), feature(feature_), colorRampParameter(std::move(colorRampParameter_)) {} @@ -41,6 +44,7 @@ public: }; optional zoom; + optional accumulated; GeometryTileFeature const * feature = nullptr; optional colorRampParameter; // Contains formatted section object, std::unordered_map. @@ -162,7 +166,7 @@ public: type::Type getType() const { return type; }; EvaluationResult evaluate(optional zoom, const Feature& feature, optional colorRampParameter) const; - + EvaluationResult evaluate(optional accumulated, const Feature& feature) const; /** * Statically analyze the expression, attempting to enumerate possible outputs. Returns * an array of values plus the sentinel null optional value, used to indicate that the diff --git a/include/mbgl/style/sources/geojson_source.hpp b/include/mbgl/style/sources/geojson_source.hpp index a03b910279..4aec1584a6 100644 --- a/include/mbgl/style/sources/geojson_source.hpp +++ b/include/mbgl/style/sources/geojson_source.hpp @@ -1,9 +1,14 @@ #pragma once +#include #include +#include #include #include -#include + +#include +#include +#include namespace mbgl { @@ -24,11 +29,15 @@ struct GeoJSONOptions { bool cluster = false; uint16_t clusterRadius = 50; uint8_t clusterMaxZoom = 17; + using ClusterExpression = std::pair, + std::shared_ptr>; + using ClusterProperties = std::unordered_map; + ClusterProperties clusterProperties; }; class GeoJSONSource : public Source { public: - GeoJSONSource(const std::string& id, const GeoJSONOptions& = {}); + GeoJSONSource(const std::string& id, optional = nullopt); ~GeoJSONSource() final; void setURL(const std::string& url); diff --git a/platform/node/test/ignores.json b/platform/node/test/ignores.json index 7665972aad..bc2535227f 100644 --- a/platform/node/test/ignores.json +++ b/platform/node/test/ignores.json @@ -85,7 +85,6 @@ "render-tests/text-variable-anchor/pitched-rotated-debug": "https://github.com/mapbox/mapbox-gl-native/issues/14211", "render-tests/text-variable-anchor/rotated-offset": "https://github.com/mapbox/mapbox-gl-native/issues/14211", "render-tests/text-variable-anchor/remember-last-placement": "skip - fails on gl-native, as symbol index is not functional at static map mode - needs issue", - "render-tests/geojson/clustered-properties": "https://github.com/mapbox/mapbox-gl-native/issues/14043", "render-tests/remove-feature-state/composite-expression": "https://github.com/mapbox/mapbox-gl-native/issues/12413", "render-tests/remove-feature-state/data-expression": "https://github.com/mapbox/mapbox-gl-native/issues/12413", "render-tests/remove-feature-state/vector-source": "https://github.com/mapbox/mapbox-gl-native/issues/12413", diff --git a/src/mbgl/style/conversion/geojson_options.cpp b/src/mbgl/style/conversion/geojson_options.cpp index 11bd7cc507..08553e34bb 100644 --- a/src/mbgl/style/conversion/geojson_options.cpp +++ b/src/mbgl/style/conversion/geojson_options.cpp @@ -1,11 +1,15 @@ #include #include +#include + +#include namespace mbgl { namespace style { namespace conversion { -optional Converter::operator()(const Convertible& value, Error& error) const { +optional Converter::operator()(const Convertible& value, + Error& error) const { GeoJSONOptions options; const auto minzoomValue = objectMember(value, "minzoom"); @@ -83,12 +87,71 @@ optional Converter::operator()(const Convertible if (toBool(*lineMetricsValue)) { options.lineMetrics = *toBool(*lineMetricsValue); } else { - error = { "GeoJSON source lineMetrics value must be a boolean" }; + error.message = "GeoJSON source lineMetrics value must be a boolean"; + return nullopt; + } + } + + const auto clusterProperties = objectMember(value, "clusterProperties"); + if (clusterProperties) { + if (!isObject(*clusterProperties)) { + error.message = "GeoJSON source clusterProperties value must be an object"; + return nullopt; + } + GeoJSONOptions::ClusterProperties result; + assert(error.message.empty()); + eachMember( + *clusterProperties, + [&](const std::string& k, + const mbgl::style::conversion::Convertible& v) -> optional { + // Each property shall be formed as ["key" : [operator, [mapExpression]]] + // or ["key" : [[operator, ['accumulated'], ['get', key]], [mapExpression]]] + if (!isArray(v) || arrayLength(v) != 2) { + error.message = + "GeoJSON source clusterProperties member must be an array with length of 2"; + return nullopt; + } + auto map = expression::dsl::createExpression(arrayMember(v, 1)); + if (!map) { + error.message = + "Failed to convert GeoJSON source clusterProperties map expression"; + return nullopt; + } + std::unique_ptr reduce; + if (isArray(arrayMember(v, 0))) { + reduce = expression::dsl::createExpression(arrayMember(v, 0)); + } else { + auto reduceOp = toString(arrayMember(v, 0)); + if (!reduceOp) { + error.message = + "GeoJSON source clusterProperties member must contain a valid operator"; + return nullopt; + } + std::stringstream ss; + // Reformulate reduce expression to [operator, ['accumulated'], ['get', key]] + // The reason to create expression via parsing string instead of invoking function + // createCompoundExpression is due to expression type disunity can’t be resolved + // with current logic of createCompoundExpression + ss << std::string(R"([")") << *reduceOp + << std::string(R"(", ["accumulated"], ["get", ")") << k + << std::string(R"("]])"); + reduce = expression::dsl::createExpression(ss.str().c_str()); + } + if (!reduce) { + error.message = + "Failed to convert GeoJSON source clusterProperties reduce expression"; + return nullopt; + } + result.emplace(k, std::make_pair(std::move(map), std::move(reduce))); + return nullopt; + }); + if (!error.message.empty()) { return nullopt; } + options.clusterProperties = std::move(result); } - return { options }; + return { std::move(options) }; } } // namespace conversion diff --git a/src/mbgl/style/expression/compound_expression.cpp b/src/mbgl/style/expression/compound_expression.cpp index 34f0f5dea3..c637856ad9 100644 --- a/src/mbgl/style/expression/compound_expression.cpp +++ b/src/mbgl/style/expression/compound_expression.cpp @@ -368,6 +368,18 @@ const auto& lineProgressCompoundExpression() { return signature; } +const auto& accumulatedCompoundExpression() { + const static auto signature = detail::makeSignature("accumulated", [](const EvaluationContext& params) -> Result { + if (!params.accumulated) { + return EvaluationError { + "The 'accumulated' expression is unavailable in the current evaluation context." + }; + } + return Value(toExpressionValue(*params.accumulated)); + }); + return signature; +} + const auto& hasContextCompoundExpression() { static auto signature = detail::makeSignature("has", [](const EvaluationContext& params, const std::string& key) -> Result { if (!params.feature) { @@ -871,6 +883,7 @@ MAPBOX_ETERNAL_CONSTEXPR const auto compoundExpressionRegistry = mapbox::eternal { "zoom", zoomCompoundExpression }, { "heatmap-density", heatmapDensityCompoundExpression }, { "line-progress", lineProgressCompoundExpression }, + { "accumulated", accumulatedCompoundExpression }, { "has", hasContextCompoundExpression }, { "has", hasObjectCompoundExpression }, { "get", getContextCompoundExpression }, diff --git a/src/mbgl/style/expression/dsl.cpp b/src/mbgl/style/expression/dsl.cpp index 70442de968..fac7dfbbd9 100644 --- a/src/mbgl/style/expression/dsl.cpp +++ b/src/mbgl/style/expression/dsl.cpp @@ -10,6 +10,10 @@ #include #include +#include +#include +#include + namespace mbgl { namespace style { namespace expression { @@ -21,6 +25,25 @@ std::unique_ptr compound(const char* op, std::vector createExpression(const char* expr) { + using JSValue = rapidjson::GenericValue, rapidjson::CrtAllocator>; + rapidjson::GenericDocument, rapidjson::CrtAllocator> document; + document.Parse<0>(expr); + if (document.HasParseError()) return nullptr; + + const JSValue* expression = &document; + expression::ParsingContext ctx; + expression::ParseResult parsed = + ctx.parseExpression(mbgl::style::conversion::Convertible(expression)); + return parsed ? std::move(*parsed) : nullptr; +} + +std::unique_ptr createExpression(const mbgl::style::conversion::Convertible& expr) { + expression::ParsingContext ctx; + expression::ParseResult parsed = ctx.parseExpression(expr); + return parsed ? std::move(*parsed) : nullptr; +} std::unique_ptr error(std::string message) { return std::make_unique(std::move(message)); diff --git a/src/mbgl/style/expression/expression.cpp b/src/mbgl/style/expression/expression.cpp index d61f435eec..6bfda99064 100644 --- a/src/mbgl/style/expression/expression.cpp +++ b/src/mbgl/style/expression/expression.cpp @@ -25,13 +25,17 @@ public: return optional(); } }; - - + EvaluationResult Expression::evaluate(optional zoom, const Feature& feature, optional colorRampParameter) const { GeoJSONFeature f(feature); return this->evaluate(EvaluationContext(zoom, &f, colorRampParameter)); } +EvaluationResult Expression::evaluate(optional accumulated, const Feature& feature) const { + GeoJSONFeature f(feature); + return this->evaluate(EvaluationContext(accumulated, &f)); +} + } // namespace expression } // namespace style } // namespace mbgl diff --git a/src/mbgl/style/expression/parsing_context.cpp b/src/mbgl/style/expression/parsing_context.cpp index a7c04f563d..6ce3a9bfaa 100644 --- a/src/mbgl/style/expression/parsing_context.cpp +++ b/src/mbgl/style/expression/parsing_context.cpp @@ -73,7 +73,8 @@ bool isConstant(const Expression& expression) { return isFeatureConstant(expression) && isGlobalPropertyConstant(expression, std::array{{"zoom", "heatmap-density"}}) && - isGlobalPropertyConstant(expression, std::array{{"zoom", "line-progress"}}); + isGlobalPropertyConstant(expression, std::array{{"zoom", "line-progress"}}) && + isGlobalPropertyConstant(expression, std::array{{"zoom", "accumulated"}}); } using namespace mbgl::style::conversion; diff --git a/src/mbgl/style/sources/geojson_source.cpp b/src/mbgl/style/sources/geojson_source.cpp index 4e3478322d..72a51e212f 100644 --- a/src/mbgl/style/sources/geojson_source.cpp +++ b/src/mbgl/style/sources/geojson_source.cpp @@ -9,7 +9,7 @@ namespace mbgl { namespace style { -GeoJSONSource::GeoJSONSource(const std::string& id, const GeoJSONOptions& options) +GeoJSONSource::GeoJSONSource(const std::string& id, optional options) : Source(makeMutable(std::move(id), options)) { } diff --git a/src/mbgl/style/sources/geojson_source_impl.cpp b/src/mbgl/style/sources/geojson_source_impl.cpp index 24ac1d7976..c3cb942709 100644 --- a/src/mbgl/style/sources/geojson_source_impl.cpp +++ b/src/mbgl/style/sources/geojson_source_impl.cpp @@ -1,6 +1,7 @@ #include -#include #include +#include +#include #include #include @@ -13,9 +14,9 @@ namespace style { class GeoJSONVTData : public GeoJSONData { public: - GeoJSONVTData(const GeoJSON& geoJSON, - const mapbox::geojsonvt::Options& options) - : impl(geoJSON, options) {} + GeoJSONVTData(const GeoJSON& geoJSON, const mapbox::geojsonvt::Options& options) + : impl(geoJSON, options) { + } mapbox::feature::feature_collection getTile(const CanonicalTileID& tileID) final { return impl.getTile(tileID.z, tileID.x, tileID.y).features; @@ -25,9 +26,8 @@ public: return {}; } - mapbox::feature::feature_collection getLeaves(const std::uint32_t, - const std::uint32_t, - const std::uint32_t) final { + mapbox::feature::feature_collection + getLeaves(const std::uint32_t, const std::uint32_t, const std::uint32_t) final { return {}; } @@ -43,7 +43,8 @@ class SuperclusterData : public GeoJSONData { public: SuperclusterData(const mapbox::feature::feature_collection& features, const mapbox::supercluster::Options& options) - : impl(features, options) {} + : impl(features, options) { + } mapbox::feature::feature_collection getTile(const CanonicalTileID& tileID) final { return impl.getTile(tileID.z, tileID.x, tileID.y); @@ -54,8 +55,8 @@ public: } mapbox::feature::feature_collection getLeaves(const std::uint32_t cluster_id, - const std::uint32_t limit, - const std::uint32_t offset) final { + const std::uint32_t limit, + const std::uint32_t offset) final { return impl.getLeaves(cluster_id, limit, offset); } @@ -67,23 +68,53 @@ private: mapbox::supercluster::Supercluster impl; }; -GeoJSONSource::Impl::Impl(std::string id_, GeoJSONOptions options_) - : Source::Impl(SourceType::GeoJSON, std::move(id_)), - options(std::move(options_)) { +template +T evaluateFeature(const mapbox::feature::feature& f, + const std::shared_ptr& expression, + optional accumulated = nullopt) { + const expression::EvaluationResult result = expression->evaluate(accumulated, f); + if (result) { + optional typed = expression::fromExpressionValue(*result); + if (typed) { + return std::move(*typed); + } + } + return T(); +} + +GeoJSONSource::Impl::Impl(std::string id_, optional options_) + : Source::Impl(SourceType::GeoJSON, std::move(id_)) { + options = options_ ? std::move(*options_) : GeoJSONOptions{}; } GeoJSONSource::Impl::Impl(const Impl& other, const GeoJSON& geoJSON) - : Source::Impl(other), - options(other.options) { + : Source::Impl(other), options(other.options) { constexpr double scale = util::EXTENT / util::tileSize; - - if (options.cluster - && geoJSON.is>() - && !geoJSON.get>().empty()) { + if (options.cluster && geoJSON.is>() && + !geoJSON.get>().empty()) { mapbox::supercluster::Options clusterOptions; clusterOptions.maxZoom = options.clusterMaxZoom; clusterOptions.extent = util::EXTENT; clusterOptions.radius = ::round(scale * options.clusterRadius); + Feature feature; + clusterOptions.map = [&](const PropertyMap& properties) -> PropertyMap { + PropertyMap ret{}; + for (const auto& p : options.clusterProperties) { + feature.properties = properties; + ret[p.first] = evaluateFeature(feature, p.second.first); + } + return ret; + }; + clusterOptions.reduce = [&](PropertyMap& toReturn, const PropertyMap& toFill) { + for (const auto& p : options.clusterProperties) { + if (toFill.count(p.first) == 0) { + continue; + } + feature.properties = toFill; + optional accumulated(toReturn[p.first]); + toReturn[p.first] = evaluateFeature(feature, p.second.second, accumulated); + } + }; data = std::make_shared( geoJSON.get>(), clusterOptions); } else { diff --git a/src/mbgl/style/sources/geojson_source_impl.hpp b/src/mbgl/style/sources/geojson_source_impl.hpp index b88ab35ee0..26b9d95a39 100644 --- a/src/mbgl/style/sources/geojson_source_impl.hpp +++ b/src/mbgl/style/sources/geojson_source_impl.hpp @@ -26,7 +26,7 @@ public: class GeoJSONSource::Impl : public Source::Impl { public: - Impl(std::string id, GeoJSONOptions); + Impl(std::string id, optional); Impl(const GeoJSONSource::Impl&, const GeoJSON&); ~Impl() final; diff --git a/test/style/conversion/geojson_options.test.cpp b/test/style/conversion/geojson_options.test.cpp index 181189775b..aa84686dce 100644 --- a/test/style/conversion/geojson_options.test.cpp +++ b/test/style/conversion/geojson_options.test.cpp @@ -38,6 +38,7 @@ TEST(GeoJSONOptions, RetainsDefaults) { ASSERT_EQ(converted.cluster, defaults.cluster); ASSERT_EQ(converted.clusterRadius, defaults.clusterRadius); ASSERT_EQ(converted.clusterMaxZoom, defaults.clusterMaxZoom); + ASSERT_TRUE(converted.clusterProperties.empty()); } TEST(GeoJSONOptions, FullConversion) { @@ -49,7 +50,12 @@ TEST(GeoJSONOptions, FullConversion) { "cluster": true, "clusterRadius": 4, "clusterMaxZoom": 5, - "lineMetrics": true + "lineMetrics": true, + "clusterProperties": { + "max": ["max", ["get", "scalerank"]], + "sum": [["+", ["accumulated"], ["get", "sum"]], ["get", "scalerank"]], + "has_island": ["any", ["==", ["get", "featureclass"], "island"]] + } })JSON", error); // GeoJSON-VT @@ -63,4 +69,8 @@ TEST(GeoJSONOptions, FullConversion) { ASSERT_EQ(converted.cluster, true); ASSERT_EQ(converted.clusterRadius, 4); ASSERT_EQ(converted.clusterMaxZoom, 5); + ASSERT_EQ(converted.clusterProperties.size(), 3); + ASSERT_EQ(converted.clusterProperties.count("max"), 1); + ASSERT_EQ(converted.clusterProperties.count("sum"), 1); + ASSERT_EQ(converted.clusterProperties.count("has_island"), 1); } diff --git a/test/style/expression/expression.test.cpp b/test/style/expression/expression.test.cpp index 8e2acfcd32..dd986c98f5 100644 --- a/test/style/expression/expression.test.cpp +++ b/test/style/expression/expression.test.cpp @@ -36,8 +36,7 @@ TEST(Expression, IsExpression) { // TODO: "feature-state": https://github.com/mapbox/mapbox-gl-native/issues/12613 // TODO: "interpolate-hcl": https://github.com/mapbox/mapbox-gl-native/issues/8720 // TODO: "interpolate-lab": https://github.com/mapbox/mapbox-gl-native/issues/8720 - // TODO: "accumulated": https://github.com/mapbox/mapbox-gl-native/issues/14043 - if (name == "feature-state" || name == "interpolate-hcl" || name == "interpolate-lab" || name == "accumulated") { + if (name == "feature-state" || name == "interpolate-hcl" || name == "interpolate-lab") { if (expression::isExpression(conversion::Convertible(expression))) { ASSERT_TRUE(false) << "Expression name" << name << "is implemented - please update Expression.IsExpression test."; } diff --git a/vendor/supercluster.hpp b/vendor/supercluster.hpp index 274ec13830..03c026c49c 160000 --- a/vendor/supercluster.hpp +++ b/vendor/supercluster.hpp @@ -1 +1 @@ -Subproject commit 274ec138306c7b110bf4dde47706aeb43dc8147e +Subproject commit 03c026c49c3e25cb4c65f91a308ab869e98f6499 -- cgit v1.2.1