Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make image operator compatible with styleimagemissing event #8775

Merged
merged 15 commits into from
Sep 24, 2019
Merged

Make image operator compatible with styleimagemissing event #8775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
30ac880
Partial implementation of styleimagemissing
Sep 19, 2019
273b146
Cleanup
Sep 19, 2019
b46932f
Return object from Image operator
Sep 19, 2019
86bcb92
Allow coalesce to work with Image object
Sep 20, 2019
63d2ea4
Fix lint
Sep 20, 2019
9ae1733
Clean up code
Sep 20, 2019
2f6721c
mostly fix the tests
Sep 20, 2019
181fea9
Fix expression tests
Sep 20, 2019
2d48c39
Fix Flow issue
Sep 20, 2019
1a5b2ab
Fix lint issue
Sep 20, 2019
13cfbf7
Mock actor in worker source tests
Sep 20, 2019
cd63a78
Fix check for undefined icon-image property
Sep 20, 2019
bf9b06e
Enable styleimagemissing with pattern properties
Sep 20, 2019
c936b93
Add debug page
Sep 23, 2019
a08d256
Make if statement more legible
Sep 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions debug/default-image.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>
9 changes: 6 additions & 3 deletions src/data/bucket/pattern_bucket_features.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines +47 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asheemmamoowala @ryanhamley If min is null => patterns[null] = true?

Copy link
Contributor Author

@ryanhamley ryanhamley Nov 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't null a possible result here even before this change?

I can't remember exactly why I changed this now off the top of my head, but I think the ternary operator might have been more of a defensive guard. I'm not actually sure that min can ever be null at this point because any value supplied to *-pattern properties will be coerced to a ResolvedImage type. If you tried to just do line-pattern: null or something, you'd get a type error before it got to this point. But maybe there's a pathway I'm not thinking about/a better way to handle this to ensure we don't get patterns[null].

// add to patternDependencies
patterns[min] = true;
patterns[mid] = true;
Expand Down
16 changes: 12 additions & 4 deletions src/data/bucket/symbol_bucket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of this comment explains the high-level reasoning for retuning an object from the image expression. That should be documented in the code for that expression, or expressly stated in the PR description (its not too late to do that yet).

This comment seems less of a clarification of what the code does here. Maybe replace it with:

// `iconImage.parameters` distinguishes between an unspecified `icon-image` property and specified but not found value. Check it here to trigger styleimagemissing events as appropriate

const symbolSortKey = layout.get('symbol-sort-key');

this.features = [];
Expand Down Expand Up @@ -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) {
Expand Down
13 changes: 13 additions & 0 deletions src/style-spec/expression/definitions/coalesce.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanhamley Why type of arg is checked? I think it should it be type of coalesce itself and then check if evaluated result is an image.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the check to if (result && result instanceof ResolvedImage && !result.available) seems to work fine. Is that what you had in mind?

if (!requestedImageName) requestedImageName = arg.evaluate(ctx).name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanhamley @asheemmamoowala arg is already evaluated on line 60

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an oversight which can be corrected

result = null;
if (argCount === this.args.length) {
result = requestedImageName;
}
}

if (result !== null) break;
}
return result;
Expand Down
3 changes: 2 additions & 1 deletion src/style-spec/expression/definitions/coercion.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
5 changes: 3 additions & 2 deletions src/style-spec/expression/definitions/image.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
15 changes: 11 additions & 4 deletions src/style-spec/expression/types/resolved_image.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Why do we have fromString if it doesn't accept strings, and the plain constructor could be used with the same arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's a good point. It's used in other places where it might be less straightforward to use the constructor though (https://github.com/mapbox/mapbox-gl-js/blob/master/src/style-spec/expression/definitions/coercion.js#L105). I'm inclined to leave this as is though because it follows the pattern of other expression types and this type and operator will likely be expanded significantly in the future as we add more image manipulation operators.

return new ResolvedImage(options);
}

serialize(): Array<mixed> {
Expand Down
2 changes: 1 addition & 1 deletion src/style-spec/function/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
4 changes: 3 additions & 1 deletion src/style/properties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Expand Down
2 changes: 1 addition & 1 deletion src/style/style_layer/symbol_style_layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion test/integration/expression-tests/image/basic/test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"type": "image"
},
"outputs": [
"monument-15"
{"name":"monument-15","available":true}
],
"serialized": [
"image",
Expand Down
2 changes: 1 addition & 1 deletion test/integration/expression-tests/image/coalesce/test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"type": "image"
},
"outputs": [
"monument-15"
{"name":"monument-15","available":true}
],
"serialized": [
"coalesce",
Expand Down
2 changes: 1 addition & 1 deletion test/integration/expression-tests/image/compound/test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"type": "image"
},
"outputs": [
"monument-15"
{"name":"monument-15","available":true}
],
"serialized": [
"image",
Expand Down
12 changes: 7 additions & 5 deletions test/unit/source/geojson_worker_source.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand All @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down
18 changes: 10 additions & 8 deletions test/unit/source/vector_tile_worker_source.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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': {}
Expand All @@ -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 = {
Expand All @@ -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 = {
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 ]; });

Expand Down Expand Up @@ -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 = {};
Expand Down