Enable within expression with Filter

add canonical as pointer

fix review findings

8 files changed, 54 insertions, 39 deletions

diff --git a/include/mbgl/style/expression/expression.hpp b/include/mbgl/style/expression/expression.hpp
index f19f64c1e7..1e34a8bd38 100644
--- a/include/mbgl/style/expression/expression.hpp
+++ b/include/mbgl/style/expression/expression.hpp
@@ -54,20 +54,20 @@ public:
         return *this;
     };
 
-    EvaluationContext& withCanonicalTileID(const mbgl::CanonicalTileID& canonical_) noexcept {
+    EvaluationContext& withCanonicalTileID(const mbgl::CanonicalTileID* canonical_) noexcept {
         canonical = canonical_;
         return *this;
     };
 
     optional<float> zoom;
     optional<mbgl::Value> accumulated;
-    optional<mbgl::CanonicalTileID> canonical;
-    GeometryTileFeature const * feature = nullptr;
+    GeometryTileFeature const* feature = nullptr;
     optional<double> colorRampParameter;
     // Contains formatted section object, std::unordered_map<std::string, Value>.
     const Value* formattedSection = nullptr;
     const FeatureState* featureState = nullptr;
     const std::set<std::string>* availableImages = nullptr;
+    const mbgl::CanonicalTileID* canonical = nullptr;
 };
 
 template <typename T>
diff --git a/include/mbgl/style/expression/within.hpp b/include/mbgl/style/expression/within.hpp
index 5e2b5e0671..88e9cc56b8 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 c6815e4cad..81ff3996db 100644
--- a/src/mbgl/layout/pattern_layout.hpp
+++ b/src/mbgl/layout/pattern_layout.hpp
@@ -108,7 +108,9 @@ public:
         const size_t featureCount = sourceLayer->featureCount();
         for (size_t i = 0; i < featureCount; ++i) {
             auto feature = sourceLayer->getFeature(i);
-            if (!leaderLayerProperties->layerImpl().filter(style::expression::EvaluationContext { this->zoom, feature.get() }))
+            if (!leaderLayerProperties->layerImpl().filter(
+                    style::expression::EvaluationContext(this->zoom, feature.get())
+                        .withCanonicalTileID(&parameters.tileID.canonical)))
                 continue;
 
             PatternLayerMap patternDependencyMap;
@@ -155,8 +157,8 @@ public:
     void createBucket(const ImagePositions& patternPositions,
                       std::unique_ptr<FeatureIndex>& featureIndex,
                       std::unordered_map<std::string, LayerRenderData>& renderData,
-                      const bool,
-                      const bool,
+                      const bool /*firstLoad*/,
+                      const bool /*showCollisionBoxes*/,
                       const CanonicalTileID& canonical) override {
         auto bucket = std::make_shared<BucketType>(layout, layerPropertiesMap, zoom, overscaling);
         for (auto & patternFeature : features) {
diff --git a/src/mbgl/layout/symbol_layout.cpp b/src/mbgl/layout/symbol_layout.cpp
index a4890c85d1..e954a26a50 100644
--- a/src/mbgl/layout/symbol_layout.cpp
+++ b/src/mbgl/layout/symbol_layout.cpp
@@ -132,7 +132,8 @@ SymbolLayout::SymbolLayout(const BucketParameters& parameters,
     const size_t featureCount = sourceLayer->featureCount();
     for (size_t i = 0; i < featureCount; ++i) {
         auto feature = sourceLayer->getFeature(i);
-        if (!leader.filter(expression::EvaluationContext { this->zoom, feature.get() }))
+        if (!leader.filter(expression::EvaluationContext(this->zoom, feature.get())
+                               .withCanonicalTileID(&parameters.tileID.canonical)))
             continue;
 
         SymbolFeature ft(std::move(feature));
diff --git a/src/mbgl/renderer/paint_property_binder.hpp b/src/mbgl/renderer/paint_property_binder.hpp
index 3a9dceacc0..61544be82d 100644
--- a/src/mbgl/renderer/paint_property_binder.hpp
+++ b/src/mbgl/renderer/paint_property_binder.hpp
@@ -227,7 +227,7 @@ public:
                               const style::expression::Value& formattedSection) override {
         using style::expression::EvaluationContext;
         auto evaluated = expression.evaluate(
-            EvaluationContext(&feature).withFormattedSection(&formattedSection).withCanonicalTileID(canonical),
+            EvaluationContext(&feature).withFormattedSection(&formattedSection).withCanonicalTileID(&canonical),
             defaultValue);
         this->statistics.add(evaluated);
         auto value = attributeValue(evaluated);
@@ -330,11 +330,11 @@ public:
         Range<T> range = {
             expression.evaluate(EvaluationContext(zoomRange.min, &feature)
                                     .withFormattedSection(&formattedSection)
-                                    .withCanonicalTileID(canonical),
+                                    .withCanonicalTileID(&canonical),
                                 defaultValue),
             expression.evaluate(EvaluationContext(zoomRange.max, &feature)
                                     .withFormattedSection(&formattedSection)
-                                    .withCanonicalTileID(canonical),
+                                    .withCanonicalTileID(&canonical),
                                 defaultValue),
         };
         this->statistics.add(range.min);
diff --git a/src/mbgl/style/conversion/filter.cpp b/src/mbgl/style/conversion/filter.cpp
index 1b77985322..f3e7050ccc 100644
--- a/src/mbgl/style/conversion/filter.cpp
+++ b/src/mbgl/style/conversion/filter.cpp
@@ -207,6 +207,9 @@ ParseResult convertLegacyFilter(const Convertible& values, Error& error) {
         return nullopt;
     } else if (arrayLength(values) <= 1) {
         return {std::make_unique<Literal>(*op != "any")};
+    } else if (*op == "within") {
+        expression::ParsingContext ctx;
+        return ctx.parseExpression(values);
     } else {
         return {
             *op == "==" ||
diff --git a/src/mbgl/style/expression/within.cpp b/src/mbgl/style/expression/within.cpp
index 1b1b8b11b9..d02ea47cb2 100644
--- a/src/mbgl/style/expression/within.cpp
+++ b/src/mbgl/style/expression/within.cpp
@@ -1,10 +1,13 @@
+#include <mbgl/style/expression/within.hpp>
+
 #include <mapbox/geojson.hpp>
 #include <mapbox/geometry.hpp>
 #include <mbgl/style/conversion_impl.hpp>
-#include <mbgl/style/expression/within.hpp>
 #include <mbgl/tile/geometry_tile_data.hpp>
+#include <mbgl/util/logging.hpp>
 #include <mbgl/util/string.hpp>
 
+namespace mbgl {
 namespace {
 
 double isLeft(mbgl::Point<double> P0, mbgl::Point<double> P1, mbgl::Point<double> P2) {
@@ -13,14 +16,13 @@ double isLeft(mbgl::Point<double> P0, mbgl::Point<double> P1, mbgl::Point<double
 
 // winding number algorithm for checking if point inside a ploygon or not.
 // http://geomalgorithms.com/a03-_inclusion.html#wn_PnPoly()
-bool pointWithinPoly(mbgl::Point<double> point, const mapbox::geometry::polygon<double>& polys) {
+bool pointWithinPolygons(mbgl::Point<double> point, const mapbox::geometry::polygon<double>& 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<double> 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<double>& geometrySet) {
-            geometrySet.match(
-                [&](const mapbox::geometry::polygon<double>& 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<double>& geometrySet) -> bool {
+            return geometrySet.match(
+                [&feature, &canonical](const mapbox::geometry::polygon<double>& polygons) -> bool {
+                    return convertGeometry(feature, canonical)
                         .match(
-                            [&](const mapbox::geometry::point<double>& point) {
-                                result = pointWithinPoly(point, polygons);
+                            [&polygons](const mapbox::geometry::point<double>& point) -> bool {
+                                return pointWithinPolygons(point, polygons);
                             },
-                            [&](const mapbox::geometry::multi_point<double>& points) {
+                            [&polygons](const mapbox::geometry::multi_point<double>& 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<mbgl::GeoJSON> parseValue(const mbgl::style::conversion::Convertible& value_,
@@ -89,15 +90,18 @@ mbgl::optional<mbgl::GeoJSON> parseValue(const mbgl::style::conversion::Converti
         }
     }
     ctx.error("'Within' expression requires valid geojson source that contains polygon geometry type.");
-    return mbgl::optional<mbgl::GeoJSON>();
+    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;
 }
diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp
index 0729ed2aa5..409dfdf428 100644
--- a/src/mbgl/tile/geometry_tile_worker.cpp
+++ b/src/mbgl/tile/geometry_tile_worker.cpp
@@ -401,7 +401,8 @@ void GeometryTileWorker::parse() {
             for (std::size_t i = 0; !obsolete && i < geometryLayer->featureCount(); i++) {
                 std::unique_ptr<GeometryTileFeature> feature = geometryLayer->getFeature(i);
 
-                if (!filter(expression::EvaluationContext { static_cast<float>(this->id.overscaledZ), feature.get() }))
+                if (!filter(expression::EvaluationContext(static_cast<float>(this->id.overscaledZ), feature.get())
+                                .withCanonicalTileID(&id.canonical)))
                     continue;
 
                 const GeometryCollection& geometries = feature->getGeometries();