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

Proposal: add expression-dependency key to spec properties that depend on internal calculations #6389

Closed
lbud opened this issue Mar 23, 2018 · 7 comments · Fixed by #6505 or #6521
Closed

Comments

@lbud
Copy link
Contributor

lbud commented Mar 23, 2018

Following from #6303 (comment), this is a proposal to add a new spec key, expression-dependency, for paint/layout properties whose display is linked to a special calculated property. These keys have string vales that specify the name of the internally calculated property upon which they depend, so currently those would be (including #6303):

"heatmap-color": {
    "expression-dependency": "heatmap-density"
}
"line-gradient": {
    "expression-dependency": "line-progress"
}

By adding a spec key we can remove special casing code, pave the way for future related properties, and move these specific instructions out of the docstrings (e.g.) and into generated documentation.

Thoughts, semantic or otherwise? @mapbox/gl-core

@lbud
Copy link
Contributor Author

lbud commented Mar 23, 2018

We'd also want to add a special key ("calculated"?) to the "dependencies" in the expressions section (

"heatmap-density": {
"doc": "Gets the kernel density estimation of a pixel in a heatmap layer, which is a relative measure of how many data points are crowded around a particular pixel. Can only be used in the `heatmap-color` property.",
"group": "Heatmap",
"sdk-support": {
"basic functionality": {
"js": "0.41.0"
}
}
)

@anandthakker
Copy link
Contributor

I wonder if it makes sense to consider the other expression-related style-spec properties at the same time:

  • function: "interpolated" | "piecewise-constant"
  • zoom-function
  • property-function

In particular, the latter two say something similar to what we're trying to say for heatmap-density / line-progress: zoom-function: true means an expression may refer to ["zoom"]; property-function: true means an expression may refer to ["get", key], ["has", key], ["geometry-type"], ["id"], or ["properties"].

What about something like:

  expression: {
    interpolated: true | false // replaces function: "interpolated" | "piecewise-constant", 
                               // and defaults to true for number, color, or array
                               // of number/color
    parameters: Array<"zoom" | "feature" | "heatmap-density" | "line-progress">
  }

So, e.g.:

"circle-color": {
    "expression": {
        "parameters": ["zoom", "feature"]
    }
}
"heatmap-color": {
    "expression": {
        "parameters": ["heatmap-density"]
    }
}
"line-gradient": {
    "expression": {
        "parameters": ["line-progress"]
    }
}
"line-dasharray": {
    "expression": {
        "interpolated": false
        "parameters": ["zoom"]
    }
}

The "interpolated" part is essentially @jfirebaugh 's proposal from #4194, which I still think makes sense.

I'm not sure if I actually like namespacing these under "expression" like I did above.

@jfirebaugh
Copy link
Contributor

Above suggestions sound good to me. One thing to look at is what will the propertyType and propertyValue functions from generate-style-code.js look like. Ideally I think they're switches on some style-spec property:

switch (property.type) {
case 'data-driven':
    return `DataDrivenProperty<${flowType(property)}>`;
case 'cross-faded-data-driven': // https://github.com/mapbox/mapbox-gl-js/pull/6303
    return `CrossFadedDataDrivenProperty <${flowType(property)}>`;
case 'cross-faded':
    return `CrossFadedProperty<${flowType(property)}>`;
case 'color-ramp':
    return `ColorRampProperty`;
default:
    return `DataConstantProperty<${flowType(property)}>`;
}

@mollymerp
Copy link
Contributor

mollymerp commented Apr 9, 2018

would something like this cover all the special casing we have now?
i'm not sure of is if parameters is the right word for describing the kinds of supported expressions 🤔

example for a property like line-pattern:

"expression": {
  "property-type": "cross-faded-data-driven", // new per @jfirebaugh's previous comment
  "interpolated": false, // replaces function: "interpolated" | "piecewise-constant", 
  "parameters": ["zoom", "feature"]  // Array<"zoom" | "feature" | "heatmap-density" | "line-progress">
                                     // replaces "zoom-function" ,`dependency` , 'property-function'
}

@lbud
Copy link
Contributor Author

lbud commented Apr 9, 2018

@mollymerp are you just hesitant about the term "parameters" or is there a feature/special case of line-pattern's behavior that you feel isn't covered in ☝️? If the former, do you have an alternative suggestion? If the latter, what is it about its expression behavior that can't be inferred from this taxonomy?

@jfirebaugh
Copy link
Contributor

👍 @mollymerp.

@mollymerp
Copy link
Contributor

@lbud oh, yes just the former. just because I think of parameters as function arguments/options and this is more about describing feature support. I don't feel super strongly about it, though. Maybe supported or something along those lines?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants