Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Cannot set style image to image of different dimensions #7618

Closed
1ec5 opened this issue Jan 6, 2017 · 8 comments · Fixed by #9213
Closed

Cannot set style image to image of different dimensions #7618

1ec5 opened this issue Jan 6, 2017 · 8 comments · Fixed by #9213
Labels
annotations Annotations on iOS and macOS or markers on Android bug Core The cross-platform C++ core, aka mbgl runtime styling

Comments

@1ec5
Copy link
Contributor

1ec5 commented Jan 6, 2017

mbgl::SpriteAtlas::_setSprite() disallows replacing an existing image with an image of different dimensions. I assume this is because the image would potentially no longer fit in the same location in the sprite sheet, but this behavior is confusing for anyone who isn’t familiar with how the sprite sheet works. Instead, if the new dimensions are larger than the original dimensions, this method should remove the old image and add the new image under the same name but in a new location within the sprite sheet.

/cc @tmpsantos

@1ec5 1ec5 added Core The cross-platform C++ core, aka mbgl crash runtime styling labels Jan 6, 2017
@1ec5 1ec5 added this to the ios-3.4.1 milestone Jan 6, 2017
@1ec5 1ec5 added bug and removed crash labels Jan 6, 2017
@jfirebaugh
Copy link
Contributor

This is prohibited because it's a complex operation to support. The process would have to be:

  • Add the new image in a new location in the sprite sheet.
  • Perform layout for all tiles that may have been using the old image (this is an asynchronous operation).
  • When it's certain that no active OpenGL buffers use the former texture coordinates, remove old image.

@jfirebaugh
Copy link
Contributor

Previously: #6735 (comment).

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 6, 2017

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.

What if the new dimensions fit within the old dimensions?

@jfirebaugh
Copy link
Contributor

What if the new dimensions fit within the old dimensions?

The only case where it's safe to update in place is if the dimensions don't change. Otherwise, existing buffers which use the prior dimensions may render incorrectly.

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 7, 2017

The same caveat applies to code that calls mbgl::Map::removeImage() and mbgl::Map::addImage() in quick succession with the same name, correct?

@jfirebaugh
Copy link
Contributor

Calling removeImage on an image that's in use is a no-no, just like calling removeSource on a source that's in use.

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 7, 2017

What I’m getting at is that e.g. -[MGLStyle setImage:forName:]’s documentation should have a note that calling it on any name that’s currently in use may result in undefined behavior (without mentioning dimensions). Then we can make it possible to resize an image, specifically for the not-in-use case (but without explicitly checking whether it’s in use). The fact that an image can be updated while in use when the dimensions stay the same would be a happy coincidence, but not something we’d officially support or document.

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 14, 2017

Per #4556 (comment), this issue also affects the annotation API, which implements custom point annotation images by removing the in-use style image and adding a new one under the same name.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
annotations Annotations on iOS and macOS or markers on Android bug Core The cross-platform C++ core, aka mbgl runtime styling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants