mapbox / mapbox-gl-style-spec

76 stars 38 forks source link

Specify a default for property functions when property value is undefined #480

Closed lbud closed 7 years ago

lbud commented 8 years ago

In the mapbox-gl-function feature-defaults branch I'm adding reference as an argument so that when an input to a property function is undefined, it defaults to the spec default. However, it seems to me there should also be a way to specify a user-defined default for this behavior. Does it seem reasonable to add an optional key in a property function for default, like

extrusion-height: {
    stops: [[0, 0], [1000, 3000]],
    type: "exponential",
    property: "levels",
    default: 20
}

(so any extrusion without a levels property will have a height of 20, rather than the spec-defined default of 10)?

cc @lucaswoj @mollymerp @mourner

mourner commented 8 years ago

I like the idea. default property name can be confusing though, since it can be confused with extrusion-height default, not the default for the input property of a function. Alternatives? propertyDefault?

lbud commented 8 years ago

propertyDefault (or property-default — hyphens would be more consistent in this spec, I think) sounds good to me.

jfirebaugh commented 8 years ago

Could we unify the syntax with the proposals for token defaults in #104?

lbud commented 8 years ago

@jfirebaugh do you mean something like

extrusion-height: {
    stops: [[0, 0], [1000, 3000]],
    type: "exponential",
    property: "levels:20"
}

? (It feels weird to use a number in a string there…)

jfirebaugh commented 8 years ago

No, I mean something akin to the syntaxes proposed in https://github.com/mapbox/mapbox-gl-style-spec/issues/362#issuecomment-161857532 or https://github.com/mapbox/mapbox-gl-style-spec/issues/104#issuecomment-231640140, like "property": {"ref": "levels", "default": 20} or "property": [{"ref": "levels"}, 20].

I think at this point we need to have a vision that unifies https://github.com/mapbox/mapbox-gl-style-spec/issues/480, https://github.com/mapbox/mapbox-gl-style-spec/issues/104, and https://github.com/mapbox/mapbox-gl-style-spec/issues/47 or the function syntax will wind up becoming a mess of ad hoc additions.

lbud commented 8 years ago

Interestingly, I realized that because of the way evaluateExponentialFunction picks a function stop — iterating over them, waiting for either a break statement or the end — we can exploit it to set, essentially, a default:

{
    "stops": [[0, 0], [1000, 1000], ["other", 20]]
    "property": "height"
}

I don't feel like this is really the intended behavior or a clean solution, buuuut… 💭

@jfirebaugh @lucaswoj

lbud commented 8 years ago

Actually, my above comment is not true — it was working in my debugging but now I'm creating demos and realizing validation throws out non-number stops in number functions, and non-ascending-order stops (so -1 doesn't work). I can still achieve this effect by ensuring that the second to last stop is a number beyond what would ever exist in the data, and then making the next number even larger: stops: [[0, 0], [1000, 1000], [2000, 20]] but this feels even more unclean.

lucaswoj commented 8 years ago

I think there are two distinct cases being discussed in this thread:

1. Specify a function input value when the property is undefined

This is being discussed in https://github.com/mapbox/mapbox-gl-style-spec/issues/104. I will weigh in on that ticket.

2. Specify a function output value when the property is undefined

"extrusion-height": {
    stops: [[0, 0], [1000, 3000]],
    type: "exponential",
    property: "levels",
    default: 20
}

any extrusion without a levels property will have a height of 20, rather than the spec-defined default of 10

In some cases, we can take care of case 2 with a carefully constructed value and the case 1 feature. In other cases, doing so is impossible or mathematically tricky. For example:

"fill-color": {
    stops: [["scrub", "yellow"], ["industrial", "purple"], ...],
    default: "white",
    type: "categorical",
    property: "class"
}

We should have a primitive for this case. @lbud's syntax proposal is 👍

jfirebaugh commented 8 years ago

@lucaswoj Good clarification -- let's focus this issue on adding the ability to provide an output value for inputs which are not otherwise in the domain of the function.

An input value of "undefined", when the property is not present on a given feature, is a common special case. But a categorical function which doesn't wish to specify a stop for every possible value is another use case.

Then to go back to @mourner's first comment:

default property name can be confusing though, since it can be confused with extrusion-height default, not the default for the input property of a function

If we are talking about extrusion-height output default, not a default input, I think default is fine -- it's the same concept as a default label in a case statement, for example.

davidtheclark commented 7 years ago

A clarification question: By "undefined" are we referring to both undefined and null? Or is null going to have to be a separate conversation?

jfirebaugh commented 7 years ago

A clarification question: By "undefined" are we referring to both undefined and null? Or is null going to have to be a separate conversation?

We're not referring to either a literal undefined or null input value, but rather to the case where the feature properties have no key-value pair for the desired property at all.

null is a separate conversation. Functions and filters don't currently have a notion of null values, largely because neither does the vector tile specification.

lucaswoj commented 7 years ago

This issue was moved to mapbox/mapbox-gl-js#4124