From 6efbca618c14598c86e2a0571a1b24537e4504ca Mon Sep 17 00:00:00 2001 From: Chris Loer Date: Tue, 21 Mar 2017 15:35:48 -0700 Subject: [PATCH] [core] De-mutex SpriteAtlas. - Consolidate symbol dependency tracking code. --- src/mbgl/layout/symbol_layout.cpp | 53 +++----- src/mbgl/layout/symbol_layout.hpp | 14 +-- src/mbgl/sprite/sprite_atlas.cpp | 42 +++++-- src/mbgl/sprite/sprite_atlas.hpp | 31 ++++- src/mbgl/style/layers/symbol_layer_impl.cpp | 6 +- src/mbgl/style/layers/symbol_layer_impl.hpp | 4 +- src/mbgl/style/source_impl.cpp | 6 - src/mbgl/style/source_impl.hpp | 4 - src/mbgl/style/style.cpp | 8 -- src/mbgl/style/style.hpp | 1 - src/mbgl/text/shaping.cpp | 8 +- src/mbgl/tile/geometry_tile.cpp | 26 +++- src/mbgl/tile/geometry_tile.hpp | 12 +- src/mbgl/tile/geometry_tile_worker.cpp | 128 +++++++++++--------- src/mbgl/tile/geometry_tile_worker.hpp | 8 +- src/mbgl/tile/tile.hpp | 1 - test/sprite/sprite_atlas.test.cpp | 34 +++--- 17 files changed, 222 insertions(+), 164 deletions(-) diff --git a/src/mbgl/layout/symbol_layout.cpp b/src/mbgl/layout/symbol_layout.cpp index 513a5e071e4..3d50e912851 100644 --- a/src/mbgl/layout/symbol_layout.cpp +++ b/src/mbgl/layout/symbol_layout.cpp @@ -32,14 +32,15 @@ using namespace style; SymbolLayout::SymbolLayout(const BucketParameters& parameters, const std::vector& layers, const GeometryTileLayer& sourceLayer, - SpriteAtlas& spriteAtlas_, + IconDependencies& iconDependencies, + uintptr_t _spriteAtlasMapIndex, GlyphDependencies& glyphDependencies) : sourceLayerName(sourceLayer.getName()), bucketName(layers.at(0)->getID()), overscaling(parameters.tileID.overscaleFactor()), zoom(parameters.tileID.overscaledZ), mode(parameters.mode), - spriteAtlas(spriteAtlas_), + spriteAtlasMapIndex(_spriteAtlasMapIndex), tileSize(util::tileSize * overscaling), tilePixelRatio(float(util::EXTENT) / tileSize) { @@ -148,6 +149,7 @@ SymbolLayout::SymbolLayout(const BucketParameters& parameters, if (hasIcon) { ft.icon = util::replaceTokens(layout.get(), getValue); + iconDependencies.insert(*ft.icon); } if (ft.text || ft.icon) { @@ -164,24 +166,7 @@ bool SymbolLayout::hasSymbolInstances() const { return !symbolInstances.empty(); } -bool SymbolLayout::canPrepare(const GlyphPositionMap& glyphPositions) { - // TODO: This is a needlessly complex way to check if we can move to the next step, we really just want to wait until - // we've gotten a reply from 'getGlyphs'. I'm just keeping this in place here to reduce the number of moving parts in the refactor - const bool hasTextField = layout.get().match([&] (const std::string& s) { return !s.empty(); }, - [&] (const auto&) { return true; } ); - - if (hasTextField && glyphPositions.empty()) { - return false; - } - - if (!layout.get().empty() && !spriteAtlas.isLoaded()) { - return false; - } - - return true; -} - -void SymbolLayout::prepare(const GlyphPositionMap& glyphs) { +void SymbolLayout::prepare(const GlyphPositionMap& glyphs, const IconAtlasMap& iconMap) { float horizontalAlign = 0.5; float verticalAlign = 0.5; @@ -269,19 +254,21 @@ void SymbolLayout::prepare(const GlyphPositionMap& glyphs) { // if feature has icon, get sprite atlas position if (feature.icon) { - auto image = spriteAtlas.getIcon(*feature.icon); - if (image) { - shapedIcon = shapeIcon(*image, - layout.evaluate(zoom, feature), - layout.evaluate(zoom, feature) * util::DEG2RAD); - assert((*image).spriteImage); - if ((*image).spriteImage->sdf) { - sdfIcons = true; - } - if ((*image).relativePixelRatio != 1.0f) { - iconsNeedLinear = true; - } else if (layout.get().constantOr(1) != 0) { - iconsNeedLinear = true; + auto icons = iconMap.find(spriteAtlasMapIndex); + if (icons != iconMap.end()) { + auto image = icons->second.find(*feature.icon); + if (image != icons->second.end()) { + shapedIcon = shapeIcon(image->second, + layout.evaluate(zoom, feature), + layout.evaluate(zoom, feature) * util::DEG2RAD); + if (image->second.sdf) { + sdfIcons = true; + } + if (image->second.relativePixelRatio != 1.0f) { + iconsNeedLinear = true; + } else if (layout.get().constantOr(1) != 0) { + iconsNeedLinear = true; + } } } } diff --git a/src/mbgl/layout/symbol_layout.hpp b/src/mbgl/layout/symbol_layout.hpp index fe5be1e4a9f..f6062ae6831 100644 --- a/src/mbgl/layout/symbol_layout.hpp +++ b/src/mbgl/layout/symbol_layout.hpp @@ -16,8 +16,6 @@ namespace mbgl { class GeometryTileLayer; class CollisionTile; -class SpriteAtlas; -class GlyphAtlas; class SymbolBucket; class Anchor; @@ -32,12 +30,11 @@ class SymbolLayout { SymbolLayout(const style::BucketParameters&, const std::vector&, const GeometryTileLayer&, - SpriteAtlas&, + IconDependencies&, + uintptr_t, GlyphDependencies&); - bool canPrepare(const GlyphPositionMap& glyphs); - - void prepare(const GlyphPositionMap& glyphs); + void prepare(const GlyphPositionMap& glyphs, const IconAtlasMap& icons); std::unique_ptr place(CollisionTile&); @@ -45,7 +42,6 @@ class SymbolLayout { enum State { Pending, // Waiting for the necessary glyphs or icons to be available. - Prepared, // The potential positions of text and icons have been determined. Placed // The final positions have been determined, taking into account prior layers. }; @@ -80,8 +76,8 @@ class SymbolLayout { style::SymbolLayoutProperties::Evaluated layout; float textMaxSize; - - SpriteAtlas& spriteAtlas; + + uintptr_t spriteAtlasMapIndex; // Actually a pointer to the SpriteAtlas for this symbol's layer, but don't use it from worker threads! const uint32_t tileSize; const float tilePixelRatio; diff --git a/src/mbgl/sprite/sprite_atlas.cpp b/src/mbgl/sprite/sprite_atlas.cpp index 2bf8a3095a6..b19eaa572b0 100644 --- a/src/mbgl/sprite/sprite_atlas.cpp +++ b/src/mbgl/sprite/sprite_atlas.cpp @@ -28,11 +28,13 @@ struct SpriteAtlas::Loader { }; SpriteAtlasElement::SpriteAtlasElement(Rect rect_, - std::shared_ptr image_, + std::shared_ptr spriteImage, Size size_, float pixelRatio) : pos(std::move(rect_)), - spriteImage(std::move(image_)), - relativePixelRatio(spriteImage->pixelRatio / pixelRatio) { + sdf(spriteImage->sdf), + relativePixelRatio(spriteImage->pixelRatio / pixelRatio), + width(spriteImage->getWidth()), + height(spriteImage->getHeight()) { const float padding = 1; @@ -105,6 +107,10 @@ void SpriteAtlas::emitSpriteLoadedIfComplete() { loaded = true; setSprites(result.get()); observer->onSpriteLoaded(); + for (auto requestor : requestors) { + requestor->onIconsAvailable(this, buildIconMap()); + } + requestors.clear(); } else { observer->onSpriteError(result.get()); } @@ -119,20 +125,16 @@ void SpriteAtlas::dumpDebugLogs() const { } void SpriteAtlas::setSprites(const Sprites& newSprites) { - std::lock_guard lock(mutex); for (const auto& pair : newSprites) { _setSprite(pair.first, pair.second); } } void SpriteAtlas::setSprite(const std::string& name, std::shared_ptr sprite) { - std::lock_guard lock(mutex); _setSprite(name, sprite); } void SpriteAtlas::removeSprite(const std::string& name) { - std::lock_guard lock(mutex); - auto it = entries.find(name); if (it == entries.end()) { return; @@ -184,7 +186,6 @@ void SpriteAtlas::_setSprite(const std::string& name, } std::shared_ptr SpriteAtlas::getSprite(const std::string& name) { - std::lock_guard lock(mutex); const auto it = entries.find(name); if (it != entries.end()) { return it->second.spriteImage; @@ -196,6 +197,18 @@ std::shared_ptr SpriteAtlas::getSprite(const std::string& nam } } +void SpriteAtlas::getIcons(IconRequestor& requestor) { + if (isLoaded()) { + requestor.onIconsAvailable(this, buildIconMap()); + } else { + requestors.insert(&requestor); + } +} + +void SpriteAtlas::removeRequestor(IconRequestor& requestor) { + requestors.erase(&requestor); +} + optional SpriteAtlas::getIcon(const std::string& name) { return getImage(name, &Entry::iconRect); } @@ -206,7 +219,6 @@ optional SpriteAtlas::getPattern(const std::string& name) { optional SpriteAtlas::getImage(const std::string& name, optional> Entry::*entryRect) { - std::lock_guard lock(mutex); auto it = entries.find(name); if (it == entries.end()) { @@ -219,6 +231,7 @@ optional SpriteAtlas::getImage(const std::string& name, Entry& entry = it->second; if (entry.*entryRect) { + assert(entry.spriteImage.get()); return SpriteAtlasElement { *(entry.*entryRect), entry.spriteImage, @@ -285,6 +298,17 @@ void SpriteAtlas::copy(const Entry& entry, optional> Entry::*entr dirty = true; } +IconMap SpriteAtlas::buildIconMap() { + IconMap icons; + for (auto entry : entries) { + icons.emplace(std::piecewise_construct, + std::forward_as_tuple(entry.first), + std::forward_as_tuple(*getIcon(entry.first))); + + } + return icons; +} + void SpriteAtlas::upload(gl::Context& context, gl::TextureUnit unit) { if (!texture) { texture = context.createTexture(image, unit); diff --git a/src/mbgl/sprite/sprite_atlas.hpp b/src/mbgl/sprite/sprite_atlas.hpp index c7b266376be..788c2589a61 100644 --- a/src/mbgl/sprite/sprite_atlas.hpp +++ b/src/mbgl/sprite/sprite_atlas.hpp @@ -6,10 +6,9 @@ #include #include -#include #include #include -#include +#include #include #include #include @@ -28,12 +27,26 @@ class SpriteAtlasElement { SpriteAtlasElement(Rect, std::shared_ptr, Size size, float pixelRatio); Rect pos; - std::shared_ptr spriteImage; + bool sdf; float relativePixelRatio; std::array size; std::array tl; std::array br; + float width; + float height; +}; + +class SpriteAtlas; + +typedef std::map IconMap; +typedef std::set IconDependencies; +typedef std::map IconAtlasMap; +typedef std::map IconDependencyMap; + +class IconRequestor { +public: + virtual void onIconsAvailable(SpriteAtlas*, IconMap) = 0; }; class SpriteAtlas : public util::noncopyable { @@ -55,8 +68,11 @@ class SpriteAtlas : public util::noncopyable { void setSprite(const std::string&, std::shared_ptr); void removeSprite(const std::string&); + + std::shared_ptr getSprite(const std::string& name); - std::shared_ptr getSprite(const std::string&); + void getIcons(IconRequestor& requestor); + void removeRequestor(IconRequestor& requestor); optional getIcon(const std::string& name); optional getPattern(const std::string& name); @@ -105,13 +121,16 @@ class SpriteAtlas : public util::noncopyable { optional getImage(const std::string& name, optional> Entry::*rect); void copy(const Entry&, optional> Entry::*rect); + + IconMap buildIconMap(); - std::mutex mutex; std::unordered_map entries; BinPack bin; PremultipliedImage image; mbgl::optional texture; - std::atomic dirty; + bool dirty; + + std::set requestors; }; } // namespace mbgl diff --git a/src/mbgl/style/layers/symbol_layer_impl.cpp b/src/mbgl/style/layers/symbol_layer_impl.cpp index 47d802a1184..48c2eeacaf5 100644 --- a/src/mbgl/style/layers/symbol_layer_impl.cpp +++ b/src/mbgl/style/layers/symbol_layer_impl.cpp @@ -37,11 +37,13 @@ std::unique_ptr SymbolLayer::Impl::createBucket(const BucketParameters&, std::unique_ptr SymbolLayer::Impl::createLayout(const BucketParameters& parameters, const std::vector& group, const GeometryTileLayer& layer, - GlyphDependencies& glyphDependencies) const { + GlyphDependencies& glyphDependencies, + IconDependencyMap& iconDependencyMap) const { return std::make_unique(parameters, group, layer, - *spriteAtlas, + iconDependencyMap[spriteAtlas], + (uintptr_t)spriteAtlas, glyphDependencies); } diff --git a/src/mbgl/style/layers/symbol_layer_impl.hpp b/src/mbgl/style/layers/symbol_layer_impl.hpp index 16bd67a4d44..f0ef94b3804 100644 --- a/src/mbgl/style/layers/symbol_layer_impl.hpp +++ b/src/mbgl/style/layers/symbol_layer_impl.hpp @@ -2,13 +2,13 @@ #include #include +#include #include #include #include namespace mbgl { -class SpriteAtlas; class SymbolLayout; namespace style { @@ -68,7 +68,7 @@ class SymbolLayer::Impl : public Layer::Impl { std::unique_ptr createBucket(const BucketParameters&, const std::vector&) const override; std::unique_ptr createLayout(const BucketParameters&, const std::vector&, - const GeometryTileLayer&, GlyphDependencies&) const; + const GeometryTileLayer&, GlyphDependencies&, IconDependencyMap&) const; IconPaintProperties::Evaluated iconPaintProperties() const; TextPaintProperties::Evaluated textPaintProperties() const; diff --git a/src/mbgl/style/source_impl.cpp b/src/mbgl/style/source_impl.cpp index 356d420e199..9fabc54f7d8 100644 --- a/src/mbgl/style/source_impl.cpp +++ b/src/mbgl/style/source_impl.cpp @@ -191,12 +191,6 @@ void Source::Impl::removeTiles() { } } -void Source::Impl::updateSymbolDependentTiles() { - for (auto& pair : tiles) { - pair.second->symbolDependenciesChanged(); - } -} - void Source::Impl::reloadTiles() { cache.clear(); diff --git a/src/mbgl/style/source_impl.hpp b/src/mbgl/style/source_impl.hpp index b4941dc638e..e8ba54b2b6b 100644 --- a/src/mbgl/style/source_impl.hpp +++ b/src/mbgl/style/source_impl.hpp @@ -48,10 +48,6 @@ class Source::Impl : public TileObserver, private util::noncopyable { // trigger re-placement of existing complete tiles. void updateTiles(const UpdateParameters&); - // Called when icons or glyphs are loaded. Triggers further processing of tiles which - // were waiting on such dependencies. - void updateSymbolDependentTiles(); - // Removes all tiles (by putting them into the cache). void removeTiles(); diff --git a/src/mbgl/style/style.cpp b/src/mbgl/style/style.cpp index 5f9983ae941..9640988402f 100644 --- a/src/mbgl/style/style.cpp +++ b/src/mbgl/style/style.cpp @@ -274,12 +274,6 @@ void Style::updateTiles(const UpdateParameters& parameters) { } } -void Style::updateSymbolDependentTiles() { - for (const auto& source : sources) { - source->baseImpl->updateSymbolDependentTiles(); - } -} - void Style::relayout() { for (const auto& sourceID : updateBatch.sourceIDs) { Source* source = getSource(sourceID); @@ -575,7 +569,6 @@ void Style::setObserver(style::Observer* observer_) { void Style::onGlyphsLoaded(const FontStack& fontStack, const GlyphRange& glyphRange) { observer->onGlyphsLoaded(fontStack, glyphRange); - updateSymbolDependentTiles(); } void Style::onGlyphsError(const FontStack& fontStack, const GlyphRange& glyphRange, std::exception_ptr error) { @@ -626,7 +619,6 @@ void Style::onTileError(Source& source, const OverscaledTileID& tileID, std::exc void Style::onSpriteLoaded() { observer->onSpriteLoaded(); observer->onUpdate(Update::Repaint); // For *-pattern properties. - updateSymbolDependentTiles(); } void Style::onSpriteError(std::exception_ptr error) { diff --git a/src/mbgl/style/style.hpp b/src/mbgl/style/style.hpp index 0c65129422e..817cab5fd58 100644 --- a/src/mbgl/style/style.hpp +++ b/src/mbgl/style/style.hpp @@ -128,7 +128,6 @@ class Style : public GlyphAtlasObserver, std::vector>::const_iterator findLayer(const std::string& layerID) const; void reloadLayerSource(Layer&); - void updateSymbolDependentTiles(); // GlyphStoreObserver implementation. void onGlyphsLoaded(const FontStack&, const GlyphRange&) override; diff --git a/src/mbgl/text/shaping.cpp b/src/mbgl/text/shaping.cpp index 7285d9c8122..b53cba2e508 100644 --- a/src/mbgl/text/shaping.cpp +++ b/src/mbgl/text/shaping.cpp @@ -13,10 +13,10 @@ namespace mbgl { PositionedIcon shapeIcon(const SpriteAtlasElement& image, const std::array& iconOffset, const float iconRotation) { float dx = iconOffset[0]; float dy = iconOffset[1]; - float x1 = dx - image.spriteImage->getWidth() / 2.0f; - float x2 = x1 + image.spriteImage->getWidth(); - float y1 = dy - image.spriteImage->getHeight() / 2.0f; - float y2 = y1 + image.spriteImage->getHeight(); + float x1 = dx - image.width/ 2.0f; + float x2 = x1 + image.width; + float y1 = dy - image.height / 2.0f; + float y2 = y1 + image.height; return PositionedIcon(image, y1, y2, x1, x2, iconRotation); } diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index 602c43148aa..bba2999ebf8 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -18,6 +18,8 @@ #include #include +#include + namespace mbgl { using namespace style; @@ -39,6 +41,9 @@ GeometryTile::GeometryTile(const OverscaledTileID& id_, GeometryTile::~GeometryTile() { glyphAtlas.removeGlyphs(*this); + for (auto spriteAtlas : pendingSpriteAtlases) { + spriteAtlas->removeRequestor(*this); + } cancel(); } @@ -78,10 +83,6 @@ void GeometryTile::setPlacementConfig(const PlacementConfig& desiredConfig) { worker.invoke(&GeometryTileWorker::setPlacementConfig, desiredConfig, correlationID); } -void GeometryTile::symbolDependenciesChanged() { - worker.invoke(&GeometryTileWorker::symbolDependenciesChanged); -} - void GeometryTile::redoLayout() { // Mark the tile as pending again if it was complete before to prevent signaling a complete // state despite pending parse operations. @@ -140,6 +141,23 @@ void GeometryTile::getGlyphs(GlyphDependencies glyphDependencies) { glyphAtlas.getGlyphs(*this, std::move(glyphDependencies)); } +void GeometryTile::onIconsAvailable(SpriteAtlas* spriteAtlas, IconMap icons) { + iconAtlasMap[(uintptr_t)spriteAtlas] = icons; + pendingSpriteAtlases.erase(spriteAtlas); + if (pendingSpriteAtlases.empty()) { + worker.invoke(&GeometryTileWorker::onIconsAvailable, std::move(iconAtlasMap)); + } +} + +// TODO: If there's any value to be gained by it, we can narrow our request to just the sprites +// we need, but SpriteAtlases are just "loaded" or "not loaded" +void GeometryTile::getIcons(IconDependencyMap iconDependencyMap) { + for (auto dependency : iconDependencyMap) { + pendingSpriteAtlases.insert(dependency.first); + dependency.first->getIcons(*this); + } +} + Bucket* GeometryTile::getBucket(const Layer& layer) { const auto& buckets = layer.is() ? symbolBuckets : nonSymbolBuckets; const auto it = buckets.find(layer.baseImpl->id); diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index c7b0264c5eb..0531917acc4 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -25,7 +26,7 @@ class UpdateParameters; class SourceQueryOptions; } // namespace style -class GeometryTile : public Tile, public GlyphRequestor { +class GeometryTile : public Tile, public GlyphRequestor, IconRequestor { public: GeometryTile(const OverscaledTileID&, std::string sourceID, @@ -37,10 +38,13 @@ class GeometryTile : public Tile, public GlyphRequestor { void setData(std::unique_ptr); void setPlacementConfig(const PlacementConfig&) override; - void symbolDependenciesChanged() override; void redoLayout() override; + void onGlyphsAvailable(GlyphPositionMap) override; + void onIconsAvailable(SpriteAtlas*, IconMap) override; + void getGlyphs(GlyphDependencies); + void getIcons(IconDependencyMap); Bucket* getBucket(const style::Layer&) override; @@ -74,8 +78,6 @@ class GeometryTile : public Tile, public GlyphRequestor { void onPlacement(PlacementResult); void onError(std::exception_ptr); - - virtual void onGlyphsAvailable(GlyphPositionMap) override; protected: const GeometryTileData* getData() { @@ -93,6 +95,8 @@ class GeometryTile : public Tile, public GlyphRequestor { Actor worker; GlyphAtlas& glyphAtlas; + std::set pendingSpriteAtlases; + IconAtlasMap iconAtlasMap; uint64_t correlationID = 0; optional requestedConfig; diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp index 0d17526cb14..76214985061 100644 --- a/src/mbgl/tile/geometry_tile_worker.cpp +++ b/src/mbgl/tile/geometry_tile_worker.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -30,7 +31,9 @@ GeometryTileWorker::GeometryTileWorker(ActorRef self_, parent(std::move(parent_)), id(std::move(id_)), obsolete(obsolete_), - mode(mode_) { + mode(mode_), + waitingForGlyphs(false), + waitingForIcons(false) { } GeometryTileWorker::~GeometryTileWorker() { @@ -43,7 +46,7 @@ GeometryTileWorker::~GeometryTileWorker() { [idle] <----------------------------. | | - set{Data,Layers,Placement}, symbolDependenciesChanged | + set{Data,Layers,Placement} | | | (do layout/placement; self-send "coalesced") | v | @@ -67,6 +70,9 @@ GeometryTileWorker::~GeometryTileWorker() { read all the queued messages until we get to "coalesced", and then redo either layout or placement if there were one or more "set"s (with layout taking priority, since it will trigger placement when complete), or return to the [idle] state if not. + + TODO: The transition diagram is complicated by handling symbol dependencies + asynchronously (insert link to updated diagram or ASCII-fy it) */ void GeometryTileWorker::setData(std::unique_ptr data_, uint64_t correlationID_) { @@ -77,14 +83,19 @@ void GeometryTileWorker::setData(std::unique_ptr data_, switch (state) { case Idle: redoLayout(); - coalesce(); + if (!hasPendingSymbolDependencies()) { + coalesce(); + } else { + state = NeedPlacement; + } break; case Coalescing: - case NeedLayout: case NeedPlacement: state = NeedLayout; break; + case NeedLayout: + break; } } catch (...) { parent.invoke(&GeometryTile::onError, std::current_exception()); @@ -99,7 +110,11 @@ void GeometryTileWorker::setLayers(std::vector> layers_, switch (state) { case Idle: redoLayout(); - coalesce(); + if (!hasPendingSymbolDependencies()) { + coalesce(); + } else { + state = NeedPlacement; + } break; case Coalescing: @@ -139,24 +154,46 @@ void GeometryTileWorker::setPlacementConfig(PlacementConfig placementConfig_, ui } } +void GeometryTileWorker::onGlyphsAvailable(GlyphPositionMap glyphs) { + glyphPositions = std::move(glyphs); + waitingForGlyphs = false; + symbolDependenciesChanged(); +} + +void GeometryTileWorker::onIconsAvailable(IconAtlasMap icons_) { + icons = std::move(icons_); + waitingForIcons = false; + symbolDependenciesChanged(); +} + +bool GeometryTileWorker::hasPendingSymbolDependencies() const { + return waitingForGlyphs || waitingForIcons; +} + void GeometryTileWorker::symbolDependenciesChanged() { try { switch (state) { - case Idle: - if (hasPendingSymbolDependencies()) { + case NeedPlacement: + if (!hasPendingSymbolDependencies()) { attemptPlacement(); coalesce(); } break; - case Coalescing: - if (hasPendingSymbolDependencies()) { - state = NeedPlacement; + case NeedLayout: + if (!hasPendingSymbolDependencies()) { + redoLayout(); + if (!hasPendingSymbolDependencies()) { + coalesce(); + } else { + state = NeedPlacement; + } } break; - case NeedPlacement: - case NeedLayout: + case Idle: + case Coalescing: + assert(false); break; } } catch (...) { @@ -164,11 +201,6 @@ void GeometryTileWorker::symbolDependenciesChanged() { } } -void GeometryTileWorker::onGlyphsAvailable(GlyphPositionMap glyphs) { - glyphPositions = std::move(glyphs); - symbolDependenciesChanged(); // TODO: This is a clumsy way to join the "glyphs loaded" and "symbol dependencies changed" signals -} - void GeometryTileWorker::coalesced() { try { switch (state) { @@ -182,12 +214,18 @@ void GeometryTileWorker::coalesced() { case NeedLayout: redoLayout(); - coalesce(); + if (!hasPendingSymbolDependencies()) { + coalesce(); + } else { + state = NeedPlacement; + } break; case NeedPlacement: attemptPlacement(); - coalesce(); + if (!hasPendingSymbolDependencies()) { + coalesce(); + } break; } } catch (...) { @@ -216,8 +254,12 @@ void GeometryTileWorker::redoLayout() { std::unordered_map> buckets; auto featureIndex = std::make_unique(); BucketParameters parameters { id, mode }; + GlyphDependencies glyphDependencies; + IconDependencyMap iconDependencyMap; + glyphPositions.clear(); + icons.clear(); std::vector> groups = groupByLayout(*layers); for (auto& group : groups) { @@ -245,7 +287,7 @@ void GeometryTileWorker::redoLayout() { if (leader.is()) { symbolLayoutMap.emplace(leader.getID(), - leader.as()->impl->createLayout(parameters, group, *geometryLayer, glyphDependencies)); + leader.as()->impl->createLayout(parameters, group, *geometryLayer, glyphDependencies, iconDependencyMap)); } else { const Filter& filter = leader.baseImpl->filter; const std::string& sourceLayerID = leader.baseImpl->sourceLayer; @@ -281,8 +323,14 @@ void GeometryTileWorker::redoLayout() { } if (!glyphDependencies.empty()) { + waitingForGlyphs = true; parent.invoke(&GeometryTile::getGlyphs, std::move(glyphDependencies)); } + + if (!iconDependencyMap.empty()) { + waitingForIcons = true; + parent.invoke(&GeometryTile::getIcons, std::move(iconDependencyMap)); + } parent.invoke(&GeometryTile::onLayout, GeometryTile::LayoutResult { std::move(buckets), @@ -294,45 +342,11 @@ void GeometryTileWorker::redoLayout() { attemptPlacement(); } -bool GeometryTileWorker::hasPendingSymbolDependencies() const { - bool result = false; - - for (const auto& symbolLayout : symbolLayouts) { - if (symbolLayout->state == SymbolLayout::Pending) { - result = true; - } - } - - return result; -} - void GeometryTileWorker::attemptPlacement() { - if (!data || !layers || !placementConfig) { + if (!data || !layers || !placementConfig || hasPendingSymbolDependencies()) { return; } - bool canPlace = true; - - // Prepare as many SymbolLayouts as possible. - for (auto& symbolLayout : symbolLayouts) { - if (obsolete) { - return; - } - - if (symbolLayout->state == SymbolLayout::Pending) { - if (symbolLayout->canPrepare(glyphPositions)) { - symbolLayout->state = SymbolLayout::Prepared; - symbolLayout->prepare(glyphPositions); - } else { - canPlace = false; - } - } - } - - if (!canPlace) { - return; // We'll be notified (via `setPlacementConfig`) when it's time to try again. - } - auto collisionTile = std::make_unique(*placementConfig); std::unordered_map> buckets; @@ -340,6 +354,10 @@ void GeometryTileWorker::attemptPlacement() { if (obsolete) { return; } + + if (symbolLayout->state == SymbolLayout::Pending) { + symbolLayout->prepare(glyphPositions,icons); + } symbolLayout->state = SymbolLayout::Placed; if (!symbolLayout->hasSymbolInstances()) { diff --git a/src/mbgl/tile/geometry_tile_worker.hpp b/src/mbgl/tile/geometry_tile_worker.hpp index 3a62203b102..b7a01c721ea 100644 --- a/src/mbgl/tile/geometry_tile_worker.hpp +++ b/src/mbgl/tile/geometry_tile_worker.hpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -33,15 +34,17 @@ class GeometryTileWorker { void setLayers(std::vector>, uint64_t correlationID); void setData(std::unique_ptr, uint64_t correlationID); void setPlacementConfig(PlacementConfig, uint64_t correlationID); - void symbolDependenciesChanged(); void onGlyphsAvailable(GlyphPositionMap glyphs); + void onIconsAvailable(IconAtlasMap icons); private: void coalesce(); void coalesced(); void redoLayout(); void attemptPlacement(); + + void symbolDependenciesChanged(); bool hasPendingSymbolDependencies() const; ActorRef self; @@ -68,6 +71,9 @@ class GeometryTileWorker { std::vector> symbolLayouts; GlyphPositionMap glyphPositions; + bool waitingForGlyphs; + IconAtlasMap icons; + bool waitingForIcons; }; } // namespace mbgl diff --git a/src/mbgl/tile/tile.hpp b/src/mbgl/tile/tile.hpp index 613b15f36c8..a5f574b567f 100644 --- a/src/mbgl/tile/tile.hpp +++ b/src/mbgl/tile/tile.hpp @@ -50,7 +50,6 @@ class Tile : private util::noncopyable { virtual Bucket* getBucket(const style::Layer&) = 0; virtual void setPlacementConfig(const PlacementConfig&) {} - virtual void symbolDependenciesChanged() {}; virtual void redoLayout() {} virtual void queryRenderedFeatures( diff --git a/test/sprite/sprite_atlas.test.cpp b/test/sprite/sprite_atlas.test.cpp index 2c425a95d2d..2335165715c 100644 --- a/test/sprite/sprite_atlas.test.cpp +++ b/test/sprite/sprite_atlas.test.cpp @@ -29,15 +29,17 @@ TEST(SpriteAtlas, Basic) { EXPECT_EQ(112u, atlas.getSize().height); auto metro = *atlas.getIcon("metro"); + float imagePixelRatio = metro.relativePixelRatio * atlas.getPixelRatio(); EXPECT_EQ(0, metro.pos.x); EXPECT_EQ(0, metro.pos.y); EXPECT_EQ(20, metro.pos.w); EXPECT_EQ(20, metro.pos.h); - EXPECT_EQ(18, metro.spriteImage->getWidth()); - EXPECT_EQ(18, metro.spriteImage->getHeight()); - EXPECT_EQ(18u, metro.spriteImage->image.size.width); - EXPECT_EQ(18u, metro.spriteImage->image.size.height); - EXPECT_EQ(1.0f, metro.spriteImage->pixelRatio); + EXPECT_EQ(18, metro.width); + EXPECT_EQ(18, metro.height); + EXPECT_EQ(18u, metro.width * imagePixelRatio); + EXPECT_EQ(18u, metro.height * imagePixelRatio); + EXPECT_EQ(1.0f, imagePixelRatio); + EXPECT_EQ(63u, atlas.getAtlasImage().size.width); EXPECT_EQ(112u, atlas.getAtlasImage().size.height); @@ -82,15 +84,16 @@ TEST(SpriteAtlas, Size) { EXPECT_EQ(112u, atlas.getSize().height); auto metro = *atlas.getIcon("metro"); + float imagePixelRatio = metro.relativePixelRatio * atlas.getPixelRatio(); EXPECT_EQ(0, metro.pos.x); EXPECT_EQ(0, metro.pos.y); EXPECT_EQ(16, metro.pos.w); EXPECT_EQ(16, metro.pos.h); - EXPECT_EQ(18, metro.spriteImage->getWidth()); - EXPECT_EQ(18, metro.spriteImage->getHeight()); - EXPECT_EQ(18u, metro.spriteImage->image.size.width); - EXPECT_EQ(18u, metro.spriteImage->image.size.height); - EXPECT_EQ(1.0f, metro.spriteImage->pixelRatio); + EXPECT_EQ(18, metro.width); + EXPECT_EQ(18, metro.height); + EXPECT_EQ(18u, metro.width * imagePixelRatio); + EXPECT_EQ(18u, metro.height * imagePixelRatio); + EXPECT_EQ(1.0f, imagePixelRatio); // Now the image was created lazily. EXPECT_EQ(89u, atlas.getAtlasImage().size.width); @@ -108,15 +111,16 @@ TEST(SpriteAtlas, Updates) { atlas.setSprite("one", std::make_shared(PremultipliedImage({ 16, 12 }), 1)); auto one = *atlas.getIcon("one"); + float imagePixelRatio = one.relativePixelRatio * atlas.getPixelRatio(); EXPECT_EQ(0, one.pos.x); EXPECT_EQ(0, one.pos.y); EXPECT_EQ(20, one.pos.w); EXPECT_EQ(16, one.pos.h); - EXPECT_EQ(16, one.spriteImage->getWidth()); - EXPECT_EQ(12, one.spriteImage->getHeight()); - EXPECT_EQ(16u, one.spriteImage->image.size.width); - EXPECT_EQ(12u, one.spriteImage->image.size.height); - EXPECT_EQ(1.0f, one.spriteImage->pixelRatio); + EXPECT_EQ(16, one.width); + EXPECT_EQ(12, one.height); + EXPECT_EQ(16u, one.width * imagePixelRatio); + EXPECT_EQ(12u, one.height * imagePixelRatio); + EXPECT_EQ(1.0f, imagePixelRatio); // Now the image was created lazily. EXPECT_EQ(32u, atlas.getAtlasImage().size.width);