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.

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

symbol-clip: dynamic filter style spec changes #10977

Merged
merged 40 commits into from
Sep 29, 2021
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
2969f28
WIP dynamic filter splitting stage 1
Jul 27, 2021
b4689b9
Add tests for isDynamicFilter
Jul 27, 2021
ff60896
More test cases for isDynamicFilter
Jul 27, 2021
24fd71c
Existing static filters get passed through unmodified.
Jul 28, 2021
b66e22c
WIP extracting static filters
Jul 30, 2021
c68f159
Working case -> any translation
Aug 2, 2021
4bdf7a1
More tests for case -> any conversion
Aug 3, 2021
a609a4d
Add support for match branches
Aug 4, 2021
aa8cc2b
WIP: V0 of adding collapsing of dynamic expressions to true
Aug 9, 2021
e18336f
more test cases
Aug 10, 2021
035ec77
tests and some more refactoring
Aug 10, 2021
495b279
remove temory inspection code from unit tests
Aug 11, 2021
eac58f6
Fix dynamic filter detection
Aug 12, 2021
81cdbc8
Fix failing spec test
Aug 12, 2021
3c2f2d3
Units tests hopefully :green: now
Aug 13, 2021
4bee8da
Add test to cover for `null` in expressions
Aug 18, 2021
0d8d809
Address CR comments and reduce number of temporary allocations
Aug 18, 2021
e015ec5
remove gl-matrix as dependency of style-spec and inline a matrix mult…
Aug 18, 2021
53c644e
Fix lint issues
Aug 18, 2021
2a063b0
Add better error messages for expression compilation failures
Aug 24, 2021
d4e9ac3
Ensure location parameter is passed down
Aug 25, 2021
0265792
Remove symbol-clip from the style-spec
Aug 25, 2021
80a52d7
Remove unused property-expression type
Aug 25, 2021
ea727a5
Move filterSpec to v8.json and replace symbol-clip with dynamic filter
Aug 30, 2021
b0533d8
Fix unit tests
Aug 31, 2021
2f8ed09
Fix most flow errors
Sep 1, 2021
566b59c
finally flow is happy
Sep 1, 2021
450ca3f
Add expression validation code
Sep 9, 2021
c8d7ed5
Add api-supported tests for validation
Sep 9, 2021
a121924
Fix some failing unit tests
Sep 9, 2021
df96e1d
Ensure layer._featureFIlter is updated on main thread as well
Sep 9, 2021
796aca0
Ensure layerType is passed down to validation
Sep 9, 2021
d70d880
Merge branch 'symbol-clip' of github.com:mapbox/mapbox-gl-js into dyn…
Sep 15, 2021
9bb07a5
Fix flow and linting
Sep 15, 2021
6a9defa
Fix unit tests
Sep 15, 2021
8e6bd6e
Pass 1 of addressing CR comments
Sep 16, 2021
5189a1a
Address review comments
Sep 28, 2021
e99fe09
cast through any to clean up
Sep 28, 2021
ce2ace4
backticks 1
Sep 29, 2021
13ffe33
backticks 2
Sep 29, 2021
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
30 changes: 0 additions & 30 deletions debug/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -26,36 +26,6 @@
hash: true
});

map.once('load', () => {
map.setLayoutProperty('building-number-label', 'symbol-clip',
["case",
["<", ["pitch"], 60], false,
["all", [">=", ["pitch"], 60], ["<", ["distance-from-center"], 2]], false,
true
]
);

map.setLayoutProperty('poi-label', 'symbol-clip',
["case",
["<", ["pitch"], 60], false,
["all", [">=", ["pitch"], 60], ["<", ["distance-from-center"], 4]], false,
true
]
);

map.setLayoutProperty('transit-label', 'symbol-clip',
["case",
["<", ["pitch"], 60], false,
["all", [">=", ["pitch"], 60], ["<", ["distance-from-center"], 3]], false,
true
]
);

map.once('idle', () => {
map.panTo([-122.4199724, 37.7687162], {duration: 50000});
});
});

</script>
</body>
</html>
2 changes: 1 addition & 1 deletion src/style-spec/expression/definitions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ CompoundExpression.register(expressions, {
'pitch': [
NumberType,
[],
(ctx) => ctx.globals.pitch
(ctx) => ctx.globals.pitch || 0
],
'distance-from-center': [
NumberType,
Expand Down
42 changes: 3 additions & 39 deletions src/style-spec/expression/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import definitions from './definitions/index.js';
import * as isConstant from './is_constant.js';
import RuntimeError from './runtime_error.js';
import {success, error} from '../util/result.js';
import {supportsPropertyExpression, supportsZoomExpression, supportsInterpolation, supportsCameraStateExpression} from '../util/properties.js';
import {supportsPropertyExpression, supportsZoomExpression, supportsInterpolation} from '../util/properties.js';

import type {Type, EvaluationKind} from './types.js';
import type {Value} from './values.js';
Expand All @@ -41,7 +41,7 @@ export type FeatureState = {[_: string]: any};

export type GlobalProperties = $ReadOnly<{
zoom: number,
pitch: number,
pitch?: number,
cameraDistanceMatrix?: number[],
heatmapDensity?: number,
lineProgress?: number,
Expand Down Expand Up @@ -191,24 +191,6 @@ export class ZoomDependentExpression<Kind: EvaluationKind> {
}
}

export class CameraDependentExpression<Kind: EvaluationKind> {
kind: Kind;
_styleExpression: StyleExpression;

constructor(kind: Kind, expression: StyleExpression) {
this.kind = kind;
this._styleExpression = expression;
}

evaluateWithoutErrorHandling(globals: GlobalProperties, feature?: Feature, featureState?: FeatureState, canonical?: CanonicalTileID, availableImages?: Array<string>, formattedSection?: FormattedSection,): any {
return this._styleExpression.evaluateWithoutErrorHandling(globals, feature, featureState, canonical, availableImages, formattedSection);
}

evaluate(globals: GlobalProperties, feature?: Feature, featureState?: FeatureState, canonical?: CanonicalTileID, availableImages?: Array<string>, formattedSection?: FormattedSection, refLocation: MercatorCoordinate): any {
return this._styleExpression.evaluate(globals, feature, featureState, canonical, availableImages, formattedSection, refLocation);
}
}

export type ConstantExpression = {
kind: 'constant',
+evaluate: (globals: GlobalProperties, feature?: Feature, featureState?: FeatureState, canonical?: CanonicalTileID, availableImages?: Array<string>) => any,
Expand Down Expand Up @@ -237,23 +219,11 @@ export type CompositeExpression = {
interpolationType: ?InterpolationType
};

export type CameraStateExpression = {
kind: 'camera',
+evaluate: (globals: GlobalProperties, feature?: Feature, featureState?: FeatureState, canonical?: CanonicalTileID, availableImages?: Array<string>) => any,
};

export type CompositeCameraStateExpression = {
kind: 'composite',
+evaluate: (globals: GlobalProperties, feature?: Feature, featureState?: FeatureState, canonical?: CanonicalTileID, availableImages?: Array<string>, formattedSection?: FormattedSection, refLocation?: MercatorCoordinate) => any,
};

export type StylePropertyExpression =
| ConstantExpression
| SourceExpression
| CameraExpression
| CompositeExpression
| CameraStateExpression
| CompositeCameraStateExpression;
| CompositeExpression;

export function createPropertyExpression(expression: mixed, propertySpec: StylePropertySpecification): Result<StylePropertyExpression, Array<ParsingError>> {
expression = createExpression(expression, propertySpec);
Expand All @@ -273,12 +243,6 @@ export function createPropertyExpression(expression: mixed, propertySpec: StyleP
return error([new ParsingError('', 'zoom expressions not supported')]);
}

if (supportsCameraStateExpression(propertySpec)) {
return success(isFeatureConstant ?
(new CameraDependentExpression('composite', expression.value): CameraStateExpression) :
(new CameraDependentExpression('composite', expression.value): CompositeCameraStateExpression));
}

const zoomCurve = findZoomCurve(parsed);
if (!zoomCurve && !isZoomConstant) {
return error([new ParsingError('', '"zoom" expression may only be used as input to a top-level "step" or "interpolate" expression.')]);
Expand Down
123 changes: 62 additions & 61 deletions src/style-spec/feature_filter/index.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
// @flow

import {createExpression} from '../expression/index.js';
import latest from '../reference/latest.js';
import type {GlobalProperties, Feature} from '../expression/index.js';
import type {CanonicalTileID} from '../../source/tile_id.js';
import type MercatorCoordinate from '../../geo/mercator_coordinate.js';

type FilterExpression = (globalProperties: GlobalProperties, feature: Feature, canonical?: CanonicalTileID) => boolean;
export type FeatureFilter = {filter: FilterExpression, dynamicFilter: FilterExpression, needGeometry: boolean};
type FilterExpression = (globalProperties: GlobalProperties, feature: Feature, canonical?: CanonicalTileID, refLocation?: MercatorCoordinate) => boolean;
export type FeatureFilter = {filter: FilterExpression, dynamicFilter?: FilterExpression, needGeometry: boolean};

export default createFilter;
export {isExpressionFilter, isDynamicFilter, extractStaticFilter};
Expand Down Expand Up @@ -53,75 +54,75 @@ function isExpressionFilter(filter: any) {
}
}

const filterSpec = {
'type': 'boolean',
'default': false,
'transition': false,
'property-type': 'data-driven',
'expression': {
'interpolated': false,
'parameters': ['zoom', 'feature', 'pitch', 'distance-from-center']
}
};

/**
* Given a filter expressed as nested arrays, return a new function
* that evaluates whether a given feature (with a .properties or .tags property)
* passes its test.
*
* @private
* @param {Array} filter mapbox gl filter
* @param {string} layerType the type of the layer this filter will be applied to.
* @returns {Function} filter-evaluating function
*/
function createFilter(filter: any): FeatureFilter {
function createFilter(filter: any, layerType?: string = 'fill'): FeatureFilter {
if (filter === null || filter === undefined) {
return {filter: () => true, dynamicFilter: () => true, needGeometry: false};
return {filter: () => true, needGeometry: false};
}

if (!isExpressionFilter(filter)) {
filter = convertFilter(filter);
}
const filterExp = ((filter: any): string[] | string | boolean);

let staticFilter = true;
try {
staticFilter = extractStaticFilter(filter);
staticFilter = extractStaticFilter(filterExp);
} catch (e) {
console.warn(
`Failed to extract static filter. Filter will continue working, but at higher memory usage and slower framerate.
This is most likely a bug, please report this via https://github.com/mapbox/mapbox-gl-js/issues/new?assignees=&labels=&template=Bug_report.md
and paste the contents of this message in the report.
Thank you!
Filter Expression:
${JSON.stringify(filter, null, 2)}
${JSON.stringify(filterExp, null, 2)}
`);
}
const dynamicFilter = filter === staticFilter ? true : filter;

// Compile the static component of the filter
const filterSpec = latest[`filter_${layerType}`];
const compiledStaticFilter = createExpression(staticFilter, filterSpec);
const compiledDynamicFilter = createExpression(dynamicFilter, filterSpec);
if (compiledStaticFilter.result === 'error' || compiledDynamicFilter.result === 'error') {
const staticFilterErrors = compiledStaticFilter.result === 'error' ?
compiledStaticFilter.value.map(err => `${err.key}: ${err.message}`).join(', ') :
'None';

const dynamicFilterErrors = compiledDynamicFilter.result === 'error' ?
compiledDynamicFilter.value.map(err => `${err.key}: ${err.message}`).join(', ') :
'None';

const errorMsg = `static-filter errors:
${staticFilterErrors}

dynamic-filter errors:
${dynamicFilterErrors}
`;
throw new Error(errorMsg);

let filterFunc = null;
if (compiledStaticFilter.result === 'error') {
throw new Error(compiledStaticFilter.value.map(err => `${err.key}: ${err.message}`).join(', '));
} else {
filterFunc = (globalProperties: GlobalProperties, feature: Feature, canonical?: CanonicalTileID) => compiledStaticFilter.value.evaluate(globalProperties, feature, {}, canonical);
}

// If the static component is not equal to the entire filter then we have a dynamic component
// Compile the dynamic component separately
let dynamicFilterFunc = null;
if (staticFilter !== filterExp) {
const compiledDynamicFilter = createExpression(filterExp, filterSpec);

if (compiledDynamicFilter.result === 'error') {
throw new Error(compiledDynamicFilter.value.map(err => `${err.key}: ${err.message}`).join(', '));
} else {
dynamicFilterFunc = (globalProperties: GlobalProperties, feature: Feature, canonical?: CanonicalTileID, refLocation?: MercatorCoordinate) => compiledDynamicFilter.value.evaluate(globalProperties, feature, {}, canonical, undefined, undefined, refLocation);
}
}

if (filterFunc) {
const needGeometry = geometryNeeded(staticFilter);

return {
filter: (globalProperties: GlobalProperties, feature: Feature, canonical?: CanonicalTileID) => compiledStaticFilter.value.evaluate(globalProperties, feature, {}, canonical),
dynamicFilter: (globalProperties: GlobalProperties, feature: Feature, canonical?: CanonicalTileID, refLocation: ?MercatorCoordinate) => compiledDynamicFilter.value.evaluate(globalProperties, feature, {}, canonical, null, null, refLocation),
filter: filterFunc,
dynamicFilter: dynamicFilterFunc ? dynamicFilterFunc : undefined,
needGeometry
};
} else {
// This branch cannot happen but need to keep flow happy :(
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like Flow is dictating the structure of this whole section at the expense of readability!
Would Flow be ok with a if (!filterFunc) {return;} so that you could unwrap the main return statement?
Or could you make filterFunc a const and use a try / catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, flow cannot detect early returns statically.

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 did a force cast instead, looks slightly cleaner.

return {filter: () => true, needGeometry: false};
}
}

Expand Down Expand Up @@ -155,30 +156,6 @@ function collapseDynamicBooleanExpressions(expression: any): any {
}
}

const dynamicConditionExpressions = new Set([
'in',
'==',
'!=',
'>',
'>=',
'<',
'<=',
'to-boolean'
]);

function collapsedExpression(expression: any): any {
if (dynamicConditionExpressions.has(expression[0])) {

for (let i = 1; i < expression.length; i++) {
const param = expression[i];
if (isDynamicFilter(param)) {
return true;
}
}
}
return expression;
}

/**
* Traverses the expression and replaces all instances of branching on a
* `dynamic` conditional (such as `['pitch']` or `['distance-from-center']`)
Expand Down Expand Up @@ -249,6 +226,30 @@ function isRootExpressionDynamic(expression: string): boolean {
expression === 'distance-from-center';
}

const dynamicConditionExpressions = new Set([
'in',
'==',
'!=',
'>',
'>=',
'<',
'<=',
'to-boolean'
]);

function collapsedExpression(expression: any): any {
if (dynamicConditionExpressions.has(expression[0])) {

for (let i = 1; i < expression.length; i++) {
const param = expression[i];
if (isDynamicFilter(param)) {
return true;
}
}
}
return expression;
}

// Comparison function to sort numbers and strings
function compare(a, b) {
return a < b ? -1 : a > b ? 1 : 0;
Expand Down
Loading