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

Add consistent support for variables in styling #15146

Open
tschaub opened this issue Sep 17, 2023 · 5 comments
Open

Add consistent support for variables in styling #15146

tschaub opened this issue Sep 17, 2023 · 5 comments

Comments

@tschaub
Copy link
Member

tschaub commented Sep 17, 2023

The WebGLTile and WebGLPoints layers accept a variables property in the style object passed to the constructor.

// for webgl tile or points layers
const layer = new Layer({style: {variables}});

Then to update those variables, the updateStyleVariables method can be called:

// for webgl tile or points layers
layer.updateStyleVariables(newVariables);

Although the parser supports var expressions, the canvas vector layer doesn't currently accept variables for styling (see #14780 (comment)). We could try to accept a varaibles as an additional property in a flat style literal, but this would be awkward for arrays of flat styles or rules.

It would be more straightforward to add a variables or styleVariables option to the constructor (instead of trying to tuck it in the style object(s) we accept).

// one alternative for vector layers
const layer = new Layer({style, variables});

If we do this, we could also add support for a top level variables option for the WebGLPoints and WebGLTile layers and could deprecate the variables property inside the style object. And then we could have consistency in the constructors and in an updateStyleVariables method.

But it is also probably worth considering allowing style variables at the map level. This would make it so variables related to styling to could be provided and updated in one place. In cases where the same variable is used across multiple layers, this would make it so you don't have to call layer.updateStyleVariables() on each layer individually.

// an alternative to doing this at the layer level is to do it at the map level
const map = new Map({layers, variables});
map.updateVariables(newVariables);

I know variables is a pretty general name. Could also be styleVariables (although that is kind of clunky).

Curious to hear what others think.

@MoonE
Copy link
Contributor

MoonE commented Sep 17, 2023

I'd prefer providing variables as a separate constructor option. As a property of the Map it feels a bit out of place.

@jahow
Copy link
Contributor

jahow commented Sep 18, 2023

I like the idea of having variables on layers as well, as layers are meant to hold the representation information for their data. This could even be merged with the concept of properties using set/get methods.

@jahow
Copy link
Contributor

jahow commented Nov 21, 2023

I'm coming back to this since we need to resolve this question in order to merge both the flat and webgl style formats.

Having variables on the map might be the best way forward IMO:

  • adding a variables option on the map wouldn't conflict with other notions; on the other hand, a variables on a layer would be similar to the existing properties option so we'd have to differentiate it by calling it styleVariables
  • we could then have the corresponding updateVariables() and getVariables() methods on the map
  • most of the time I feel like style variables don't only make sense for one layer, but have a meaning for the whole map: things such as filtering by attributes, changing colors or size, etc.

The fact is that Map instances can also have properties accessed using set/get/getProperties etc., and I'm not sure how much these two APIs could coexist without creating confusion. Should we offer a similarly named API such as setVariables/applyVariables?

@gberaudo
Copy link
Member

I like the style expressions because they allow compact serializable of dynamic styling.
I also like the layer.updateStyleVariables(newVariables); method on the WebGL layer, which provides values, not related to a particular feature, that the styles can resolve (if I understood correctly).

Removing variables from the style makes sense for the reasons already mentionned. Also, style and variables have different lifecycle and are certainly handled in different places in application code.

I agree that variables at the map level would be useful. What about resolving variables both at the layer and map levels?
We could pass the current map variables in the framestate object and fallback to it when the variable is not found in the layer variables.

I think that styleVariables is a good name that will avoid confusion with properties.

I don't think we need to be able to set variables individually (because contrary to properties there is no listeners that would be triggered).
Having a getStyleVariables() would be great for introspection that libraries like OL-Cesium rely on.

@ahocevar
Copy link
Member

ahocevar commented Feb 7, 2024

Thinking about how this could be used in the future in ol-mapbox-style, I think it could make sense to have style variables on ol/layer/Base (i.e. the common base class of ol/layer/Group and ol/layer/Layer). Variables would then work similar to opacity in the layer state, which is hierarchical (layer group -> layer). For style variables, we could Object.assign going through the hierarchy, i.e. deeper in the hierarchy takes precedence (like in css), and handle it in the layer state as well.

To define variables on the map, we could expose constructor options and/or methods that set the variables on the map's top-level layer group.

The key benefit of this css-like approach would be that layers could still override variables that are defined on the map's root layer group).

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

No branches or pull requests

5 participants