Mapbox-gl-js: Add Map#{add,remove}Image

[scothis]

In Mapbox Studio, keeping the sprite sheet in sync between the Studio UI, mapbox.com and the map is tricky and has been a consistent source of subtile bugs. Since we have the sprite json and png within Studio, we'd like a way to manually manage the sprite sheet in mapbox-gl-js.

map.setSprite(spriteJson, springPNG)

The json property is the metadata object, The png property could either be a data URI or a Blob.

This method should be supported in the style_batch API as well.

This API proposal would overload the API proposal in #2058, it has two args instead of one.

[jfirebaugh]

I don't think we should support this; it would make it impossible to implement a 100% fidelity getStyle() method.

[scothis]

Would you be ok with leaving this functionality undocumented, or made as a private API? It would indeed break compatibility with getStyle().

[lucaswoj]

We could have both if we allow embedded base64 encoded sprites in the stylesheet https://github.com/mapbox/mapbox-gl-style-spec/issues/220

[iH8]

Endusers myself included are really having trouble setting/creating spritesheets. But i gather from this answer that there's a way to do so in Studio? I'de really like it if somebody would take a look at this question: http://stackoverflow.com/questions/35565968/how-can-i-add-markers-to-the-mapbox-light-map-style That's not the first question regarding sprites and will certainly not the last. I hope there's a good solution so i can help these people. Sorry if this is not the place, feel free to delete this comment. Thanks in advance.

[dcervelli]

We have a hacky workaround for this (#2277) that uses stringified JSON in the ImageSprite base to essentially implement a map.setStyle(spriteJson, spritePng) call. We create the png programmatically and send it over as a base64 data:/image/png URL. It serves our needs for now but ideally we'd being used a supported path. Is there any consensus around mapbox/mapbox-gl-style-spec#220 or another way of doing this?

[AndyMoreland]

I work with @dcervelli on this and we've spoken to @ryanbaumann about this problem offline. We're still maintaining our fork (with this commit: https://github.com/AndyMoreland/mapbox-gl-js/commit/2a3d8799068bd046e72bcdffd0439c8b35c67854) over at https://github.com/AndyMoreland/mapbox-gl-js/commits/0-22-1-sprite-and-npe-and-no-validation-and-fast-selection.

With a recent update of mapbox-gl-js it seems like the raster tile source changed some of its behavior so that when we update the sprite map using our workaround we see all of the background tiles flicker and redraw. This means that changing a single point's sprite requires redrawing everything on the map, which is not a great user experience.

It'd be awesome if we could get some sort of API for changing the sprite map without redrawing everything. We'd be happy to take a painful API right now. We can guarantee only additive changes -- or things like that -- if necessary.

[jfirebaugh]

We added addImage and removeImage in gl-native, and I'm 👍 on porting them to GL JS.

I'm less concerned about round-tripping now than I was in https://github.com/mapbox/mapbox-gl-js/issues/2059#issuecomment-179979564. getStyle() will become less important if we go in the direction of #1989, and https://github.com/mapbox/mapbox-gl-style-spec/issues/220 proposes inlining images such that mutations via addImage and removeImage _could_ be reflected by getStyle().

[lucaswoj]

If I were an end user, I would likely believe that a method called Map#addImage would _add_ an _image_ to the map -- a shortcut for adding an image source and raster style layer.

Is it too late to adjust course slightly and call it Map#addIcon or Map#addIconImage?

[jfirebaugh]

The name was chosen based on the intended future direction described in https://github.com/mapbox/mapbox-gl-style-spec/issues/220.

[lucaswoj]

Thanks! Got it. I'll take the naming conversation upstream.

[lucaswoj]

Based on the upstream conversation, Map#addStyleImage would be consistent with GL Native's GLMapStyle#addImage and disambiguate between image sources and "style images."

[jfirebaugh]

I'm not a fan of addStyleImage. We don't use a Style adjective in any other runtime styling APIs. If that was the naming convention, we would have Map#addStyleLayer, Map#addStyleSource, etc.

Given our planned direction in #2741, I think it's acceptable to introduce a Map#addImage method now with the intent that it will eventually move to Style#addImage.

[pwilczynski]

From the roadmap, it sounds like work on this might be coming in the next sprint, does that sound right?

[lucaswoj]

As the roadmap says:

Upcoming

_Things we hope to start building in the next several months._

😄

[pwilczynski]

@lucaswoj / @alinapaz: @AndyMoreland and I were looking at the way you guys are looking to fix this, and want to be really clear about what our problem is with the current implementation. While adding an API to change the sprite sheet will be helpful, our key issue is that all layers are redrawn when the sprite sheet is changed. This causes a really bad user experience for us when we add new layers to the map with new sprites, as the raster base layer flashes repeatedly.

[lucaswoj]

@pwilczynski Yes, smooth sprite transitions are in scope for this project 😄

[pwilczynski]

@lucaswoj - really excited about this, but struggling a bit to figure out where to inject our UINT32 patch in the new model. Any suggestions?

[lucaswoj]

@pwilczynski The changes shouldn't affect your patch. Are you seeing a regression?

The sprite atlas copies images out of the sprite and into a new texture. The Map#addImage adds the ability to copy standalone images into the texture too. The coordinate system hasn't changed.

[anandthakker]

@pwilczynski am I right in thinking that the reason for the uint32 patch was to work around a limit in the number of symbols that could be present in a single tile? If so, then I _think_ the patch may no longer be necessary as a result of a previous change (https://github.com/mapbox/mapbox-gl-js/pull/4217) that removed this limitation.

[AndyMoreland]

yeah, that was the reason.

there are still warnings in the code alluding to the problems:

https://github.com/mapbox/mapbox-gl-js/blob/master/src/data/bucket/symbol_bucket.js#L689
https://github.com/mapbox/mapbox-gl-js/blob/master/src/data/bucket/symbol_bucket.js#L717

[AndyMoreland]

I haven't finished integrating with the new addImage api so I can't verify that there is or is not a regression here yet.