Add "default" property for functions #4175

jfirebaugh · 2017-02-02T00:36:41Z

It is used in the following circumstances:

In categorical functions, when the feature value does not match any of the stop domain values.
In property and zoom-and-property functions, when a feature does not contain a value for the specified property.
In identity functions, when the feature value is not valid for the style property (for example, if the function is being used for a circle-color property but the feature property value is not a string or not a valid color).
In interval or exponential property and zoom-and-property functions, when the feature value is not numeric.

If no default is provided, the style property's default is used in these circumstances.

Implementing this required breaking changes to the function API in mapbox-gl-style-spec. The export is now a function which accepts the function definition and the spec JSON for a style property. It handles color parsing and validation of feature values internally.

Fixes #4124
Fixes #4144
Fixes #4146
Fixes #4150
Fixes #4018

Launch Checklist

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

benchmark	master `fac6358`	function-default `a90eaeb`
map-load	223 ms	99 ms
style-load	77 ms	77 ms
buffer	1,026 ms	1,036 ms
fps	60 fps	60 fps
frame-duration	3.2 ms, 0% > 16ms	3.1 ms, 0% > 16ms
query-point	0.87 ms	0.89 ms
query-box	64.55 ms	66.92 ms
geojson-setdata-small	7 ms	6 ms
geojson-setdata-large	148 ms	179 ms

lucaswoj · 2017-02-03T00:22:09Z

js/style-spec/CHANGELOG.md

+definition, and an object defining a style spec property. It handles color parsing and validation of feature values
+internally.
+* Functions now support a "default" property.
+* `parseColor` was promoted from gl-js.


cc @ryanbaumann @kronick

lucaswoj · 2017-02-03T00:24:10Z

js/style-spec/function/index.js

@@ -20,15 +28,38 @@ function createFunction(parameters, defaultType) {
        const zoomAndFeatureDependent = parameters.stops && typeof parameters.stops[0][0] === 'object';
        const featureDependent = zoomAndFeatureDependent || parameters.property !== undefined;
        const zoomDependent = zoomAndFeatureDependent || !featureDependent;
-        const type = parameters.type || defaultType || 'exponential';
+        const type = parameters.type || (propertySpec.function === 'interpolated' ? 'exponential' : 'interval');


Have you thought about renaming the propertySpec.function "enum" values now that this is part of our public API?

Filed #4194 for what I think we should do there.

lucaswoj · 2017-02-03T00:26:27Z

js/style-spec/function/index.js

            }
            fun = function(zoom, feature) {
                return outputFunction(evaluateExponentialFunction({
                    stops: featureFunctionStops,
                    base: parameters.base
-                }, zoom)(zoom, feature));
+                }, propertySpec, zoom)(zoom, feature));


Why do you need to pass propertySpec to evaluateExponentialFunction both as an argument and embedded within featureFunctionStops?

Ah. I misread the code. This is 👍

Where is it embedded within featureFunctionStops?

Oop, didn't see your followup comment. 👍

lucaswoj · 2017-02-03T00:27:04Z

js/style-spec/function/index.js

+            // Special case for fill-outline-color, which has no spec default.
+            if (evaluatedLower === undefined || evaluatedUpper === undefined) {
+                return undefined;
+            }


lucaswoj · 2017-02-03T00:29:23Z

package.json

@@ -16,8 +16,6 @@
    "@mapbox/unitbezier": "^0.0.0",
    "brfs": "^1.4.0",
    "bubleify": "^0.5.1",
-    "csscolorparser": "^1.0.2",
-    "csscolorparser": "~1.0.2",


We don't currently install the dependencies from js/style-spec/package.json so this may still be needed

Isn't that what this does?

Ah. Yes. The postinstall hook. 🚢

* No more than one level of nesting * Remove nonsensical or redundant tests * Use correct terminology * Test identity functions properly * Complete test coverage

If the text-field property is data-driven, the value of layout['text-field'] is not defined.

It is used in the following circumstances: * In categorical functions, when the feature value does not match any of the stop domain values. * In property and zoom-and-property functions, when a feature does not contain a value for the specified property. * In identity functions, when the feature value is not valid for the style property (for example, if the function is being used for a `circle-color` property but the feature property value is not a string or not a valid color). * In interval or exponential property and zoom-and-property functions, when the feature value is not numeric. If no default is provided, the style property's default is used in these circumstances. Implementing this required breaking changes to the function API in mapbox-gl-style-spec. The export is now a function which accepts the function definition and the spec JSON for a style property. It handles color parsing and validation of feature values internally.

jfirebaugh · 2017-02-03T01:15:30Z

js/data/bucket/symbol_bucket.js

        const textFont = layout['text-font'];
        const iconImage = layout['icon-image'];

-        const hasText = layout['text-field'] && textFont;
+        const hasText = textFont && (!layer.isLayoutValueFeatureConstant('text-field') || layout['text-field']);


FYI @anandthakker.

If the text-field property is data-driven, the value of layout['text-field'] is not defined.

@jfirebaugh ah, whoops - I made the bad assumption that layout contained the style JSON.

jfirebaugh mentioned this pull request Feb 2, 2017

[core] Add support for data-driven styling mapbox/mapbox-gl-native#7372

Merged

mourner added the under development label Feb 2, 2017

jfirebaugh force-pushed the function-default branch from 19b4d67 to a90eaeb Compare February 2, 2017 23:08

jfirebaugh removed the under development label Feb 2, 2017

jfirebaugh requested a review from lucaswoj February 2, 2017 23:15

lucaswoj reviewed Feb 3, 2017

View reviewed changes

jfirebaugh added 2 commits February 2, 2017 17:01

Clean up function tests

f05bcd6

* No more than one level of nesting * Remove nonsensical or redundant tests * Use correct terminology * Test identity functions properly * Complete test coverage

Simplify

ee9a1d7

jfirebaugh force-pushed the function-default branch from a90eaeb to 90a5d3d Compare February 3, 2017 01:01

jfirebaugh added 2 commits February 2, 2017 17:14

Make test for symbol layer text more robust

f7e2e8f

If the text-field property is data-driven, the value of layout['text-field'] is not defined.

jfirebaugh force-pushed the function-default branch from 90a5d3d to 5e61f88 Compare February 3, 2017 01:14

jfirebaugh commented Feb 3, 2017

View reviewed changes

anandthakker approved these changes Feb 3, 2017

View reviewed changes

jfirebaugh merged commit e328b80 into master Feb 3, 2017

jfirebaugh deleted the function-default branch February 3, 2017 18:01

ashleyclough mentioned this pull request Mar 25, 2017

Regression in color property styling between 0.32.x and 0.33.x #4496

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "default" property for functions #4175

Add "default" property for functions #4175

jfirebaugh commented Feb 2, 2017 •

edited

Loading

lucaswoj Feb 3, 2017

lucaswoj Feb 3, 2017

jfirebaugh Feb 3, 2017

lucaswoj Feb 3, 2017

lucaswoj Feb 3, 2017

jfirebaugh Feb 3, 2017

jfirebaugh Feb 3, 2017

lucaswoj Feb 3, 2017

lucaswoj Feb 3, 2017

jfirebaugh Feb 3, 2017

lucaswoj Feb 3, 2017

jfirebaugh Feb 3, 2017

anandthakker Feb 3, 2017

Add "default" property for functions #4175

Add "default" property for functions #4175

Conversation

jfirebaugh commented Feb 2, 2017 • edited Loading

Launch Checklist

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

Choose a reason for hiding this comment

jfirebaugh commented Feb 2, 2017 •

edited

Loading