-
Notifications
You must be signed in to change notification settings - Fork 1.3k
add onStyleImageMissing to allow dynamically loaded or generated images #14253
Conversation
We need some asynchronous way of telling the renderer when the listener has been called. This is important on Android when the renderer and map exist on separate threads.
First of all, sorry for not taking the time to review and respond to mapbox/mapbox-gl-js#7587 in greater detail before the design for this event was implemented in GL JS and now ported here. Hopefully some of this feedback is still actionable, either here or in related work. For now, this feedback is specifically about the API proposed here for lazily loading images, not about its intended use for animation (#14125). I’m a big fan of the idea that developers can supply programmatically generated content on demand. Computed shape sources already facilitate extremely dynamic or repetitive content, and computed raster sources would allow for tighter integration with platform 2D drawing libraries. (#7471 proposes the iOS equivalent to It seems like The nice thing about the lambda expressions proposed in #7860 is that they would be embedded in the context in which they’re evaluated. By analogy, developers really like the completion block in There was concern in #7860 about having to block worker threads on expressions that can only be evaluated on the main thread. But the event proposed here would also have to be called on the main thread, as with any MGLMapViewDelegate method, because map view delegates are typically UI classes. |
For 1 maybe there could be a parallel notification to ask if canRemoveImage? |
@eklipse2k8, @ansis - one thing to consider: what would the map display when it's waiting for the image to be generated asynchronously? And when the image is finally added, will the placement of the features be stable or will they be reshuffled to fit the image? |
@1ec5 Are you suggesting that the optionality to provide an image in a callback synchronously would be preferable? For the The |
The profile picture use case described in mapbox/mapbox-gl-js#7587 implies that the images are not only lazily loaded but also lazily fetched from the network. On the other hand, an asynchronous callback model is error-prone. What happens if the corresponding mbgl method never gets called? This PR currently relies on the developer to call Lazy network requests are also poorly suited to animation, which is apparently an intended use case. Imagine the user having to sit through each frame loading on demand in series, only animating smoothly after looping. In practice, I think we can expect that an application is either generating each frame completely programmatically (which can be synchronous) or is receiving the animated image wholesale. An asynchronous, frame-by-frame API for animation begins to sound like a streaming video API, which seems like overkill for icons, at least from the perspective of a developer wanting support for animated image formats (#10595).
At least on iOS, the map is guaranteed to try to rerender 60 times a second (unless another frame rate is specified), so that may not be a problem visually. Regardless, if the developer is required to add the image to the style themselves, they’ll expect the model ( |
- only reparse tiles when the size of the updated icon is different - make updates for same-sized icons much faster - fix crashes
@1ec5 thanks for all the feedback!
This would be a pretty deep change that has a bunch of potential issues. As you mentioned, threading presents some challenges (also in -js). Crossing the jni boundary could be slow. Deduplication could be tricky. These apply here as well, but on a different scale than supporting lambdas everywhere. I'd say this idea is worth researching though!
Yep! the iOS SDK could implement it this way if that is following the precedent. I opted for not doing this in core because of the lack of precedent.
Yep! Those exact tradeoffs were discussed for the -js implementation. It isn't a clear choice which is better but I chose the current approach because lack of discoverability seemed like less of a trap than making your map hang. Your idea about returning an image in the delegate sounds like it could improve the discoverability!
This pr isn't really aimed at animation. The main use case is generating static icons. But this pr also optimizes updating images with |
@alexshalamov @asheemmamoowala Thanks for your reviews. I've added another commit 6089031 which fixed some of your suggested changes and also implemented patching for image atlases. Previously If you could review it, that would be great! cc @kkaefer in case you have interest in reviewing this port. |
This PR doesn't support asynchronous images but if you know the size of the image you can provide a blank image and then update it later with // in your MapObserver implementation
onStyleImageMissing(const std::string& id) {
uint32_t width = 10;
uint32_t height = 10;
mbgl::PremultipliedImage image({ width, height });
auto image = std::make_unique<mbgl::style::Image>(id, std::move(image), 1);
map->getStyle().addImage(std::move(image));
// after adding the image generate the real one
loadAndGenerateImage(id).
}
loadAndGenerateImage(const std::string& id) {
// load all your resources and then call generateImage(...)
...
}
generateImage(const std::string& id) {
uint32_t width = 10;
uint32_t height = 10;
mbgl::PremultipliedImage image({ width, height });
... // modify `image` with your contents
// update the image
map->getStyle().addImage(std::move(image));
} |
@ansais is this the only way to provide images asynchronously or a performance recommendation? |
src/mbgl/renderer/image_manager.hpp
Outdated
void removeImage(const std::string&); | ||
|
||
void getImages(ImageRequestor&, ImageRequestPair&&); | ||
void removeRequestor(ImageRequestor&); | ||
void imagesAdded(); | ||
|
||
std::map<std::string, uint32_t> updatedImageVersions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have to be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ImageAtlas accesses this directly. Is there a better way?
observer.onStyleImageMissing(id); | ||
} | ||
|
||
done(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a done
function? What does it do that we can't do just through the mere fact of returning from this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be wrong here, but: on Android the map and renderer are on different threads. The missing image event originates on the render thread but needs to be called on the ui thread. The observer for RendererFrontend
needs to be implemented in a way to calls onto the main thread and then calls the callback back on the render thread. I haven't implemented the changes to the Android platform yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, I think you're right. This isn't necessary here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I think the RendererFrontend
's observer needs to have this callback and since Map::Impl
implements the same interface it also needs it
}; | ||
|
||
ImageAtlas makeImageAtlas(const ImageMap&, const ImageMap&); | ||
ImageAtlas makeImageAtlas(const ImageMap&, const ImageMap&, const std::unordered_map<std::string, uint32_t>& versionMap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we're accumulating more objects that work together to form an ImageAtlas, maybe it's time to bundle them up together in a follow up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few non-blocking nits
…ed images (#14253) Also make `Style#updateImage(...)` much faster when the image doesn't change size. This can be useful for asynchronously generating images.
…ed images (#14253) Also make `Style#updateImage(...)` much faster when the image doesn't change size. This can be useful for asynchronously generating images.
ports mapbox/mapbox-gl-js#7987
This adds a
onStyleImageMissing
method toMapObserver
that gets called with the id of each missing icon. Missing images can be provided by callingStyle#addImage
from within the listener. If an image is not added it is skipped and tile parsing continues.how the listener is called:
ImageManager
callsRenderer::Impl#onStyleImageMissing
with id and callbackRenderer::Impl#onStyleImageMissing
calls the observer set byRendererFrontend
with id and callbackRendererFrontend
calls theMap::Impl#onStyleImageMissing
on the map thread with id and callbackMap::Impl#onStyleImageMissing
callsMapObserver#onStyleImageMissing
with just the id and then calls the callback after the listener has been calledRendererFrontend
is responsible for invoking the callback on the render thread.TODO