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

Runtime styling sometimes doesn't take effect #4738

Closed
kkaefer opened this issue May 22, 2017 · 4 comments
Closed

Runtime styling sometimes doesn't take effect #4738

kkaefer opened this issue May 22, 2017 · 4 comments
Assignees
Labels

Comments

@kkaefer
Copy link
Contributor

kkaefer commented May 22, 2017

When using a GeoJSON source, and setting runtime styling properties, then updating the GeoJSON source data, the changes to the style don't always take effect.

https://jsfiddle.net/ygxuk5zy/ contains a reduced test case (replace your access token). This is how it should look:

screen shot 2017-05-22 at 23 05 13

This is how it actually looks:

screen shot 2017-05-22 at 23 05 20

@anandthakker
Copy link
Contributor

Ah - the problem here is that map.getLayer().setPaintProperty() is not intended to be a user-facing method. In fact, that's true of all the methods of the StyleLayer class; I think map.getLayer() should really be returning a serialized copy of the layer -- I will ticket this separately.

Replacing map.getLayer('route').setPaintProperty('line-width', 5) with map.setPaintProperty('route', 'line-width', 5) fixes the fiddle.

@jfirebaugh
Copy link
Contributor

jfirebaugh commented Jun 22, 2017

There's an actual bug here too, however. Consider the following execution sequence:

Main Worker
geoJSONSource.setData(new);
sends geojson.loadData message
setPaintProperty(layer, prop, new); (using DDS)
receives geojson.loadData message
performs indexing
sends response
receives response
sends reloadTile message

And then the following (in independent relative ordering):

Main Worker
🎉 animation frame 🎉 receives reloadTile message
style.update(...) parses the tile using new data but old property definition 💣
broadcast updateLayers
send reloadTile (due to the DDS property change)

If the old property definition depended on attributes that aren't present in the new data -- for example, if the property value is a categorical source function where the source domain values are unique feature IDs -- then the result will fall back to default values (e.g. black color).

@jfirebaugh jfirebaugh reopened this Jun 22, 2017
@jfirebaugh
Copy link
Contributor

@anandthakker Can you take a look at this? I am finding it difficult to see any easy fix without large scale refactoring (#2291, #4875), so that #3692 is feasible.

@anandthakker
Copy link
Contributor

@jfirebaugh until we can take on the necessary refactoring to solve the problem more systematically, a temporary measure to patch this would be to expose something like SourceCache#{pause,resume}(). The Style could use this to prevent tiles from being (re)loaded between the call to a runtime-styling method and the subsequent animation frame => updateLayers broadcast.

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

No branches or pull requests

3 participants