Disallow stop functions and zoom expressions in heatmap-color (#5625)

* Disallow stop functions in heatmap-color Fixes #5623 * Respect property spec 'zoom-function' in createExpression Fixes #5622 * Add validation test for heatmap-color as a stop function * Fixup: remove unused name parameter in createFunction()
mapbox · Nov 9, 2017 · d947033 · d947033
1 parent 3b7187a
commit d947033
Show file tree

Hide file tree

Showing 16 changed files with 85 additions and 97 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,8 @@
+# master
+
+### :warning: Breaking changes
+- Require `heatmap-color` property to use expressions (not stop functions) [#5624](https://github.com/mapbox/mapbox-gl-js/issues/5624)
+
 # 0.41.0 (October 11, 2017)
 
 ### :warning: Breaking changes

diff --git a/docs/pages/example/heatmap-layer.html b/docs/pages/example/heatmap-layer.html
@@ -43,16 +43,17 @@
             //Color ramp for heatmap.  Domain is 0 (low) to 1 (high).
             //Begin color ramp at 0-stop with a 0-transparancy color
             //to create a blur-like effect.
-            "heatmap-color": {
-                "stops": [
-                    [0, "rgba(33,102,172,0)"],
-                    [0.2, "rgb(103,169,207)"],
-                    [0.4, "rgb(209,229,240)"],
-                    [0.6, "rgb(253,219,199)"],
-                    [0.8, "rgb(239,138,98)"],
-                    [1, "rgb(178,24,43)"]
-                ]
-            },
+            "heatmap-color": [
+                "interpolate",
+                ["linear"],
+                ["heatmap-density"],
+                0, "rgba(33,102,172,0)",
+                0.2, "rgb(103,169,207)",
+                0.4, "rgb(209,229,240)",
+                0.6, "rgb(253,219,199)",
+                0.8, "rgb(239,138,98)",
+                1, "rgb(178,24,43)"
+            ],
             //Adjust the heatmap radius by zoom level
             "heatmap-radius": {
                 "stops": [

diff --git a/src/style-spec/expression/index.js b/src/style-spec/expression/index.js
@@ -144,6 +144,13 @@ function createExpression(expression: mixed,
     }
 
     const isZoomConstant = isConstant.isGlobalPropertyConstant(parsed, ['zoom']);
+    if (!isZoomConstant && context === 'property' && propertySpec['zoom-function'] === false) {
+        return {
+            result: 'error',
+            errors: [new ParsingError('', 'zoom expressions not supported')]
+        };
+    }
+
     if (isZoomConstant) {
         return {
             result: 'success',

diff --git a/src/style-spec/feature_filter/index.js b/src/style-spec/feature_filter/index.js
@@ -50,7 +50,8 @@ const filterSpec = {
     'type': 'boolean',
     'default': false,
     'function': true,
-    'property-function': true
+    'property-function': true,
+    'zoom-function': true
 };
 
 /**

diff --git a/src/style-spec/function/convert.js b/src/style-spec/function/convert.js
@@ -34,10 +34,7 @@ function convertFunction(parameters, propertySpec, name) {
             throw new Error('Unimplemented');
         }
 
-        if (name === 'heatmap-color') {
-            assert(zoomDependent);
-            expression = convertZoomFunction(parameters, propertySpec, stops, ['heatmap-density']);
-        } else if (zoomAndFeatureDependent) {
+        if (zoomAndFeatureDependent) {
             expression = convertZoomAndPropertyFunction(parameters, propertySpec, stops, defaultExpression);
         } else if (zoomDependent) {
             expression = convertZoomFunction(parameters, propertySpec, stops);

diff --git a/src/style-spec/function/index.js b/src/style-spec/function/index.js
@@ -14,7 +14,7 @@ function identityFunction(x) {
     return x;
 }
 
-function createFunction(parameters, propertySpec, name) {
+function createFunction(parameters, propertySpec) {
     const isColor = propertySpec.type === 'color';
     const zoomAndFeatureDependent = parameters.stops && typeof parameters.stops[0][0] === 'object';
     const featureDependent = zoomAndFeatureDependent || parameters.property !== undefined;
@@ -123,20 +123,14 @@ function createFunction(parameters, propertySpec, name) {
             }
         };
     } else if (zoomDependent) {
-        let evaluate;
-        if (name === 'heatmap-color') {
-            evaluate = ({heatmapDensity}) => outputFunction(innerFun(parameters, propertySpec, heatmapDensity, hashedStops, categoricalKeyType));
-        } else {
-            evaluate = ({zoom}) => outputFunction(innerFun(parameters, propertySpec, zoom, hashedStops, categoricalKeyType));
-        }
         return {
             isFeatureConstant: true,
             isZoomConstant: false,
             interpolationFactor: type === 'exponential' ?
                 Interpolate.interpolationFactor.bind(undefined, {name: 'exponential', base: parameters.base !== undefined ? parameters.base : 1}) :
                 () => 0,
             zoomStops: parameters.stops.map(s => s[0]),
-            evaluate
+            evaluate: ({zoom}) => outputFunction(innerFun(parameters, propertySpec, zoom, hashedStops, categoricalKeyType))
         };
     } else {
         return {

diff --git a/src/style-spec/reference/v8.json b/src/style-spec/reference/v8.json
@@ -3149,19 +3149,20 @@
     },
     "heatmap-color": {
       "type": "color",
-      "default": {
-          "stops": [
-              [0, "rgba(0, 0, 255, 0)"],
-              [0.1, "royalblue"],
-              [0.3, "cyan"],
-              [0.5, "lime"],
-              [0.7, "yellow"],
-              [1, "red"]
-          ]
-      },
-      "doc": "Defines the color of each pixel based on its density value in a heatmap. Should be either a stop function with input values ranging from `0` to `1`, or an expression which uses `[\"heatmap-density\"]`.",
-      "function": "interpolated",
-      "zoom-function": true,
+      "default": [
+        "interpolate",
+        ["linear"],
+        ["heatmap-density"],
+        0, "rgba(0, 0, 255, 0)",
+        0.1, "royalblue",
+        0.3, "cyan",
+        0.5, "lime",
+        0.7, "yellow",
+        1, "red"
+      ],
+      "doc": "Defines the color of each pixel based on its density value in a heatmap.  Should be an expression that uses `[\"heatmap-density\"]` as input.",
+      "function": "interpolated",
+      "zoom-function": false,
       "property-function": false,
       "transition": true,
       "sdk-support": {

diff --git a/src/style-spec/style-spec.js b/src/style-spec/style-spec.js
@@ -4,40 +4,47 @@ export type StylePropertySpecification = {
     type: 'number',
     'function': boolean,
     'property-function': boolean,
+    'zoom-function': boolean,
     default?: number
 } | {
     type: 'string',
     'function': boolean,
     'property-function': boolean,
+    'zoom-function': boolean,
     default?: string
 } | {
     type: 'boolean',
     'function': boolean,
     'property-function': boolean,
+    'zoom-function': boolean,
     default?: boolean
 } | {
     type: 'enum',
     'function': boolean,
     'property-function': boolean,
+    'zoom-function': boolean,
     values: {[string]: {}},
     default?: string
 } | {
     type: 'color',
     'function': boolean,
     'property-function': boolean,
+    'zoom-function': boolean,
     default?: string
 } | {
     type: 'array',
     value: 'number',
     'function': boolean,
     'property-function': boolean,
+    'zoom-function': boolean,
     length?: number,
     default?: Array<number>
 } | {
     type: 'array',
     value: 'string',
     'function': boolean,
     'property-function': boolean,
+    'zoom-function': boolean,
     length?: number,
     default?: Array<string>
 };

diff --git a/src/style-spec/validate/validate_function.js b/src/style-spec/validate/validate_function.js
@@ -49,7 +49,7 @@ module.exports = function validateFunction(options) {
     if (options.styleSpec.$version >= 8) {
         if (isPropertyFunction && !options.valueSpec['property-function']) {
             errors.push(new ValidationError(options.key, options.value, 'property functions not supported'));
-        } else if (isZoomFunction && !options.valueSpec['zoom-function']) {
+        } else if (isZoomFunction && !options.valueSpec['zoom-function'] && options.objectKey !== 'heatmap-color') {
             errors.push(new ValidationError(options.key, options.value, 'zoom functions not supported'));
         }
     }

diff --git a/src/style/light.js b/src/style/light.js
@@ -43,7 +43,7 @@ class Light extends Evented {
         }, lightOpts);
 
         for (const prop of properties) {
-            this._declarations[prop] = new StyleDeclaration(specifications[prop], lightOpts[prop], prop);
+            this._declarations[prop] = new StyleDeclaration(specifications[prop], lightOpts[prop]);
         }
 
         return this;
@@ -96,7 +96,7 @@ class Light extends Evented {
             } else if (value === null || value === undefined) {
                 delete this._declarations[key];
             } else {
-                this._declarations[key] = new StyleDeclaration(specifications[key], value, key);
+                this._declarations[key] = new StyleDeclaration(specifications[key], value);
             }
         }
     }
@@ -112,7 +112,7 @@ class Light extends Evented {
         const spec = specifications[property];
 
         if (declaration === null || declaration === undefined) {
-            declaration = new StyleDeclaration(spec, spec.default, property);
+            declaration = new StyleDeclaration(spec, spec.default);
         }
 
         if (oldTransition && oldTransition.declaration.json === declaration.json) return;

diff --git a/src/style/style_declaration.js b/src/style/style_declaration.js
@@ -7,9 +7,9 @@ const util = require('../util/util');
 
 import type {StyleDeclarationExpression, Feature, GlobalProperties} from '../style-spec/expression';
 
-function normalizeToExpression(parameters, propertySpec, name): StyleDeclarationExpression {
+function normalizeToExpression(parameters, propertySpec): StyleDeclarationExpression {
     if (isFunction(parameters)) {
-        return createFunction(parameters, propertySpec, name);
+        return createFunction(parameters, propertySpec);
     } else if (isExpression(parameters)) {
         const expression = createExpression(parameters, propertySpec, 'property');
         if (expression.result !== 'success') {
@@ -47,14 +47,14 @@ class StyleDeclaration {
     minimum: number;
     expression: StyleDeclarationExpression;
 
-    constructor(reference: any, value: any, name: string) {
+    constructor(reference: any, value: any) {
         this.value = util.clone(value);
 
         // immutable representation of value. used for comparison
         this.json = JSON.stringify(this.value);
 
         this.minimum = reference.minimum;
-        this.expression = normalizeToExpression(this.value, reference, name);
+        this.expression = normalizeToExpression(this.value, reference);
     }
 
     calculate(globals: GlobalProperties, feature?: Feature) {

diff --git a/src/style/style_layer.js b/src/style/style_layer.js
@@ -108,7 +108,7 @@ class StyleLayer extends Evented {
         } else {
             const key = `layers.${this.id}.layout.${name}`;
             if (this._validate(validateStyle.layoutProperty, key, name, value, options)) return;
-            this._layoutDeclarations[name] = new StyleDeclaration(this._layoutSpecifications[name], value, name);
+            this._layoutDeclarations[name] = new StyleDeclaration(this._layoutSpecifications[name], value);
         }
         this._updateLayoutValue(name);
     }
@@ -146,7 +146,7 @@ class StyleLayer extends Evented {
             delete this._paintDeclarations[name];
         } else {
             if (this._validate(validateStyle.paintProperty, validateStyleKey, name, value, options)) return;
-            this._paintDeclarations[name] = new StyleDeclaration(this._paintSpecifications[name], value, name);
+            this._paintDeclarations[name] = new StyleDeclaration(this._paintSpecifications[name], value);
         }
     }
 
@@ -265,7 +265,7 @@ class StyleLayer extends Evented {
         const spec = this._paintSpecifications[name];
 
         if (declaration === null || declaration === undefined) {
-            declaration = new StyleDeclaration(spec, spec.default, name);
+            declaration = new StyleDeclaration(spec, spec.default);
         }
 
         if (oldTransition && oldTransition.declaration.json === declaration.json) return;

diff --git a/test/integration/render-tests/heatmap-color/function/expected.png b/test/integration/render-tests/heatmap-color/function/expected.png
diff --git a/test/integration/render-tests/heatmap-color/function/style.json b/test/integration/render-tests/heatmap-color/function/style.json
diff --git a/test/unit/style-spec/fixture/functions.input.json b/test/unit/style-spec/fixture/functions.input.json
@@ -942,6 +942,23 @@
       "paint": {
         "line-dasharray": ["interpolate", ["linear"], ["zoom"], 0, ["literal", [1, 2]], 1, ["literal", [3, 4]]]
       }
+    },
+    {
+      "id": "invalid expression - heatmap-color must be zoom constant",
+      "type": "heatmap",
+      "source": "source",
+      "source-layer": "layer",
+      "paint": {
+        "heatmap-color": ["step", ["zoom"], ["step", ["heatmap-density"], "red", 0.5, "blue"], 1, ["step", ["heatmap-density"], "yellow", 0.5, "green"]]
+      }
+    }, {
+      "id": "invalid expression - heatmap-color must not be a function",
+      "type": "heatmap",
+      "source": "source",
+      "source-layer": "layer",
+      "paint": {
+        "heatmap-color": { "stops": [[0, "red"], [1, "blue"]] }
+      }
     }
   ]
 }
diff --git a/test/unit/style-spec/fixture/functions.output.json b/test/unit/style-spec/fixture/functions.output.json
@@ -186,5 +186,13 @@
   {
     "message": "layers[52].paint.line-dasharray: Type array<number> is not interpolatable.",
     "line": 943
+  },
+  {
+    "message": "layers[53].paint.heatmap-color: zoom expressions not supported",
+    "line": 952
+  },
+  {
+    "message": "layers[54].paint.heatmap-color: zoom functions not supported",
+    "line": 960
   }
 ]