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

[core] Replace inline SpriteAtlas updates with diffing #9010

Merged
merged 1 commit into from
May 30, 2017
Merged

[core] Replace inline SpriteAtlas updates with diffing #9010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions src/mbgl/renderer/style_diff.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,10 @@

namespace mbgl {

template <class T>
StyleDifference<T> diff(const std::vector<T>& a, const std::vector<T>& b) {
template <class T, class Eq>
StyleDifference<T> diff(const std::vector<T>& a, const std::vector<T>& b, const Eq& eq) {
std::vector<T> lcs;

auto eq = [] (const T& lhs, const T& rhs) {
return std::tie(lhs->id, lhs->type)
== std::tie(rhs->id, rhs->type);
};

longest_common_subsequence(a.begin(), a.end(), b.begin(), b.end(), std::back_inserter(lcs), eq);

auto aIt = a.begin();
Expand Down Expand Up @@ -43,14 +38,27 @@ StyleDifference<T> diff(const std::vector<T>& a, const std::vector<T>& b) {
return result;
}

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks down when two styles have sprites with image names that are identical, but actually refer to different images.

Copy link
Contributor Author

@jfirebaugh jfirebaugh May 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jfirebaugh jfirebaugh May 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned this up in b2e7794 but left the assertion; I don't believe there's currently a way to trigger it.

});
}

SourceDifference diffSources(const std::vector<ImmutableSource>& a,
const std::vector<ImmutableSource>& b) {
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and we don't (yet) use diffing to morph between two completely separate styles.

Ah, I thought we already did that, hence the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
}

LayerDifference diffLayers(const std::vector<ImmutableLayer>& a,
const std::vector<ImmutableLayer>& b) {
return diff(a, b);
return diff(a, b, [] (const ImmutableLayer& lhs, const ImmutableLayer& rhs) {
return std::tie(lhs->id, lhs->type)
== std::tie(rhs->id, rhs->type);
});
}

} // namespace mbgl
7 changes: 7 additions & 0 deletions src/mbgl/renderer/style_diff.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include <mbgl/style/image_impl.hpp>
#include <mbgl/style/source_impl.hpp>
#include <mbgl/style/layer_impl.hpp>
#include <mbgl/util/immutable.hpp>
Expand All @@ -15,6 +16,12 @@ class StyleDifference {
std::unordered_map<std::string, T> changed;
};

using ImmutableImage = Immutable<style::Image::Impl>;
using ImageDifference = StyleDifference<ImmutableImage>;

ImageDifference diffImages(const std::vector<ImmutableImage>&,
const std::vector<ImmutableImage>&);

using ImmutableSource = Immutable<style::Source::Impl>;
using SourceDifference = StyleDifference<ImmutableSource>;

Expand Down
28 changes: 13 additions & 15 deletions src/mbgl/sprite/sprite_atlas.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,17 +52,16 @@ void SpriteAtlas::onSpriteLoaded() {
}

void SpriteAtlas::addImage(Immutable<style::Image::Impl> image_) {
assert(entries.find(image_->id) == entries.end());
entries.emplace(image_->id, Entry { image_ });
icons.clear();
}

auto it = entries.find(image_->id);
if (it == entries.end()) {
entries.emplace(image_->id, Entry { image_, {}, {} });
return;
}
void SpriteAtlas::updateImage(Immutable<style::Image::Impl> image_) {
assert(entries.find(image_->id) != entries.end());
Entry& entry = entries.at(image_->id);

Entry& entry = it->second;

// There is already a sprite with that name in our store.
// Style::addImage should prevent changing size.
assert(entry.image->image.size == image_->image.size);

entry.image = std::move(image_);
Expand All @@ -74,15 +73,13 @@ void SpriteAtlas::addImage(Immutable<style::Image::Impl> image_) {
if (entry.patternBin) {
copy(entry, &Entry::patternBin);
}
}

void SpriteAtlas::removeImage(const std::string& id) {
icons.clear();
}

auto it = entries.find(id);
assert(it != entries.end());

Entry& entry = it->second;
void SpriteAtlas::removeImage(const std::string& id) {
assert(entries.find(id) != entries.end());
Entry& entry = entries.at(id);

if (entry.iconBin) {
shelfPack.unref(*entry.iconBin);
Expand All @@ -92,7 +89,8 @@ void SpriteAtlas::removeImage(const std::string& id) {
shelfPack.unref(*entry.patternBin);
}

entries.erase(it);
entries.erase(id);
icons.clear();
}

const style::Image::Impl* SpriteAtlas::getImage(const std::string& id) const {
Expand Down
2 changes: 2 additions & 0 deletions src/mbgl/sprite/sprite_atlas.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ class SpriteAtlas : public util::noncopyable {
void dumpDebugLogs() const;

const style::Image::Impl* getImage(const std::string&) const;

void addImage(Immutable<style::Image::Impl>);
void updateImage(Immutable<style::Image::Impl>);
void removeImage(const std::string&);

void getIcons(IconRequestor&, IconDependencies);
Expand Down
41 changes: 34 additions & 7 deletions src/mbgl/style/style.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,35 @@ void Style::update(const UpdateParameters& parameters) {
}


std::vector<Immutable<Image::Impl>> newImageImpls;
newImageImpls.reserve(images.size());
for (const auto& entry : images) {
newImageImpls.push_back(entry.second->impl);
}

const ImageDifference imageDiff = diffImages(imageImpls, newImageImpls);
imageImpls = std::move(newImageImpls);

// Remove removed images from sprite atlas.
for (const auto& entry : imageDiff.removed) {
spriteAtlas->removeImage(entry.first);
}

// Add added images to sprite atlas.
for (const auto& entry : imageDiff.added) {
spriteAtlas->addImage(entry.second);
}

// Update changed images.
for (const auto& entry : imageDiff.changed) {
spriteAtlas->updateImage(entry.second);
}

if (spriteLoaded && !spriteAtlas->isLoaded()) {
spriteAtlas->onSpriteLoaded();
}


std::vector<Immutable<Source::Impl>> newSourceImpls;
newSourceImpls.reserve(sources.size());
for (const auto& source : sources) {
Expand Down Expand Up @@ -509,6 +538,10 @@ bool Style::isLoaded() const {
return false;
}

if (!spriteLoaded) {
return false;
}

for (const auto& source: sources) {
if (!source->loaded) {
return false;
Expand All @@ -521,10 +554,6 @@ bool Style::isLoaded() const {
}
}

if (!spriteAtlas->isLoaded()) {
return false;
}

return true;
}

Expand All @@ -535,13 +564,11 @@ void Style::addImage(std::unique_ptr<style::Image> image) {
Log::Warning(Event::Sprite, "Can't change sprite dimensions for '%s'", id.c_str());
return;
}
spriteAtlas->addImage(image->impl);
images[id] = std::move(image);
}

void Style::removeImage(const std::string& id) {
images.erase(id);
spriteAtlas->removeImage(id);
}

const style::Image* Style::getImage(const std::string& id) const {
Expand Down Expand Up @@ -764,7 +791,7 @@ void Style::onSpriteLoaded(std::vector<std::unique_ptr<Image>>&& images_) {
for (auto& image : images_) {
addImage(std::move(image));
}
spriteAtlas->onSpriteLoaded();
spriteLoaded = true;
observer->onUpdate(Update::Repaint); // For *-pattern properties.
}

Expand Down
3 changes: 3 additions & 0 deletions src/mbgl/style/style.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <mbgl/style/layer_observer.hpp>
#include <mbgl/style/light_observer.hpp>
#include <mbgl/style/update_batch.hpp>
#include <mbgl/style/image.hpp>
#include <mbgl/renderer/render_source.hpp>
#include <mbgl/renderer/render_source_observer.hpp>
#include <mbgl/renderer/render_layer.hpp>
Expand Down Expand Up @@ -138,6 +139,7 @@ class Style : public GlyphAtlasObserver,
double defaultBearing = 0;
double defaultPitch = 0;

std::vector<Immutable<Image::Impl>> imageImpls;
std::vector<Immutable<Source::Impl>> sourceImpls;
std::vector<Immutable<Layer::Impl>> layerImpls;

Expand Down Expand Up @@ -179,6 +181,7 @@ class Style : public GlyphAtlasObserver,

UpdateBatch updateBatch;
ZoomHistory zoomHistory;
bool spriteLoaded = false;

public:
bool loaded = false;
Expand Down
2 changes: 1 addition & 1 deletion test/sprite/sprite_atlas.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ TEST(SpriteAtlas, Updates) {
for (size_t i = 0; i < image2.bytes(); i++) {
image2.data.get()[i] = 255;
}
atlas.addImage(makeMutable<style::Image::Impl>("one", std::move(image2), 1));
atlas.updateImage(makeMutable<style::Image::Impl>("one", std::move(image2), 1));

test::checkImage("test/fixtures/sprite_atlas/updates_after", atlas.getAtlasImage());
}
Expand Down