[core] Replace inline SpriteAtlas updates with diffing #9010
Conversation
include/mbgl/util/immutable.hpp
Outdated
| }; | ||
|
|
||
| template <class S, class U> | ||
| Immutable<S> staticImmutableCast(const Immutable<U>& u) { | ||
| return Immutable<S>(std::static_pointer_cast<const S>(u.ptr)); | ||
| } | ||
|
|
||
| template <class S, class U> | ||
| Immutable<S> dynamicImmutableCast(const Immutable<U>& u) { | ||
| (void)dynamic_cast<const S&>(*u); // Call for side effect of failure. |
There was a problem hiding this comment.
When the dynamic_cast fails, it'll throw a std::bad_cast exception, since the type is a reference type (rather than a pointer type). Can you please clarify this in the comment?
| impl_->visibility != impl().visibility || | ||
| impl_->layout != impl().layout || | ||
| impl_->paint.hasDataDrivenPropertyDifference(impl().paint)); | ||
| baseImpl = impl_; |
There was a problem hiding this comment.
Some implementations use std::swap and some use assignment. Is there a way to unify that?
| @@ -33,6 +33,17 @@ std::unique_ptr<SymbolLayout> RenderSymbolLayer::createLayout(const BucketParame | |||
| glyphDependencies); | |||
| } | |||
|
|
|||
| bool RenderSymbolLayer::updateImpl(Immutable<style::Layer::Impl> baseImpl_) { | |||
| auto impl_ = dynamicImmutableCast<style::SymbolLayer::Impl>(baseImpl_); | |||
There was a problem hiding this comment.
Instead of introducing yet another cast, can we do auto& impl_ = dynamic_cast<const style::SymbolLayer::Impl&>(*baseImpl_);, and just call std::swap(baseImpl_, baseImpl); below?
src/mbgl/style/style.cpp
Outdated
|
|
||
| // Create render images for newly added images. | ||
| for (const auto& entry : imageDiff.added) { | ||
| spriteAtlas->addImage(entry.second); |
There was a problem hiding this comment.
Yes, of the Immutable reference.
jfirebaugh
force-pushed
the
image-id
branch
2 times, most recently
from
71cc762
to
4aa786d
Compare
jfirebaugh
force-pushed
the
render-sprite-atlas
branch
2 times, most recently
from
957e6e0
to
f3dbb82
Compare
jfirebaugh
force-pushed
the
render-sprite-atlas
branch
4 times, most recently
from
ad1d78c
to
b2886db
Compare
jfirebaugh
force-pushed
the
render-sprite-atlas
branch
6 times, most recently
from
4faedf6
to
ac2f1f3
Compare
|
This is ready for re-review. |
| ImageDifference diffImages(const std::vector<ImmutableImage>& a, | ||
| const std::vector<ImmutableImage>& b) { | ||
| return diff(a, b, [] (const ImmutableImage& lhs, const ImmutableImage& rhs) { | ||
| return lhs->id == rhs->id; |
There was a problem hiding this comment.
This breaks down when two styles have sprites with image names that are identical, but actually refer to different images.
There was a problem hiding this comment.
In that case, the image will be added to the "changed" list, and subsequently passed to SpriteAtlas::addImage.
- If the image is the same size, this will work.
- If the image is a different size, this will fail an assertion.
#7618 is the underlying issue that should be fixed; I'm planning on working on that this week. In the meantime, I'll change the assertion to a regular conditional, mirroring this check.
There was a problem hiding this comment.
Cleaned this up in b2e7794 but left the assertion; I don't believe there's currently a way to trigger it.
| return diff(a, b); | ||
| return diff(a, b, [] (const ImmutableSource& lhs, const ImmutableSource& rhs) { | ||
| return std::tie(lhs->id, lhs->type) | ||
| == std::tie(rhs->id, rhs->type); |
There was a problem hiding this comment.
Similarly, this breaks down when id/type match, but different styles use the same source name (composite is the leading contender) to point to different underlying tilesets.
There was a problem hiding this comment.
Yes, what needs to happen in this situation is that the source reloads its existing tiles and clears the cache. Refactoring Source::{reload,updateTiles,invalidate,remove}Tiles is also on my list this week, and I think that will be the best way to address this. Currently I don't think there's a way to trigger this -- we don't expose an API for changing a source's tileset, and we don't (yet) use diffing to morph between two completely separate styles. (Same for the image issue in fact.)
There was a problem hiding this comment.
and we don't (yet) use diffing to morph between two completely separate styles.
Ah, I thought we already did that, hence the comment.
There was a problem hiding this comment.
Update: after thinking about how to handle this case once we do support smart set style, I decided I want to rework the "Eliminate Style::updateBatch in favor of diffing" part of this PR significantly. I'll land just the SpriteAtlas portion for now.
jfirebaugh
force-pushed
the
style-diff
branch
2 times, most recently
from
9d94da0
to
a8900b7
Compare
jfirebaugh
changed the title
Use style diffing to resolve updates to images and updates that require reloading tiles.