Rework spec function/expression taxonomy #6505

lbud · 2018-04-12T00:02:14Z

This PR reworks the taxonomy to describe how a property can use functions/expressions, as described in #6389 (comment) and #6389 (comment):

type ExpressionType = 'data-driven' | 'cross-faded' | 'cross-faded-data-driven' | 'color-ramp' | 'data-constant' | 'constant';

type ExpressionParameters = Array<'zoom' | 'feature' | 'heatmap-density' | 'line-progress'>;

type ExpressionSpecification = {
    interpolated: boolean,
    parameters: ExpressionParameters
}

StylePropertySpecification = {
    ...
    'property-type': ExpressionType,
    'expression': ExpressionSpecification
}

so now any property all properties include a property-type key, and for all property-types except constant also an expression property, i.e.

    "type": "number",
    "property-type": "data-driven",
    "expression": {
        "interpolated": true,
        "parameters": [
            "zoom",
            "feature"
        ]
    }

Benchmarks: http://bl.ocks.org/lbud/raw/2ac3b8869074c112802166978cea8345/ (I couldn't reproduce the Layer discrepancies on subsequent isolated bench runs).

Closes #6389
Supersedes / closes #4194
Refs #6430
Refs #6303

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
post benchmark scores
manually test the debug page

anandthakker

This feels so much cleaner ☺️

anandthakker · 2018-04-12T00:19:56Z

src/style-spec/style-spec.js

+type ExpressionParameters = Array<'zoom' | 'feature' | 'heatmap-density' | 'line-progress'>;
+
+type ExpressionSpecification = {
+    type: ExpressionType,


Looks like field is currently called property-type, not type

It feels slightly weird to me to have property-type nested under expression. Would it make sense to move it up to the layout/paint property specification, and also add constant as one of the options for properties that can't take any expression? E.g.:

"visibility": { "property-type": "constant", // no "expression" field "type": "enum", /* ... */ }

"line-cap": { "property-type": "data-constant", "expression": { "interpolated": false, "parameters": ["zoom"] }, "type": "enum", /* ... */ }

Then we could always use propertySpec["property-type"] and never have to rely on the presence/absence of the "expression" field

Sounds good to me @anandthakker!

anandthakker · 2018-04-12T00:27:40Z

src/style-spec/util/properties.js

+// @flow
+
+export function isPropertyFunction(spec: Object): boolean {
+    const dataDrivenTypes = new Set(['data-driven', 'cross-faded-data-driven']);


I don't think we can use Set / Map, since they'd require polyfills for IE 😢

Ah, this would have showed up in the style-spec build test, but it's not getting run on CI 😬 #6433

anandthakker · 2018-04-12T00:31:13Z

src/style-spec/util/properties.js

+
+export function isZoomFunction(spec: Object): boolean {
+    return spec.expression && spec.expression.parameters.indexOf('zoom') > -1;
+}


Rename to isPropertyExpression and isZoomExpression?

anandthakker · 2018-04-12T19:16:22Z

build/generate-style-code.js

+            return `ColorRampProperty`;
+        case 'data-constant':
+        default:
+            return `DataConstantProperty<${flowType(property)}>`;


Just for completeness, should we also list case 'constant': here? (Alternatively, since DataConstantProperty<T> isn't really intended to represent a totally constant property, it could be case 'constant': throw new Error("Constant properties unimplemented");)

anandthakker · 2018-04-12T19:16:41Z

build/generate-style-code.js

+            return `new ColorRampProperty(styleSpec["${type}_${property.layerType}"]["${property.name}"])`;
+        case 'data-constant':
+        default:
+            return `new DataConstantProperty(styleSpec["${type}_${property.layerType}"]["${property.name}"])`;


Same as above

jfirebaugh · 2018-04-12T18:37:58Z

src/style-spec/util/properties.js

@@ -0,0 +1,13 @@
+// @flow
+
+export function isPropertyExpression(spec: Object): boolean {


isPropertyExpression → supportsPropertyExpressions -- or something similar that doesn't imply that the function is testing whether the input itself is a property expression. (Similarly for isZoomExpression and isInterpolated.)

Also, can spec be typed as StylePropertySpecification?

jfirebaugh · 2018-04-12T19:09:55Z

src/style-spec/function/convert.js

@@ -236,8 +236,8 @@ function appendStopPair(curve, input, output, isStep) {
 function getFunctionType(parameters, propertySpec) {
    if (parameters.type) {
        return parameters.type;
-    } else if (propertySpec.function) {
-        return propertySpec.function === 'interpolated' ? 'exponential' : 'interval';
+    } else if (propertySpec.expression) {


Should this be asserting propertySpec.expression? We shouldn't be trying to create a function for something that doesn't support expressions.

👍 this was a bad workaround for a flow error — will fix.

…nd params

lbud requested review from jfirebaugh and anandthakker April 12, 2018 00:04

anandthakker reviewed Apr 12, 2018

View reviewed changes

lbud requested a review from anandthakker April 12, 2018 18:32

anandthakker approved these changes Apr 12, 2018

View reviewed changes

jfirebaugh approved these changes Apr 12, 2018

View reviewed changes

Lauren Budorick added 3 commits April 16, 2018 14:22

Use new 'expression' object to indicate property-type, interpolable a…

fd051d8

…nd params

spec.expression.property-type -> spec.property-type

dc5decc

Minor review followups

caba650

lbud force-pushed the expression-spec branch from d37c94c to caba650 Compare April 16, 2018 21:23

lbud merged commit 77b9821 into master Apr 16, 2018

lbud deleted the expression-spec branch April 16, 2018 21:47

lbud pushed a commit that referenced this pull request Apr 16, 2018

Revert "Rework spec function/expression taxonomy" #6505

c01e491

lbud restored the expression-spec branch April 16, 2018 23:02

lbud mentioned this pull request Apr 16, 2018

Rework spec function/expression taxonomy (recreate #6505) #6521

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework spec function/expression taxonomy #6505

Rework spec function/expression taxonomy #6505

lbud commented Apr 12, 2018 •

edited

Loading

anandthakker left a comment

anandthakker Apr 12, 2018

anandthakker Apr 12, 2018

lbud Apr 12, 2018

anandthakker Apr 12, 2018

anandthakker Apr 12, 2018

anandthakker Apr 12, 2018

anandthakker Apr 12, 2018

anandthakker Apr 12, 2018

jfirebaugh Apr 12, 2018

jfirebaugh Apr 12, 2018

lbud Apr 16, 2018

		@@ -0,0 +1,13 @@
		// @flow

		export function isPropertyExpression(spec: Object): boolean {

Rework spec function/expression taxonomy #6505

Rework spec function/expression taxonomy #6505

Conversation

lbud commented Apr 12, 2018 • edited Loading

Launch Checklist

anandthakker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbud commented Apr 12, 2018 •

edited

Loading