Reload tiles on adding sprite image

[ivovandongen]

Fixes #6536

[mention-bot]

@ivovandongen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jfirebaugh, @tmpsantos and @kkaefer to be potential reviewers.

[ivovandongen]

The current way of reloading introduces some flickering on symbols (most notably labels) as all sources for current symbol layers are reloaded. Not sure if there is around this so that only sources are updated that actually reference the sprite image in the currently loaded tiles.

cc @jfirebaugh

[jfirebaugh]

As I noted in #6536 (comment), I believe we should prohibit the use of addImage to update an existing image. Due to the use of a texture atlas and asynchronous layout, I don't think there is an easy way to support the ability to update an existing image on the fly

[ivovandongen]

@jfirebaugh

I believe we should prohibit the use of addImage to update an existing image.

Agreed. The title of the PR is somewhat misleading (changing now). This is actually fixing a case where you remove an image and later add one with the same id.

[ivovandongen]

@jfirebaugh If you want, I can add an exception in the case where an image is added for an existing id to prevent confusion there (now there is a log warning, only in the case the dimensions don't match).

[jfirebaugh]

This is actually fixing a case where you remove an image and later add one with the same id.

Yeah, but there's a symbol layer that references that ID the whole time right? It's functionally the same as updating an existing image.

Basically, I think the same rules should apply here as in #6612 (comment) -- no removing an image that's in use.

[ivovandongen]

@jfirebaugh

Yeah, but there's a symbol layer that references that ID the whole time right? It's functionally the same as updating an existing image.

Yes

Basically, I think the same rules should apply here as in #6612 (comment) -- no removing an image that's in use.

I agree on the principle. However, I do see people running in to this quite quickly when using this api. The first example I made, I ran into it myself. Wondering why the image didn't update. It also adds the need to always update the source and the sprite instead of just the sprite.

How about adding an explicit update call then indeed? This would be the most intuitive to use.

This does leave us with the original issues: what to do on removeImage. Currently, it removes the sprite, but the rendered tiles will still show it, this feels buggy as it will disappear / appear when zooming for example. We can do two things here; either throw an exception on removeImage when the sprite is in use (although this might require some additional bookkeeping) or reload the rendered tiles, which is probably more in line with what the developer would expect.

So the changes would be:

• Adding a check on addImage so it throws an exception when adding a sprite with an existing id
• Updating rendered tiles on removeImage / addImage to avoid "stale" tiles.
• Adding an updateImage call that will update an existing image (or add it in case it wasn't before)

[jfirebaugh]

The problem is that rendering is in an undefined state in the interval between removing the image from the SpriteAtlas and the tiles completing the reload. During that period, the existing buffers for symbol layers may contain references to coordinates within the sprite texture that no longer contain the expected image data -- portions that could have, for example, been overwritten by an unrelated image, possibly with different dimensions.

We could allow in place updates where the image dimensions do not change. This case should not require reloading tiles, only updating the texture and repainting.