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

Fix feature-state dependent styles for *-pattern properties #7207

Open
mollymerp opened this issue Aug 27, 2018 · 12 comments
Open

Fix feature-state dependent styles for *-pattern properties #7207

mollymerp opened this issue Aug 27, 2018 · 12 comments

Comments

@mollymerp
Copy link
Contributor

mollymerp commented Aug 27, 2018

per #6289 (comment)

In the recent addition of data-driven styling support for *-pattern properties, feature-state dependent expressions do not work. The removal of the possibleOutputs state (#6289 (comment)) makes it possible that the icons needed forfeature-state related paint array updates may not be available in the tile's ImageAtlas at update time.

We haven't run into this before because the other data-driven properties that require glyph/icon assets are layout properties which don't support feature-state expressions yet.

I see a couple options on how to fix this:

  • request new images in the CrossFadedCompositeBinder#updatePaintArrays method that runs on the main thread when feature states are updated/changed and update the tile's ImageAtlas
    • 👎 binder would need access to ImageManager, binder would have to send the new ImageAtlas to the tile somehow
  • pass available feature-states to the buckets at parse time so the expressions can be evaluated with feature state and all needed images can be requested
    • 👎 additional payload on worker thread transfer, doesn't cover the case where no features have the state value that is used in the expression when it is set as a paint property
  • reintroduce possibleOutputs state and require that feature-state dependent expressions for cross-faded-data-driven properties be contained as literals
    • 👎 lots of special-case logic and behavior, might be confusing/unexpected to users
  • not support feature-state expressions for cross-faded-data-driven properties
    • 👎 this is the current behavior, but it fails silently

leaning towards the first option here, but curious if anyone has a different preference or another idea.
cc @asheemmamoowala @jfirebaugh

@jfirebaugh
Copy link
Contributor

I lean toward the first option, although it sounds like we have a few other issues also pushing us towards the second one.

Overall, it feels to me like we might have to think about rearchitecting how we partition work to the worker.

  • Divide it up much more granularly -- per bucket.
  • Use SharedArrayBuffer to share feature data?
  • Have the option of doing certain buckets synchronously on the main thread where necessary. Like Add a synchronous version of GeoJSONSource#setData #2275 but maybe for other cases too, like when a property uses feature-state or when setLayoutProperty is used.

@mollymerp mollymerp self-assigned this Sep 6, 2018
@charandas
Copy link

Any update on this @mollymerp?

@charandas
Copy link

I would love some attention on this from the team, it's holding me on a fork since the days of #6289.

@asheemmamoowala
Copy link
Contributor

@charandas currently there are no plans to implement this because it requires a lot of effort to implement, and our team is focused on higher priority issues at the moment.

@songololo
Copy link

Would it please be possible to explicitly state in the docs which style specs are supported by feature-state operations?

@asheemmamoowala
Copy link
Contributor

@songololo Thank you for the suggestion. It turns out that we already have the relevant infromation to update the docs. You can follow the docs update in https://github.com/mapbox/mapbox-gl-js-docs/pull/171

@asheemmamoowala
Copy link
Contributor

Native PR is moving at: mapbox/mapbox-gl-native#15872

@jmurphy-asurity
Copy link

Is there any chance this is going to get worked on? @jmalanen says the native PR is on hold? This is a critical feature for us.

@derrickrung
Copy link

My team also has a deep shared interest in this piece of work. Any updates on when we might possibly see some traction on this? Or any ways we can help catalyze that?

@wtravO
Copy link

wtravO commented Dec 2, 2020

Any updates on this?

@jmurphy-asurity
Copy link

I'm still stuck on a fork because of this. It's really frustrating.

@SWRB
Copy link

SWRB commented Dec 17, 2020

Hi @asheemmamoowala I've got some customers that aren't able to enjoy the benefits of v2 due to reliance on feature state to show multiple data overlays. Do we have any updates in v2 that might be a solve here?

@karimnaaji karimnaaji added this to the v2.1.0 milestone Jan 4, 2021
@ryanhamley ryanhamley removed this from the v2.1.0 milestone Jan 11, 2021
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