Smart setStyle (part 1)

[anandthakker]

Goal: make map.setStyle({ layers: [ ..., { one layout property different from map's current style } ] }) as good as map.setLayoutProperty() (modulo a pretty cheap diff)

• Add mapbox-gl-style-spec's diff implementation as Style#setState.
• Change Map#setStyle to delegate to Style#setState (instead of constructing a new Style instance).
• Properly handle changes to layer.source, layer.source-layer, or layer.type: Handle changes to a layer's 'source', 'source-layer', or 'type' in diffStyles mapbox-gl-style-spec#570
• deref incoming layers before processing
• Handle diff operations that rely on currently-missing setters:
  • setGlyphs, setSprite: I think we can reconstruct this.glyphSource and this.sprite, but need to make sure that tiles are then reloaded for all sources ticketed separately: Add Style#setGlyphs method #3625
  • setTransition: looks like this is used at update-time, so it may be safe to simply rely on setting this.stylesheet to the up-to-date style (i.e., no diff magic required)
  • setLayerProperty: used for every layer property other than {min,max}zoom, layout, paint, filter, metadata, -- I think this just leaves: paint.*, type, ref, source, and source-layer. If I didn't miss any, and we drop paint classes, then this should be a nonissue.
• Update unit tests for Map#setStyle
• Add tests for Style#setState
  • If Style's individual mutators are going to be basically become private helper functions, would it make sense to just change each of their unit tests into a corresponding test for setState?
• Add runtime-styling render tests for setStyle. Done in Add runtime-styling tests for 'smart setStyle' mapbox-gl-test-suite#170, pending review/merge
• Merge Clear source tiles for removed/re-added layer #3655 (fixes bug wherein removing a symbol layer and readding it as a circle layer causes an exception to be thrown)

Launch Checklist

• briefly describe the changes in this PR
• write tests for all new functionality
• document any changes to public APIs
• post benchmark scores
• manually test the debug page

[anandthakker]

@jfirebaugh @lucaswoj Here's an initial working "mvp," with a few lingering tasks listed in the top of the ticket. What are you thoughts on the questions re: unit tests?

[jfirebaugh]

What does diff do if layer.source, layer.source-layer, or layer.type have changed?

Looks like those properties are one of the cases where it'll generate an exception trying to call setLayerProperty, which doesn't exist. What should happen is a removeLayer/addLayer pair.

I also notice that for sources, any differences in properties currently results in a removeSource/addSource pair. This will not produce the expected result if there are layers actively using that source. Removing an actively-used source is not supported.

• Should we handle layer.ref in a style passed to setState? Reject it?

Use deref on the style input before doing anything else.

• Handle diff operations that rely on currently-missing setters:
  • setGlyphs, setSprite: I think we can reconstruct this.glyphSource and this.sprite, but need to make sure that tiles are then reloaded for all sources

Yeah, let's try adding setters for these independently of this PR.

• If Style's individual mutators are going to be basically become private helper functions, would it make sense to just change each of their unit tests into a corresponding test for setState?

Eventually, yes. But for now let's make parallel setStyle tests.

My inclination is to do all testing for this via test-suite.

[anandthakker]

Looks like those properties are one of the cases where it'll generate an exception trying to call setLayerProperty, which doesn't exist. What should happen is a removeLayer/addLayer pair.

Agreed: mapbox/mapbox-gl-style-spec#570

I also notice that for sources, any differences in properties currently results in a removeSource/addSource pair. This will not produce the expected result if there are layers actively using that source. Removing an actively-used source is not supported.

Oh, interesting. I suppose the most straightforward thing to do here would be to modify the style diff so that removeSource implies removeLayer for any layers that use that source, with removeSource/addSource pairs thus producing [removeLayer, removeLayer, removeSource, addSource, addLayer, addLayer, ...]

Yeah, let's try adding setters for these [setSprite, setGlyphs] independently of this PR

👍 in the interim, I propose to have Style#setState throw when it encounters these, and Map#setStyle fall back to new Style() in that case.

My inclination is to do all testing for this via test-suite

If/when we move the diffing logic to be resolved only once per 'batch', it will probably make sense for there to be some unit tests around that.

[anandthakker]

benchmark	master f1d1bf4	smart-set-style b8f9a9c
map-load	192 ms	171 ms
style-load	119 ms	122 ms
buffer	938 ms	948 ms
fps	60 fps	60 fps
frame-duration	4.5 ms, 0% > 16ms	4.5 ms, 0% > 16ms
query-point	0.81 ms	0.85 ms
query-box	69.82 ms	74.15 ms
geojson-setdata-small	6 ms	3 ms
geojson-setdata-large	216 ms	253 ms

[anandthakker]

@jfirebaugh @lucaswoj k, I think this should be ready for 👀, pending also the merge of upstream PR mapbox/mapbox-gl-test-suite#170 (and subsequent update of package.json here)

[AndyMoreland]

@anandthakker above you mention that all sources are going to need to reload tiles when the sprite changes -- does that mean that raster tile sources are going to redraw? It'd be super nice for my company's use case if we could avoid that because we add lots of new sprites at runtime and right now the full-map flickers are pretty awful.

[anandthakker]

@AndyMoreland Yes, as of now (both before and after this particular PR), the fully reloading the style is still going to happen, but there are plans to remove this necessity -- check out the tickets referenced here: #3625 (comment). I'm not up to speed on how all of those interrelate, but the one that most directly bears on this problem is probably #2058

[AndyMoreland]

Thanks for the response. Will check the referenced issues out.

[anandthakker]

Added a fix for #3633 in a540347

[anandthakker]

This should now only be pending mapbox/mapbox-gl-test-suite#170 and a review, of course. It passes all of those tests, so if that one's good, then this should be ready as well.

[anandthakker]

@jfirebaugh @lucaswoj updated checklist at the top to include #3655 as a prerequisite.

[anandthakker]

Updated setstyle.html example to use light-v9/dark-v9. Caveats:

• Had to force the sprite to be the same URL for both, due to Add map.setSprite(spriteUrl)method #2058
• Had to add a missing fill-outline-color to a layer, due to: Fix bug wherein unsetting and resetting fill-outline-color causes TypeError to be thrown #3657