[core] Update unti tests. Remove unnecessary debugging code

5 files changed, 132 insertions, 177 deletions

diff --git a/include/mbgl/style/expression/within.hpp b/include/mbgl/style/expression/within.hpp
index 900e84296f..4116b60bc8 100644
--- a/include/mbgl/style/expression/within.hpp
+++ b/include/mbgl/style/expression/within.hpp
@@ -13,7 +13,7 @@ namespace expression {
 
 class Within final : public Expression {
 public:
-    explicit Within(GeoJSON geojson);
+    explicit Within(GeoJSON geojson, Feature::geometry_type geometries_);
 
     ~Within() override;
 
@@ -32,6 +32,7 @@ public:
 
 private:
     GeoJSON geoJSONSource;
+    Feature::geometry_type geometries;
 };
 
 } // namespace expression
diff --git a/src/mbgl/style/expression/within.cpp b/src/mbgl/style/expression/within.cpp
index c5582b4168..0447dfe228 100644
--- a/src/mbgl/style/expression/within.cpp
+++ b/src/mbgl/style/expression/within.cpp
@@ -5,9 +5,9 @@
 #include <mbgl/style/conversion_impl.hpp>
 #include <mbgl/tile/geometry_tile_data.hpp>
 
+#include <mbgl/util/geometry_within.hpp>
 #include <mbgl/util/logging.hpp>
 #include <mbgl/util/string.hpp>
-#include <mbgl/util/geometry_within.hpp>
 
 #include <rapidjson/document.h>
 #include <mbgl/style/conversion/json.hpp>
@@ -38,100 +38,64 @@ public:
 
 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 {
-            mbgl::Feature f(geometrySet);
-            PolygonFeature polyFeature(f, CanonicalTileID(0, 0, 0));
-            auto refinedGeoSet = convertGeometry(polyFeature, CanonicalTileID(0, 0, 0));
-            return refinedGeoSet.match(
-                [&feature, &canonical](const mapbox::geometry::multi_polygon<double>& polygons) -> bool {
-                    return convertGeometry(feature, canonical)
-                        .match(
-                            [&polygons](const mapbox::geometry::point<double>& point) -> bool {
-                                return pointWithinPolygons(point, polygons);
-                            },
-                            [&polygons](const mapbox::geometry::multi_point<double>& points) -> bool {
-                                return std::all_of(points.begin(), points.end(), [&polygons](const auto& p) {
-                                    return pointWithinPolygons(p, polygons);
-                                });
-                            },
-                            [](const auto&) -> bool {return false;});
-                },
-                [&feature, &canonical](const mapbox::geometry::polygon<double>& polygon) -> bool {
-                    return convertGeometry(feature, canonical)
-                        .match(
-                            [&polygon](const mapbox::geometry::point<double>& point) -> bool {
-                                return pointWithinPolygon(point, polygon);
-                            },
-                            [&polygon](const mapbox::geometry::multi_point<double>& points) -> bool {
-                                return std::all_of(points.begin(), points.end(), [&polygon](const auto& p) {
-                                    return pointWithinPolygon(p, polygon);
-                                });
-                            },
-                            [](const auto&) -> bool { return false; });
-                },
-                [](const auto&) -> bool { return false; });
+                          const Feature::geometry_type& polygonGeoSet) {
+    return polygonGeoSet.match(
+        [&feature, &canonical](const mapbox::geometry::multi_polygon<double>& polygons) -> bool {
+            return convertGeometry(feature, canonical)
+                .match(
+                    [&polygons](const mapbox::geometry::point<double>& point) -> bool {
+                        return pointWithinPolygons(point, polygons);
+                    },
+                    [&polygons](const mapbox::geometry::multi_point<double>& points) -> bool {
+                        return std::all_of(points.begin(), points.end(), [&polygons](const auto& p) {
+                            return pointWithinPolygons(p, polygons);
+                        });
+                    },
+                    [](const auto&) -> bool { return false; });
+        },
+        [&feature, &canonical](const mapbox::geometry::polygon<double>& polygon) -> bool {
+            return convertGeometry(feature, canonical)
+                .match(
+                    [&polygon](const mapbox::geometry::point<double>& point) -> bool {
+                        return pointWithinPolygon(point, polygon);
+                    },
+                    [&polygon](const mapbox::geometry::multi_point<double>& points) -> bool {
+                        return std::all_of(points.begin(), points.end(), [&polygon](const auto& p) {
+                            return pointWithinPolygon(p, polygon);
+                        });
+                    },
+                    [](const auto&) -> bool { return false; });
         },
         [](const auto&) -> bool { return false; });
 }
 
-
-
 bool linesWithinPolygons(const mbgl::GeometryTileFeature& feature,
                          const mbgl::CanonicalTileID& canonical,
-                         const mbgl::GeoJSON& geoJson) {
-    mbgl::Log::Debug(mbgl::Event::General, "------------------------Canonical ID is-------------------------------");
-    mbgl::Log::Debug(
-        mbgl::Event::General, "----- '%d' / '%d' / '%d'-------------", canonical.z, canonical.x, canonical.y);
-
-    return geoJson.match(
-        [&feature, &canonical](const mapbox::geometry::geometry<double>& geometrySet) -> bool {
-            mbgl::Feature f(geometrySet);
-            PolygonFeature polyFeature(f, CanonicalTileID(0, 0, 0));
-            auto refinedGeoSet = convertGeometry(polyFeature, CanonicalTileID(0, 0, 0));
-            return refinedGeoSet.match(
-                [&feature, &canonical](const MultiPolygon<double>& polygons) -> bool {
-                    return convertGeometry(feature, canonical)
-                        .match(
-                            [&polygons](const LineString<double>& line) -> bool {
-                                printLine(line);
-                                return lineStringWithinPolygons(line, polygons);
-                            },
-                            [&polygons](const MultiLineString<double>& lines) -> bool {
-                                return std::all_of(lines.begin(), lines.end(), [&polygons](const auto& line) {
-                                    printLine(line);
-                                    return lineStringWithinPolygons(line, polygons);
-                                });
-                            },
-                            [](const auto&) -> bool {
-                                return false;
-                            });
-                },
-                [&feature, &canonical](const Polygon<double>& polygon) -> bool {
-                    return convertGeometry(feature, canonical)
-                        .match(
-                            [&polygon](const LineString<double>& line) -> bool {
-                                printLine(line);
-                                return lineStringWithinPolygon(line, polygon);
-                            },
-                            [&polygon](const MultiLineString<double>& lines) -> bool {
-                                return std::all_of(lines.begin(), lines.end(), [&polygon](const auto& line) {
-                                    printLine(line);
-                                    return lineStringWithinPolygon(line, polygon);
-                                });
-                            },
-                            [](const auto&) -> bool {
-                                return false;
-                            });
-                },
-                [](const auto&) -> bool {
-                    return false;
-                });
+                         const Feature::geometry_type& polygonGeoSet) {
+    return polygonGeoSet.match(
+        [&feature, &canonical](const MultiPolygon<double>& polygons) -> bool {
+            return convertGeometry(feature, canonical)
+                .match([&polygons](
+                           const LineString<double>& line) -> bool { return lineStringWithinPolygons(line, polygons); },
+                       [&polygons](const MultiLineString<double>& lines) -> bool {
+                           return std::all_of(lines.begin(), lines.end(), [&polygons](const auto& line) {
+                               return lineStringWithinPolygons(line, polygons);
+                           });
+                       },
+                       [](const auto&) -> bool { return false; });
+        },
+        [&feature, &canonical](const Polygon<double>& polygon) -> bool {
+            return convertGeometry(feature, canonical)
+                .match([&polygon](
+                           const LineString<double>& line) -> bool { return lineStringWithinPolygon(line, polygon); },
+                       [&polygon](const MultiLineString<double>& lines) -> bool {
+                           return std::all_of(lines.begin(), lines.end(), [&polygon](const auto& line) {
+                               return lineStringWithinPolygon(line, polygon);
+                           });
+                       },
+                       [](const auto&) -> bool { return false; });
         },
-        [](const auto&) -> bool {
-            return false;
-        });
+        [](const auto&) -> bool { return false; });
 }
 
 mbgl::optional<mbgl::GeoJSON> parseValue(const mbgl::style::conversion::Convertible& value_,
@@ -150,7 +114,7 @@ mbgl::optional<mbgl::GeoJSON> parseValue(const mbgl::style::conversion::Converti
             }
         }
     }
-    ctx.error("'Within' expression requires valid geojson source that contains polygon geometry type.");
+    ctx.error("'within' expression requires valid geojson source that contains polygon geometry type.");
     return nullopt;
 }
 
@@ -159,7 +123,8 @@ mbgl::optional<mbgl::GeoJSON> parseValue(const mbgl::style::conversion::Converti
 namespace style {
 namespace expression {
 
-Within::Within(GeoJSON geojson) : Expression(Kind::Within, type::Boolean), geoJSONSource(std::move(geojson)) {}
+Within::Within(GeoJSON geojson, Feature::geometry_type geometries_)
+    : Expression(Kind::Within, type::Boolean), geoJSONSource(std::move(geojson)), geometries(std::move(geometries_)) {}
 
 Within::~Within() = default;
 
@@ -172,11 +137,12 @@ EvaluationResult Within::evaluate(const EvaluationContext& params) const {
     auto geometryType = params.feature->getType();
     // Currently only support Point/Points in Polygon/Polygons
     if (geometryType == FeatureType::Point) {
-        return pointsWithinPolygons(*params.feature, *params.canonical, geoJSONSource);
+        return pointsWithinPolygons(*params.feature, *params.canonical, geometries);
     } else if (geometryType == FeatureType::LineString) {
-        return linesWithinPolygons(*params.feature, *params.canonical, geoJSONSource);
+        return linesWithinPolygons(*params.feature, *params.canonical, geometries);
     }
-    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/LineString geometry type.");
 
     return false;
 }
@@ -194,8 +160,20 @@ ParseResult Within::parse(const Convertible& value, ParsingContext& ctx) {
         if (!parsedValue) {
             return ParseResult();
         }
-        return ParseResult(std::make_unique<Within>(*parsedValue));
+
+        return parsedValue->match(
+            [&parsedValue](const mapbox::geometry::geometry<double>& geometrySet) {
+                mbgl::Feature f(geometrySet);
+                PolygonFeature polyFeature(f, CanonicalTileID(0, 0, 0));
+                auto refinedGeoSet = convertGeometry(polyFeature, CanonicalTileID(0, 0, 0));
+                return ParseResult(std::make_unique<Within>(*parsedValue, refinedGeoSet));
+            },
+            [&ctx](const auto&) {
+                ctx.error("'within' expression requires geojson source that contains valid geometry data.");
+                return ParseResult();
+            });
     }
+    ctx.error("'within' expression needs to be an array with exactly one argument.");
     return ParseResult();
 }
 
diff --git a/src/mbgl/util/geometry_within.cpp b/src/mbgl/util/geometry_within.cpp
index 0d60a5622b..bb62047077 100644
--- a/src/mbgl/util/geometry_within.cpp
+++ b/src/mbgl/util/geometry_within.cpp
@@ -8,51 +8,41 @@ bool rayIntersect(const Point<double>& p, const Point<double>& p1, const Point<d
     return ((p1.y > p.y) != (p2.y > p.y)) && (p.x < (p2.x - p1.x) * (p.y - p1.y) / (p2.y - p1.y) + p1.x);
 }
 
-// a, b are end point for line segment1, c and d are end points for line segment2
-bool lineIntersectLine(const Point<double>& a,
-                       const Point<double>& b,
-                       const Point<double>& c,
-                       const Point<double>& d) {
-    const auto perp = [](const Point<double>& v1, const Point<double>& v2) {
-        return (v1.x * v2.y - v1.y * v2.x);
-    };
-
-    // check if p1 and p2 are in different sides of line segment q1->q2
-    const auto twoSided = [](const Point<double>& p1,
-                             const Point<double>& p2,
-                             const Point<double>& q1,
-                             const Point<double>& q2) {
-        double x1, y1, x2, y2, x3, y3;
-
-        // q1->p1 (x1, y1), q1->p2 (x2, y2), q1->q2 (x3, y3)
-        x1 = p1.x - q1.x;
-        y1 = p1.y - q1.y;
-        x2 = p2.x - q1.x;
-        y2 = p2.y - q1.y;
-        x3 = q2.x - q1.x;
-        y3 = q2.y - q1.y;
-        if ((x1 * y3 - x3 * y1) * (x2 * y3 - x3 * y2) < 0) return true;
-        return false;
-    };
+// a, b are end points for line segment1, c and d are end points for line segment2
+bool lineIntersectLine(const Point<double>& a, const Point<double>& b, const Point<double>& c, const Point<double>& d) {
+    const auto perp = [](const Point<double>& v1, const Point<double>& v2) { return (v1.x * v2.y - v1.y * v2.x); };
 
+    // check if two segments are parallel or not
+    // precondition is end point a, b is inside polygon, if line a->b is
+    // parallel to polygon edge c->d, then a->b won't intersect with c->d
     auto vectorP = Point<double>(b.x - a.x, b.y - a.y);
     auto vectorQ = Point<double>(d.x - c.x, d.y - c.y);
-
-    // check if two segments are parallel or not
-    // precondition is end point a, b is inside polygon, if they line a->b is
-    // parallel to edge c->d, then a->b won't intersect with c->d
     if (perp(vectorQ, vectorP) == 0) return false;
 
-    // check if there are intersecting with each other
-    // a and b lie in separate side of segment c->d
-    // c and d lie in separate side of segment a->b
+    // check if p1 and p2 are in different sides of line segment q1->q2
+    const auto twoSided =
+        [](const Point<double>& p1, const Point<double>& p2, const Point<double>& q1, const Point<double>& q2) {
+            double x1, y1, x2, y2, x3, y3;
+
+            // q1->p1 (x1, y1), q1->p2 (x2, y2), q1->q2 (x3, y3)
+            x1 = p1.x - q1.x;
+            y1 = p1.y - q1.y;
+            x2 = p2.x - q1.x;
+            y2 = p2.y - q1.y;
+            x3 = q2.x - q1.x;
+            y3 = q2.y - q1.y;
+            if ((x1 * y3 - x3 * y1) * (x2 * y3 - x3 * y2) < 0) return true;
+            return false;
+        };
+
+    // If lines are intersecting with each other, the relative location should be:
+    // a and b lie in different sides of segment c->d
+    // c and d lie in different sides of segment a->b
     if (twoSided(a, b, c, d) && twoSided(c, d, a, b)) return true;
     return false;
 }
 
-bool lineIntersectPolygon(const Point<double>& p1,
-                          const Point<double>& p2,
-                          const Polygon<double>& polygon) {
+bool lineIntersectPolygon(const Point<double>& p1, const Point<double>& p2, const Polygon<double>& polygon) {
     for (auto ring : polygon) {
         auto length = ring.size();
         // loop through every edge of the ring
@@ -64,26 +54,7 @@ bool lineIntersectPolygon(const Point<double>& p1,
     }
     return false;
 }
-}
-
-void printPolygon(const Polygon<double>& polygon) {
-    mbgl::Log::Debug(mbgl::Event::General, "-------Print Polygon------");
-    for (auto ring : polygon) {
-        auto length = ring.size();
-        mbgl::Log::Debug(mbgl::Event::General, "-------Print New ring with size: %d ------", length);
-        // loop through every edge of the ring
-        for (std::size_t i = 0; i < length - 1; ++i) {
-            mbgl::Log::Debug(mbgl::Event::General, "point [%.15f, %.15f],", ring[i].x, ring[i].y);
-        }
-    }
-}
-
-void printLine(const LineString<double>& lineString) {
-    mbgl::Log::Debug(mbgl::Event::General, "-------Print LineString------");
-    for (std::size_t i = 0; i < lineString.size(); ++i) {
-        mbgl::Log::Debug(mbgl::Event::General, "point [%.15f, %.15f],", lineString[i].x, lineString[i].y);
-    }
-}
+} // namespace
 
 // ray casting algorithm for detecting if point is in polygon
 bool pointWithinPolygon(const Point<double>& point, const Polygon<double>& polygon) {
@@ -107,14 +78,11 @@ bool pointWithinPolygons(const Point<double>& point, const MultiPolygon<double>&
     return false;
 }
 
-
-bool lineStringWithinPolygon(const LineString<double>& line,
-                             const Polygon<double>& polygon) {
+bool lineStringWithinPolygon(const LineString<double>& line, const Polygon<double>& polygon) {
     const auto length = line.size();
-    // First, check if endpoints of line are all inside polygon
+    // First, check if geometry points of line segments are all inside polygon
     for (std::size_t i = 0; i < length; ++i) {
         if (!pointWithinPolygon(line[i], polygon)) {
-            mbgl::Log::Debug(mbgl::Event::General, "point %.15f, %.15f is not inside polygon", line[i].x, line[i].y);
             return false;
         }
     }
@@ -122,12 +90,6 @@ bool lineStringWithinPolygon(const LineString<double>& line,
     // Second, check if there is line segment intersecting polygon edge
     for (std::size_t i = 0; i < length - 1; ++i) {
         if (lineIntersectPolygon(line[i], line[i + 1], polygon)) {
-            mbgl::Log::Debug(mbgl::Event::General,
-            "---------line [%.15f, %.15f] to [%.15f, %.15f] is not inside polygon",
-            line[i].x,
-            line[i].y,
-            line[i + 1].x,
-            line[i + 1].y);
             return false;
         }
     }
@@ -140,4 +102,4 @@ bool lineStringWithinPolygons(const LineString<double>& line, const MultiPolygon
     }
     return false;
 }
-}
+} // namespace mbgl
diff --git a/src/mbgl/util/geometry_within.hpp b/src/mbgl/util/geometry_within.hpp
index fe9312cd4a..86658ecc83 100644
--- a/src/mbgl/util/geometry_within.hpp
+++ b/src/mbgl/util/geometry_within.hpp
@@ -3,17 +3,11 @@
 
 namespace mbgl {
 
-void printPolygon(const Polygon<double>& polygon);
-
-void printLine(const LineString<double>& lineString);
-
-// ray casting algorithm for detecting if point is in polygon
 bool pointWithinPolygon(const Point<double>& point, const Polygon<double>& polygon);
 
 bool pointWithinPolygons(const Point<double>& point, const MultiPolygon<double>& polygons);
 
-bool lineStringWithinPolygon(const LineString<double>& lineString,
-                             const Polygon<double>& polygon);
+bool lineStringWithinPolygon(const LineString<double>& lineString, const Polygon<double>& polygon);
 
 bool lineStringWithinPolygons(const LineString<double>& line, const MultiPolygon<double>& polygons);
-}
+} // namespace mbgl
diff --git a/test/style/property_expression.test.cpp b/test/style/property_expression.test.cpp
index 0624cff385..672cff44f5 100644
--- a/test/style/property_expression.test.cpp
+++ b/test/style/property_expression.test.cpp
@@ -265,15 +265,35 @@ TEST(PropertyExpression, WithinExpression) {
     ASSERT_TRUE(expression);
     PropertyExpression<bool> propExpr(std::move(expression));
 
-    // evaluation test with valid geojson source but FeatureType is not Point (currently only support
+    // evaluation test with valid geojson source but FeatureType is not Point/LineString (currently only support
     // FeatureType::Point)
     {
-        // testLine is inside polygon, but will return false
-        LineString<double> testLine{{-9.228515625, -17.560246503294888}, {-2.4609375, -16.04581345375217}};
-        auto geoLine = convertGeometry(testLine, canonicalTileID);
-        StubGeometryTileFeature lineFeature(FeatureType::LineString, geoLine);
+        // testPoly is inside polygon, but will return false
+        Polygon<double> testRing{{{-9.228515625, -17.560246503294888},
+                                  {-2.4609375, -16.04581345375217},
+                                  {-9.228515625, -17.560246503294888}}};
+        auto geoPoly = convertGeometry(testRing, canonicalTileID);
+        StubGeometryTileFeature polyFeature(FeatureType::Polygon, geoPoly);
+
+        auto evaluatedResult = propExpr.evaluate(EvaluationContext(&polyFeature).withCanonicalTileID(&canonicalTileID));
+        EXPECT_FALSE(evaluatedResult);
+    }
+    // evaluation test with valid geojson source and valid linestring features
+    {
+        // testLine is inside polygon, but will return true
+        LineString<double> testLine0{{-9.228515625, -17.560246503294888}, {-2.4609375, -16.04581345375217}};
+        auto geoLine0 = convertGeometry(testLine0, canonicalTileID);
+        StubGeometryTileFeature lineFeature0(FeatureType::LineString, geoLine0);
 
-        auto evaluatedResult = propExpr.evaluate(EvaluationContext(&lineFeature).withCanonicalTileID(&canonicalTileID));
+        // testLine is intersecting polygon even though end points are all inside polygon, but will return false
+        LineString<double> testLine1{{-10.4150390625, -10.082445532162465}, {-8.8275146484375, -9.194292714912638}};
+        auto geoLine1 = convertGeometry(testLine1, canonicalTileID);
+        StubGeometryTileFeature lineFeature1(FeatureType::LineString, geoLine1);
+
+        auto evaluatedResult =
+            propExpr.evaluate(EvaluationContext(&lineFeature0).withCanonicalTileID(&canonicalTileID));
+        EXPECT_TRUE(evaluatedResult);
+        evaluatedResult = propExpr.evaluate(EvaluationContext(&lineFeature1).withCanonicalTileID(&canonicalTileID));
         EXPECT_FALSE(evaluatedResult);
     }