Make image operator compatible with styleimagemissing event (#8775) (#…

…8793)
mapbox · Sep 24, 2019 · ad86cf2 · ad86cf2
1 parent c63159a
commit ad86cf2
Show file tree

Hide file tree

Showing 15 changed files with 157 additions and 33 deletions.
diff --git a/debug/default-image.html b/debug/default-image.html
@@ -0,0 +1,85 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <title>Mapbox GL JS debug page</title>
+    <meta charset='utf-8'>
+    <meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no">
+    <link rel='stylesheet' href='../dist/mapbox-gl.css' />
+    <style>
+        body { margin: 0; padding: 0; }
+        html, body, #map { height: 100%; }
+    </style>
+</head>
+
+<body>
+<div id='map'></div>
+
+<script src='../dist/mapbox-gl-dev.js'></script>
+<script src='access_token_generated.js'></script>
+<script>
+
+var map = window.map = new mapboxgl.Map({
+    container: 'map',
+    zoom: 4,
+    center: [-96, 37.8],
+    style: 'mapbox://styles/mapbox/light-v10',
+    hash: true
+});
+
+map.on('load', function () {
+    map.addLayer({
+        "id": "points",
+        "type": "symbol",
+        "source": {
+            "type": "geojson",
+            "data": {
+                "type": "FeatureCollection",
+                "features": [
+                    {
+                        "type": "Feature",
+                        "geometry": {
+                            "type": "Point",
+                            "coordinates": [
+                                -77.03238901390978,
+                                38.913188059745586
+                            ]
+                        },
+                        "properties": {
+                            "icon": "monument",
+                            "title": "Mapbox DC"
+                        }
+                    }, {
+                        "type": "Feature",
+                        "geometry": {
+                            "type": "Point",
+                            "coordinates": [
+                                -122.414,
+                                37.776
+                            ]
+                        },
+                        "properties": {
+                            "title": "Mapbox SF",
+                            "icon": "harbor"
+                        }
+                    }
+                ]
+            }
+        },
+        "layout": {
+            "icon-image": ["coalesce", ["image", "foo"], ["image", "bar"], ["image", ["concat", ["get", "icon"], "-15"]], ["image", "baz"]],
+            "text-field": "{title}",
+            "text-font": [
+                "Open Sans Semibold",
+                "Arial Unicode MS Bold"
+            ],
+            "text-offset": [
+                0,
+                0.6
+            ],
+            "text-anchor": "top"
+        }
+    });
+});
+</script>
+</body>
+</html>
diff --git a/src/data/bucket/pattern_bucket_features.js b/src/data/bucket/pattern_bucket_features.js
@@ -41,9 +41,12 @@ export function addPatternDependencies(type: string, layers: PatternStyleLayers,
 
         const patternPropertyValue = patternProperty.value;
         if (patternPropertyValue.kind !== "constant") {
-            const min = patternPropertyValue.evaluate({zoom: zoom - 1}, patternFeature, {}, options.availableImages);
-            const mid = patternPropertyValue.evaluate({zoom}, patternFeature, {}, options.availableImages);
-            const max = patternPropertyValue.evaluate({zoom: zoom + 1}, patternFeature, {}, options.availableImages);
+            let min = patternPropertyValue.evaluate({zoom: zoom - 1}, patternFeature, {}, options.availableImages);
+            let mid = patternPropertyValue.evaluate({zoom}, patternFeature, {}, options.availableImages);
+            let max = patternPropertyValue.evaluate({zoom: zoom + 1}, patternFeature, {}, options.availableImages);
+            min = min && min.name ? min.name : min;
+            mid = mid && mid.name ? mid.name : mid;
+            max = max && max.name ? max.name : max;
             // add to patternDependencies
             patterns[min] = true;
             patterns[mid] = true;

diff --git a/src/data/bucket/symbol_bucket.js b/src/data/bucket/symbol_bucket.js
@@ -378,7 +378,11 @@ class SymbolBucket implements Bucket {
         const hasText =
             (textField.value.kind !== 'constant' || textField.value.value.toString().length > 0) &&
             (textFont.value.kind !== 'constant' || textFont.value.value.length > 0);
-        const hasIcon = iconImage.value.kind !== 'constant' || iconImage.value.value && iconImage.value.value.toString().length > 0;
+        // we should always resolve the icon-image value if the property was defined in the style
+        // this allows us to fire the styleimagemissing event if image evaluation returns null
+        // the only way to distinguish between null returned from a coalesce statement with no valid images
+        // and null returned because icon-image wasn't defined is to check whether or not iconImage.parameters is an empty object
+        const hasIcon = iconImage.value.kind !== 'constant' || !!iconImage.value.value || Object.keys(iconImage.parameters).length > 0;
         const symbolSortKey = layout.get('symbol-sort-key');
 
         this.features = [];
@@ -415,9 +419,13 @@ class SymbolBucket implements Bucket {
                 // but plain string token evaluation skips that pathway so do the
                 // conversion here.
                 const resolvedTokens = layer.getValueAndResolveTokens('icon-image', feature, availableImages);
-                icon = resolvedTokens instanceof ResolvedImage ?
-                    resolvedTokens :
-                    ResolvedImage.fromString(resolvedTokens);
+                if (resolvedTokens instanceof ResolvedImage) {
+                    icon = resolvedTokens;
+                } else if (!resolvedTokens || typeof resolvedTokens === 'string') {
+                    icon = ResolvedImage.fromString({name: resolvedTokens, available: false});
+                } else {
+                    icon = ResolvedImage.fromString(resolvedTokens);
+                }
             }
 
             if (!text && !icon) {

diff --git a/src/style-spec/expression/definitions/coalesce.js b/src/style-spec/expression/definitions/coalesce.js
@@ -53,8 +53,21 @@ class Coalesce implements Expression {
 
     evaluate(ctx: EvaluationContext) {
         let result = null;
+        let argCount = 0;
+        let requestedImageName;
         for (const arg of this.args) {
+            argCount++;
             result = arg.evaluate(ctx);
+            // we need to keep track of the first requested image in a coalesce statement
+            // if coalesce can't find a valid image, we return the first image name so styleimagemissing can fire
+            if (arg.type.kind === 'image' && !result.available) {
+                if (!requestedImageName) requestedImageName = arg.evaluate(ctx).name;
+                result = null;
+                if (argCount === this.args.length) {
+                    result = requestedImageName;
+                }
+            }
+
             if (result !== null) break;
         }
         return result;

diff --git a/src/style-spec/expression/definitions/coercion.js b/src/style-spec/expression/definitions/coercion.js
@@ -102,7 +102,8 @@ class Coercion implements Expression {
             // created by properties that expect the 'formatted' type.
             return Formatted.fromString(valueToString(this.args[0].evaluate(ctx)));
         } else if (this.type.kind === 'image') {
-            return ResolvedImage.fromString(valueToString(this.args[0].evaluate(ctx)));
+            const name = valueToString(this.args[0].evaluate(ctx));
+            return ResolvedImage.fromString({name, available: false});
         } else {
             return valueToString(this.args[0].evaluate(ctx));
         }

diff --git a/src/style-spec/expression/definitions/image.js b/src/style-spec/expression/definitions/image.js
@@ -29,12 +29,13 @@ export default class ImageExpression implements Expression {
 
     evaluate(ctx: EvaluationContext) {
         const evaluatedImageName = this.input.evaluate(ctx);
+        let available = false;
 
         if (ctx.availableImages && ctx.availableImages.indexOf(evaluatedImageName) > -1) {
-            return evaluatedImageName;
+            available = true;
         }
 
-        return null;
+        return {name: evaluatedImageName, available};
     }
 
     eachChild(fn: (Expression) => void) {

diff --git a/src/style-spec/expression/types/resolved_image.js b/src/style-spec/expression/types/resolved_image.js
@@ -1,18 +1,25 @@
 // @flow
 
+export type ResolvedImageOptions = {
+    name: string,
+    available: boolean
+};
+
 export default class ResolvedImage {
     name: string;
+    available: boolean;
 
-    constructor(name: string) {
-        this.name = name;
+    constructor(options: ResolvedImageOptions) {
+        this.name = options.name;
+        this.available = options.available;
     }
 
     toString(): string {
         return this.name;
     }
 
-    static fromString(imageName: string): ResolvedImage {
-        return new ResolvedImage(imageName);
+    static fromString(options: ResolvedImageOptions): ResolvedImage {
+        return new ResolvedImage(options);
     }
 
     serialize(): Array<mixed> {

diff --git a/src/style-spec/function/index.js b/src/style-spec/function/index.js
@@ -203,7 +203,7 @@ function evaluateIdentityFunction(parameters, propertySpec, input) {
     } else if (propertySpec.type === 'formatted') {
         input = Formatted.fromString(input.toString());
     } else if (propertySpec.type === 'image') {
-        input = ResolvedImage.fromString(input.toString());
+        input = ResolvedImage.fromString({name: input.toString(), available: false});
     } else if (getType(input) !== propertySpec.type && (propertySpec.type !== 'enum' || !propertySpec.values[input])) {
         input = undefined;
     }

diff --git a/src/style/properties.js b/src/style/properties.js
@@ -599,7 +599,9 @@ export class CrossFadedDataDrivenProperty<T> extends DataDrivenProperty<?CrossFa
         if (value.value === undefined) {
             return new PossiblyEvaluatedPropertyValue(this, {kind: 'constant', value: undefined}, parameters);
         } else if (value.expression.kind === 'constant') {
-            const constantValue = value.expression.evaluate(parameters, (null: any), {}, availableImages);
+            const evaluatedValue = value.expression.evaluate(parameters, (null: any), {}, availableImages);
+            const isImageExpression = value.property.specification.type === 'image';
+            const constantValue = isImageExpression && typeof evaluatedValue !== 'string' ? evaluatedValue.name : evaluatedValue;
             const constant = this._calculate(constantValue, constantValue, constantValue, parameters);
             return new PossiblyEvaluatedPropertyValue(this, {kind: 'constant', value: constant}, parameters);
         } else if (value.expression.kind === 'camera') {

diff --git a/src/style/style_layer/symbol_style_layer.js b/src/style/style_layer/symbol_style_layer.js
@@ -95,7 +95,7 @@ class SymbolStyleLayer extends StyleLayer {
     getValueAndResolveTokens(name: *, feature: Feature, availableImages: Array<string>) {
         const value = this.layout.get(name).evaluate(feature, {}, availableImages);
         const unevaluated = this._unevaluatedLayout._values[name];
-        if (!unevaluated.isDataDriven() && !isExpression(unevaluated.value)) {
+        if (!unevaluated.isDataDriven() && !isExpression(unevaluated.value) && value) {
             return resolveTokens(feature.properties, value);
         }
 

diff --git a/test/integration/expression-tests/image/basic/test.json b/test/integration/expression-tests/image/basic/test.json
@@ -17,7 +17,7 @@
       "type": "image"
     },
     "outputs": [
-      "monument-15"
+      {"name":"monument-15","available":true}
     ],
     "serialized": [
       "image",

diff --git a/test/integration/expression-tests/image/coalesce/test.json b/test/integration/expression-tests/image/coalesce/test.json
@@ -14,7 +14,7 @@
       "type": "image"
     },
     "outputs": [
-      "monument-15"
+      {"name":"monument-15","available":true}
     ],
     "serialized": [
       "coalesce",

diff --git a/test/integration/expression-tests/image/compound/test.json b/test/integration/expression-tests/image/compound/test.json
@@ -12,7 +12,7 @@
       "type": "image"
     },
     "outputs": [
-      "monument-15"
+      {"name":"monument-15","available":true}
     ],
     "serialized": [
         "image",

diff --git a/test/unit/source/geojson_worker_source.test.js b/test/unit/source/geojson_worker_source.test.js
@@ -4,6 +4,8 @@ import StyleLayerIndex from '../../../src/style/style_layer_index';
 import {OverscaledTileID} from '../../../src/source/tile_id';
 import perf from '../../../src/util/performance';
 
+const actor = {send: () => {}};
+
 test('reloadTile', (t) => {
     t.test('does not rebuild vector data unless data has changed', (t) => {
         const layers = [
@@ -14,7 +16,7 @@ test('reloadTile', (t) => {
             }
         ];
         const layerIndex = new StyleLayerIndex(layers);
-        const source = new GeoJSONWorkerSource(null, layerIndex, []);
+        const source = new GeoJSONWorkerSource(actor, layerIndex, []);
         const originalLoadVectorData = source.loadVectorData;
         let loadVectorCallCount = 0;
         source.loadVectorData = function(params, callback) {
@@ -126,7 +128,7 @@ test('resourceTiming', (t) => {
         t.stub(perf, 'getEntriesByName').callsFake(() => { return [ exampleResourceTiming ]; });
 
         const layerIndex = new StyleLayerIndex(layers);
-        const source = new GeoJSONWorkerSource(null, layerIndex, [], (params, callback) => { return callback(null, geoJson); });
+        const source = new GeoJSONWorkerSource(actor, layerIndex, [], (params, callback) => { return callback(null, geoJson); });
 
         source.loadData({source: 'testSource', request: {url: 'http://localhost/nonexistent', collectResourceTiming: true}}, (err, result) => {
             t.equal(err, null);
@@ -158,7 +160,7 @@ test('resourceTiming', (t) => {
         t.stub(perf, 'clearMeasures').callsFake(() => { return null; });
 
         const layerIndex = new StyleLayerIndex(layers);
-        const source = new GeoJSONWorkerSource(null, layerIndex, [], (params, callback) => { return callback(null, geoJson); });
+        const source = new GeoJSONWorkerSource(actor, layerIndex, [], (params, callback) => { return callback(null, geoJson); });
 
         source.loadData({source: 'testSource', request: {url: 'http://localhost/nonexistent', collectResourceTiming: true}}, (err, result) => {
             t.equal(err, null);
@@ -169,7 +171,7 @@ test('resourceTiming', (t) => {
 
     t.test('loadData - data', (t) => {
         const layerIndex = new StyleLayerIndex(layers);
-        const source = new GeoJSONWorkerSource(null, layerIndex, []);
+        const source = new GeoJSONWorkerSource(actor, layerIndex, []);
 
         source.loadData({source: 'testSource', data: JSON.stringify(geoJson)}, (err, result) => {
             t.equal(err, null);
@@ -205,7 +207,7 @@ test('loadData', (t) => {
 
     const layerIndex = new StyleLayerIndex(layers);
     function createWorker() {
-        const worker = new GeoJSONWorkerSource(null, layerIndex, []);
+        const worker = new GeoJSONWorkerSource(actor, layerIndex, []);
 
         // Making the call to loadGeoJSON asynchronous
         // allows these tests to mimic a message queue building up

diff --git a/test/unit/source/vector_tile_worker_source.test.js b/test/unit/source/vector_tile_worker_source.test.js
@@ -7,8 +7,10 @@ import VectorTileWorkerSource from '../../../src/source/vector_tile_worker_sourc
 import StyleLayerIndex from '../../../src/style/style_layer_index';
 import perf from '../../../src/util/performance';
 
+const actor = {send: () => {}};
+
 test('VectorTileWorkerSource#abortTile aborts pending request', (t) => {
-    const source = new VectorTileWorkerSource(null, new StyleLayerIndex(), []);
+    const source = new VectorTileWorkerSource(actor, new StyleLayerIndex(), []);
 
     source.loadTile({
         source: 'source',
@@ -33,7 +35,7 @@ test('VectorTileWorkerSource#abortTile aborts pending request', (t) => {
 });
 
 test('VectorTileWorkerSource#removeTile removes loaded tile', (t) => {
-    const source = new VectorTileWorkerSource(null, new StyleLayerIndex(), []);
+    const source = new VectorTileWorkerSource(actor, new StyleLayerIndex(), []);
 
     source.loaded = {
         '0': {}
@@ -52,7 +54,7 @@ test('VectorTileWorkerSource#removeTile removes loaded tile', (t) => {
 });
 
 test('VectorTileWorkerSource#reloadTile reloads a previously-loaded tile', (t) => {
-    const source = new VectorTileWorkerSource(null, new StyleLayerIndex(), []);
+    const source = new VectorTileWorkerSource(actor, new StyleLayerIndex(), []);
     const parse = t.spy();
 
     source.loaded = {
@@ -74,7 +76,7 @@ test('VectorTileWorkerSource#reloadTile reloads a previously-loaded tile', (t) =
 });
 
 test('VectorTileWorkerSource#reloadTile queues a reload when parsing is in progress', (t) => {
-    const source = new VectorTileWorkerSource(null, new StyleLayerIndex(), []);
+    const source = new VectorTileWorkerSource(actor, new StyleLayerIndex(), []);
     const parse = t.spy();
 
     source.loaded = {
@@ -108,7 +110,7 @@ test('VectorTileWorkerSource#reloadTile queues a reload when parsing is in progr
 
 test('VectorTileWorkerSource#reloadTile handles multiple pending reloads', (t) => {
     // https://github.com/mapbox/mapbox-gl-js/issues/6308
-    const source = new VectorTileWorkerSource(null, new StyleLayerIndex(), []);
+    const source = new VectorTileWorkerSource(actor, new StyleLayerIndex(), []);
     const parse = t.spy();
 
     source.loaded = {
@@ -156,7 +158,7 @@ test('VectorTileWorkerSource#reloadTile handles multiple pending reloads', (t) =
 });
 
 test('VectorTileWorkerSource#reloadTile does not reparse tiles with no vectorTile data but does call callback', (t) => {
-    const source = new VectorTileWorkerSource(null, new StyleLayerIndex(), []);
+    const source = new VectorTileWorkerSource(actor, new StyleLayerIndex(), []);
     const parse = t.spy();
 
     source.loaded = {
@@ -215,7 +217,7 @@ test('VectorTileWorkerSource provides resource timing information', (t) => {
         type: 'fill'
     }]);
 
-    const source = new VectorTileWorkerSource(null, layerIndex, [], loadVectorData);
+    const source = new VectorTileWorkerSource(actor, layerIndex, [], loadVectorData);
 
     t.stub(perf, 'getEntriesByName').callsFake(() => { return [ exampleResourceTiming ]; });
 
@@ -250,7 +252,7 @@ test('VectorTileWorkerSource provides resource timing information (fallback meth
         type: 'fill'
     }]);
 
-    const source = new VectorTileWorkerSource(null, layerIndex, [], loadVectorData);
+    const source = new VectorTileWorkerSource(actor, layerIndex, [], loadVectorData);
 
     const sampleMarks = [100, 350];
     const marks = {};