[core] Fix composite function approximation for non-integer stops

4 files changed, 83 insertions, 27 deletions

diff --git a/include/mbgl/style/function/composite_function.hpp b/include/mbgl/style/function/composite_function.hpp
index 1b785f4670..c3de0726c0 100644
--- a/include/mbgl/style/function/composite_function.hpp
+++ b/include/mbgl/style/function/composite_function.hpp
@@ -49,8 +49,16 @@ public:
           defaultValue(std::move(defaultValue_)) {
     }
 
-    std::tuple<Range<float>, Range<InnerStops>>
-    coveringRanges(float zoom) const {
+    struct CoveringRanges {
+        float zoom;
+        Range<float> coveringZoomRange;
+        Range<InnerStops> coveringStopsRange;
+    };
+
+    // Return the relevant stop zoom values and inner stops that bracket a given zoom level. This
+    // is the first step toward evaluating the function, and is used for in the course of both partial
+    // evaluation of data-driven paint properties, and full evaluation of data-driven layout properties.
+    CoveringRanges coveringRanges(float zoom) const {
         return stops.match(
             [&] (const auto& s) {
                 assert(!s.stops.empty());
@@ -63,7 +71,8 @@ public:
                     minIt--;
                 }
                 
-                return std::make_tuple(
+                return CoveringRanges {
+                    zoom,
                     Range<float> {
                         minIt == s.stops.end() ? s.stops.rbegin()->first : minIt->first,
                         maxIt == s.stops.end() ? s.stops.rbegin()->first : maxIt->first
@@ -72,38 +81,49 @@ public:
                         s.innerStops(minIt == s.stops.end() ? s.stops.rbegin()->second : minIt->second),
                         s.innerStops(maxIt == s.stops.end() ? s.stops.rbegin()->second : maxIt->second)
                     }
-                );
+                };
             }
         );
     }
 
+    // Given a range of zoom values (typically two adjacent integer zoom levels, e.g. 5.0 and 6.0),
+    // return the covering ranges for both. This is used in the course of partial evaluation for
+    // data-driven paint properties.
+    Range<CoveringRanges> rangeOfCoveringRanges(Range<float> zoomRange) {
+        return Range<CoveringRanges> {
+            coveringRanges(zoomRange.min),
+            coveringRanges(zoomRange.max)
+        };
+    }
+
+    // Given the covering ranges for range of zoom values (typically two adjacent integer zoom levels,
+    // e.g. 5.0 and 6.0), and a feature, return the results of fully evaluating the function for that
+    // feature at each of the two zoom levels. These two results are what go into the paint vertex buffers
+    // for vertices associated with this feature. The shader will interpolate between them at render time.
     template <class Feature>
-    Range<T> evaluate(Range<InnerStops> coveringStops,
-                      const Feature& feature,
-                      T finalDefaultValue) const {
-        optional<Value> v = feature.getValue(property);
-        if (!v) {
-            return {
+    Range<T> evaluate(const Range<CoveringRanges>& ranges, const Feature& feature, T finalDefaultValue) {
+        optional<Value> value = feature.getValue(property);
+        if (!value) {
+            return Range<T> {
                 defaultValue.value_or(finalDefaultValue),
                 defaultValue.value_or(finalDefaultValue)
             };
         }
-        auto eval = [&] (const auto& s) {
-            return s.evaluate(*v).value_or(defaultValue.value_or(finalDefaultValue));
-        };
         return Range<T> {
-            coveringStops.min.match(eval),
-            coveringStops.max.match(eval)
+            evaluateFinal(ranges.min, *value, finalDefaultValue),
+            evaluateFinal(ranges.max, *value, finalDefaultValue)
         };
     }
 
-    T evaluate(float zoom, const GeometryTileFeature& feature, T finalDefaultValue) const {
-        std::tuple<Range<float>, Range<InnerStops>> ranges = coveringRanges(zoom);
-        Range<T> resultRange = evaluate(std::get<1>(ranges), feature, finalDefaultValue);
-        return util::interpolate(
-            resultRange.min,
-            resultRange.max,
-            util::interpolationFactor(1.0f, std::get<0>(ranges), zoom));
+    // Fully evaluate the function for a zoom value and feature. This is used when evaluating data-driven
+    // layout properties.
+    template <class Feature>
+    T evaluate(float zoom, const Feature& feature, T finalDefaultValue) const {
+        optional<Value> value = feature.getValue(property);
+        if (!value) {
+            return defaultValue.value_or(finalDefaultValue);
+        }
+        return evaluateFinal(coveringRanges(zoom), *value, finalDefaultValue);
     }
 
     friend bool operator==(const CompositeFunction& lhs,
@@ -115,6 +135,17 @@ public:
     std::string property;
     Stops stops;
     optional<T> defaultValue;
+
+private:
+    T evaluateFinal(const CoveringRanges& ranges, const Value& value, T finalDefaultValue) const {
+        auto eval = [&] (const auto& s) {
+            return s.evaluate(value).value_or(defaultValue.value_or(finalDefaultValue));
+        };
+        return util::interpolate(
+            ranges.coveringStopsRange.min.match(eval),
+            ranges.coveringStopsRange.max.match(eval),
+            util::interpolationFactor(1.0f, ranges.coveringZoomRange, ranges.zoom));
+    }
 };
 
 } // namespace style
diff --git a/mapbox-gl-js b/mapbox-gl-js
-Subproject 24938a6e6767c880c709ee6ffb2d3ffbaad838c
+Subproject a222ef16b5a34b15a52e7c722dcdfceafdc62af
diff --git a/src/mbgl/renderer/paint_property_binder.hpp b/src/mbgl/renderer/paint_property_binder.hpp
index c22291767a..8ac6ce8cf1 100644
--- a/src/mbgl/renderer/paint_property_binder.hpp
+++ b/src/mbgl/renderer/paint_property_binder.hpp
@@ -189,11 +189,11 @@ public:
     CompositeFunctionPaintPropertyBinder(style::CompositeFunction<T> function_, float zoom, T defaultValue_)
         : function(std::move(function_)),
           defaultValue(std::move(defaultValue_)),
-          coveringRanges(function.coveringRanges(zoom)) {
+          rangeOfCoveringRanges(function.rangeOfCoveringRanges({zoom, zoom + 1})) {
     }
 
     void populateVertexVector(const GeometryTileFeature& feature, std::size_t length) override {
-        Range<T> range = function.evaluate(std::get<1>(coveringRanges), feature, defaultValue);
+        Range<T> range = function.evaluate(rangeOfCoveringRanges, feature, defaultValue);
         this->statistics.add(range.min);
         this->statistics.add(range.max);
         AttributeValue value = zoomInterpolatedAttributeValue(
@@ -217,7 +217,7 @@ public:
     }
 
     float interpolationFactor(float currentZoom) const override {
-        return util::interpolationFactor(1.0f, std::get<0>(coveringRanges), currentZoom);
+        return util::interpolationFactor(1.0f, { rangeOfCoveringRanges.min.zoom, rangeOfCoveringRanges.max.zoom }, currentZoom);
     }
 
     T uniformValue(const PossiblyEvaluatedPropertyValue<T>& currentValue) const override {
@@ -230,10 +230,10 @@ public:
     }
 
 private:
-    using InnerStops = typename style::CompositeFunction<T>::InnerStops;
     style::CompositeFunction<T> function;
     T defaultValue;
-    std::tuple<Range<float>, Range<InnerStops>> coveringRanges;
+    using CoveringRanges = typename style::CompositeFunction<T>::CoveringRanges;
+    Range<CoveringRanges> rangeOfCoveringRanges;
     gl::VertexVector<Vertex> vertexVector;
     optional<gl::VertexBuffer<Vertex>> vertexBuffer;
 };
diff --git a/test/style/function/composite_function.test.cpp b/test/style/function/composite_function.test.cpp
index e0804d4b27..09b79a8b8f 100644
--- a/test/style/function/composite_function.test.cpp
+++ b/test/style/function/composite_function.test.cpp
@@ -44,3 +44,28 @@ TEST(CompositeFunction, ZoomInterpolation) {
     }), 0.0f)
     .evaluate(0.0f, oneInteger, -1.0f)) << "Should interpolate TO the first stop";
 }
+
+TEST(CompositeFunction, Issue8460) {
+    CompositeFunction<float> fn1("property", CompositeExponentialStops<float>({
+        {15.0f, {{uint64_t(1), 0.0f}}},
+        {15.2f, {{uint64_t(1), 600.0f}}},
+    }), 0.0f);
+
+    EXPECT_NEAR(  0.0f, fn1.evaluate(15.0f, oneInteger, -1.0f), 0.00);
+    EXPECT_NEAR(300.0f, fn1.evaluate(15.1f, oneInteger, -1.0f), 0.01);
+    EXPECT_NEAR(600.0f, fn1.evaluate(15.2f, oneInteger, -1.0f), 0.00);
+    EXPECT_NEAR(600.0f, fn1.evaluate(16.0f, oneInteger, -1.0f), 0.00);
+
+    CompositeFunction<float> fn2("property", CompositeExponentialStops<float>({
+        {15.0f, {{uint64_t(1), 0.0f}}},
+        {15.2f, {{uint64_t(1), 300.0f}}},
+        {18.0f, {{uint64_t(1), 600.0f}}},
+    }), 0.0f);
+
+    EXPECT_NEAR(  0.0f, fn2.evaluate(15.0f, oneInteger, -1.0f), 0.00);
+    EXPECT_NEAR(150.0f, fn2.evaluate(15.1f, oneInteger, -1.0f), 0.01);
+    EXPECT_NEAR(300.0f, fn2.evaluate(15.2f, oneInteger, -1.0f), 0.00);
+    EXPECT_NEAR(385.71f, fn2.evaluate(16.0f, oneInteger, -1.0f), 0.01);
+    EXPECT_NEAR(600.0f, fn2.evaluate(18.0f, oneInteger, -1.0f), 0.00);
+    EXPECT_NEAR(600.0f, fn2.evaluate(19.0f, oneInteger, -1.0f), 0.00);
+}