From e4dda35f831ea64b2445036b9c64681c505724bf Mon Sep 17 00:00:00 2001 From: Chris Loer Date: Fri, 30 Mar 2018 16:02:56 -0700 Subject: [PATCH 1/3] [core] Convert GeometryTileWorker to "one-phase" loading Modest simplification refactoring (issue #10457). Also, fixes issue #11538, which was caused in part by a hole in the vestigial two-phase loading. --- src/mbgl/tile/geometry_tile.cpp | 46 ++----- src/mbgl/tile/geometry_tile.hpp | 33 ++--- src/mbgl/tile/geometry_tile_worker.cpp | 177 +++++++++++++++---------- src/mbgl/tile/geometry_tile_worker.hpp | 14 +- test/tile/vector_tile.test.cpp | 33 ----- 5 files changed, 141 insertions(+), 162 deletions(-) diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index 0c8a92d8b7b..f95d7220bdf 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -125,36 +125,17 @@ void GeometryTile::setShowCollisionBoxes(const bool showCollisionBoxes_) { } void GeometryTile::onLayout(LayoutResult result, const uint64_t resultCorrelationID) { - // Don't mark ourselves loaded or renderable until the first successful placement - // TODO: Ideally we'd render this tile without symbols as long as this tile wasn't - // replacing a tile at a different zoom that _did_ have symbols. - (void)resultCorrelationID; - nonSymbolBuckets = std::move(result.nonSymbolBuckets); - // It is possible for multiple onLayouts to be called before an onPlacement is called, in which - // case we will discard previous pending data - pendingFeatureIndex = {{ false, std::move(result.featureIndex) }}; - pendingData = {{ false, std::move(result.tileData) }}; - observer->onTileChanged(*this); -} - -void GeometryTile::onPlacement(PlacementResult result, const uint64_t resultCorrelationID) { loaded = true; renderable = true; if (resultCorrelationID == correlationID) { pending = false; } + + nonSymbolBuckets = std::move(result.nonSymbolBuckets); symbolBuckets = std::move(result.symbolBuckets); - // When symbolBuckets arrive, mark pendingData/FeatureIndex as "ready for commit" at the - // time of the next global placement. We are counting on these symbolBuckets being - // in sync with the data/index in the last `onLayout` call, even though the correlation IDs - // may not be the same (e.g. "setShowCollisionBoxes" could bump the correlation ID while - // waiting for glyph dependencies) - if (pendingData) { - pendingData->first = true; - } - if (pendingFeatureIndex) { - pendingFeatureIndex->first = true; - } + + dataPendingCommit = {{ std::move(result.tileData), std::move(result.featureIndex) }}; + if (result.glyphAtlasImage) { glyphAtlasImage = std::move(*result.glyphAtlasImage); } @@ -227,17 +208,12 @@ Bucket* GeometryTile::getBucket(const Layer::Impl& layer) const { } void GeometryTile::commitFeatureIndex() { - // We commit our pending FeatureIndex and GeometryTileData when: - // 1) An `onPlacement` result has delivered us updated symbolBuckets since we received the pending data - // 2) A global placement has run, synchronizing the global CollisionIndex with the latest - // symbolBuckets (and thus with the latest FeatureIndex/GeometryTileData) - if (pendingFeatureIndex && pendingFeatureIndex->first) { - featureIndex = std::move(pendingFeatureIndex->second); - pendingFeatureIndex = nullopt; - } - if (pendingData && pendingData->first) { - data = std::move(pendingData->second); - pendingData = nullopt; + // We commit our pending FeatureIndex and GeometryTileData when a global placement has run, + // synchronizing the global CollisionIndex with the latest symbolBuckets/FeatureIndex/GeometryTileData + if (dataPendingCommit) { + data = std::move(dataPendingCommit->first); + featureIndex = std::move(dataPendingCommit->second); + dataPendingCommit = nullopt; } } diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index 8843d6e5791..98b5f0c4908 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -68,30 +68,24 @@ class GeometryTile : public Tile, public GlyphRequestor, ImageRequestor { std::unordered_map> nonSymbolBuckets; std::unique_ptr featureIndex; std::unique_ptr tileData; + std::unordered_map> symbolBuckets; + optional glyphAtlasImage; + optional iconAtlasImage; LayoutResult(std::unordered_map> nonSymbolBuckets_, std::unique_ptr featureIndex_, - std::unique_ptr tileData_) + std::unique_ptr tileData_, + std::unordered_map> symbolBuckets_, + optional glyphAtlasImage_, + optional iconAtlasImage_) : nonSymbolBuckets(std::move(nonSymbolBuckets_)), featureIndex(std::move(featureIndex_)), - tileData(std::move(tileData_)) {} - }; - void onLayout(LayoutResult, uint64_t correlationID); - - class PlacementResult { - public: - std::unordered_map> symbolBuckets; - optional glyphAtlasImage; - optional iconAtlasImage; - - PlacementResult(std::unordered_map> symbolBuckets_, - optional glyphAtlasImage_, - optional iconAtlasImage_) - : symbolBuckets(std::move(symbolBuckets_)), + tileData(std::move(tileData_)), + symbolBuckets(std::move(symbolBuckets_)), glyphAtlasImage(std::move(glyphAtlasImage_)), iconAtlasImage(std::move(iconAtlasImage_)) {} }; - void onPlacement(PlacementResult, uint64_t correlationID); + void onLayout(LayoutResult, uint64_t correlationID); void onError(std::exception_ptr, uint64_t correlationID); @@ -124,16 +118,15 @@ class GeometryTile : public Tile, public GlyphRequestor, ImageRequestor { uint64_t correlationID = 0; std::unordered_map> nonSymbolBuckets; + std::unordered_map> symbolBuckets; + + optional, std::unique_ptr>> dataPendingCommit; std::unique_ptr featureIndex; - optional>> pendingFeatureIndex; std::unique_ptr data; - optional>> pendingData; optional glyphAtlasImage; optional iconAtlasImage; - std::unordered_map> symbolBuckets; - const MapMode mode; bool showCollisionBoxes; diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp index 8b6f1f63bf5..482cbe00532 100644 --- a/src/mbgl/tile/geometry_tile_worker.cpp +++ b/src/mbgl/tile/geometry_tile_worker.cpp @@ -41,48 +41,73 @@ GeometryTileWorker::GeometryTileWorker(ActorRef self_, GeometryTileWorker::~GeometryTileWorker() = default; /* - NOTE: The comments below are technically correct, but currently - conceptually misleading. The change to foreground label placement - means that: - (1) "placement" here is a misnomer: the remaining role of - "attemptPlacement" is symbol buffer generation - (2) Once a tile has completed layout, we will only run - "attemptPlacement" once - (3) Tiles won't be rendered until "attemptPlacement" has run once - - TODO: Simplify GeometryTileWorker to fit its new role - https://github.com/mapbox/mapbox-gl-native/issues/10457 - GeometryTileWorker is a state machine. This is its transition diagram. States are indicated by [state], lines are transitions triggered by messages, (parentheses) are actions taken on transition. - [idle] <----------------------------. - | | - set{Data,Layers,Placement}, symbolDependenciesChanged | - | | - (do layout/placement; self-send "coalesced") | - v | - [coalescing] --- coalesced ------------. - | | - .-----------------. .---------------. + [Idle] <-------------------------. + | | + set{Data,Layers}, symbolDependenciesChanged, | + setShowCollisionBoxes | + | | + (do parse and/or symbol layout; self-send "coalesced") | + v | + [Coalescing] --- coalesced ---------. + | | + .-----------. .---------------------. | | - .--- set{Data,Layers} setPlacement -----. - | | | | - | v v | - .-- [need layout] <-- set{Data,Layers} -- [need placement] --. + .--- set{Data,Layers} setShowCollisionBoxes, + | | symbolDependenciesChanged --. + | | | | + | v v | + .-- [NeedsParse] <-- set{Data,Layers} -- [NeedsSymbolLayout] ---. | | coalesced coalesced | | v v - (do layout or placement; self-send "coalesced"; goto [coalescing]) - - The idea is that in the [idle] state, layout or placement happens immediately - in response to a "set" message. During this processing, multiple "set" messages - might get queued in the mailbox. At the end of processing, we self-send "coalesced", - 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. + (do parse or symbol layout; self-send "coalesced"; goto [coalescing]) + + The idea is that in the [idle] state, parsing happens immediately in response to + a "set" message, and symbol layout happens once all symbol dependencies are met. + During this processing, multiple "set" messages might get queued in the mailbox. + At the end of processing, we self-send "coalesced", read all the queued messages + until we get to "coalesced", and then re-parse if there were one or more "set"s or + return to the [idle] state if not. + + One important goal of the design is to prevent starvation. Under heavy load new + requests for tiles should not prevent in progress request from completing. + It is nevertheless possible to restart an in-progress request: + + - [Idle] setData -> parse() + sends getGlyphs, hasPendingSymbolDependencies() is true + enters [Coalescing], sends coalesced + - [Coalescing] coalesced -> [Idle] + - [Idle] setData -> new parse(), interrupts old parse() + sends getGlyphs, hasPendingSymbolDependencies() is true + enters [Coalescing], sends coalesced + - [Coalescing] onGlyphsAvailable -> [NeedsSymbolLayout] + hasPendingSymbolDependencies() may or may not be true + - [NeedsSymbolLayout] coalesced -> performSymbolLayout() + Generates result depending on whether dependencies are met + -> [Idle] + + In this situation, we are counting on the idea that even with rapid changes to + the tile's data, the set of glyphs/images it requires will not keep growing without + limit. + + Although parsing (which populates all non-symbol buckets and requests dependencies + for symbol buckets) is internally separate from symbol layout, we only return + results to the foreground when we have completed both steps. Because we _move_ + the result buckets to the foreground, it is necessary to re-generate all buckets from + scratch for `setShowCollisionBoxes`, even though it only affects symbol layers. + + The GL JS equivalent (in worker_tile.js and vector_tile_worker_source.js) + is somewhat simpler because it relies on getGlyphs/getImages calls that transfer + an entire set of glyphs/images on every tile load, while the native logic + maintains a local state that can be incrementally updated. Because each tile load + call becomes self-contained, the equivalent of the coalescing logic is handled by + 'reloadTile' queueing a single extra 'reloadTile' callback to run after the next + completed parse. */ void GeometryTileWorker::setData(std::unique_ptr data_, uint64_t correlationID_) { @@ -92,14 +117,14 @@ void GeometryTileWorker::setData(std::unique_ptr data_, switch (state) { case Idle: - redoLayout(); + parse(); coalesce(); break; case Coalescing: - case NeedLayout: - case NeedPlacement: - state = NeedLayout; + case NeedsParse: + case NeedsSymbolLayout: + state = NeedsParse; break; } } catch (...) { @@ -114,16 +139,16 @@ void GeometryTileWorker::setLayers(std::vector> layers_, switch (state) { case Idle: - redoLayout(); + parse(); coalesce(); break; case Coalescing: - case NeedPlacement: - state = NeedLayout; + case NeedsSymbolLayout: + state = NeedsParse; break; - case NeedLayout: + case NeedsParse: break; } } catch (...) { @@ -138,16 +163,20 @@ void GeometryTileWorker::setShowCollisionBoxes(bool showCollisionBoxes_, uint64_ switch (state) { case Idle: - attemptPlacement(); - coalesce(); + if (!hasPendingParseResult()) { + // Trigger parse if nothing is in flight, otherwise symbol layout will automatically + // pick up the change + parse(); + coalesce(); + } break; case Coalescing: - state = NeedPlacement; + state = NeedsSymbolLayout; break; - case NeedPlacement: - case NeedLayout: + case NeedsSymbolLayout: + case NeedsParse: break; } } catch (...) { @@ -160,19 +189,23 @@ void GeometryTileWorker::symbolDependenciesChanged() { switch (state) { case Idle: if (symbolLayoutsNeedPreparation) { - attemptPlacement(); + // symbolLayoutsNeedPreparation can only be set true by parsing + // and the parse result can only be cleared by performSymbolLayout + // which also clears symbolLayoutsNeedPreparation + assert(hasPendingParseResult()); + performSymbolLayout(); coalesce(); } break; case Coalescing: if (symbolLayoutsNeedPreparation) { - state = NeedPlacement; + state = NeedsSymbolLayout; } break; - case NeedPlacement: - case NeedLayout: + case NeedsSymbolLayout: + case NeedsParse: break; } } catch (...) { @@ -191,13 +224,16 @@ void GeometryTileWorker::coalesced() { state = Idle; break; - case NeedLayout: - redoLayout(); + case NeedsParse: + parse(); coalesce(); break; - case NeedPlacement: - attemptPlacement(); + case NeedsSymbolLayout: + // We may have entered NeedsSymbolLayout while coalescing + // after a performSymbolLayout. In that case, we need to + // start over with parsing in order to do another layout. + hasPendingParseResult() ? performSymbolLayout() : parse(); coalesce(); break; } @@ -279,7 +315,7 @@ static std::vector> toRenderLayers(const std::vecto return renderLayers; } -void GeometryTileWorker::redoLayout() { +void GeometryTileWorker::parse() { if (!data || !layers) { return; } @@ -292,8 +328,8 @@ void GeometryTileWorker::redoLayout() { } std::unordered_map> symbolLayoutMap; - std::unordered_map> buckets; - auto featureIndex = std::make_unique(); + nonSymbolBuckets.clear(); + featureIndex = std::make_unique(); BucketParameters parameters { id, mode, pixelRatio }; GlyphDependencies glyphDependencies; @@ -352,7 +388,7 @@ void GeometryTileWorker::redoLayout() { } for (const auto& layer : group) { - buckets.emplace(layer->getID(), bucket); + nonSymbolBuckets.emplace(layer->getID(), bucket); } } } @@ -368,13 +404,7 @@ void GeometryTileWorker::redoLayout() { requestNewGlyphs(glyphDependencies); requestNewImages(imageDependencies); - parent.invoke(&GeometryTile::onLayout, GeometryTile::LayoutResult { - std::move(buckets), - std::move(featureIndex), - *data ? (*data)->clone() : nullptr, - }, correlationID); - - attemptPlacement(); + performSymbolLayout(); } bool GeometryTileWorker::hasPendingSymbolDependencies() const { @@ -385,9 +415,13 @@ bool GeometryTileWorker::hasPendingSymbolDependencies() const { } return !pendingImageDependencies.empty(); } + +bool GeometryTileWorker::hasPendingParseResult() const { + return bool(featureIndex); +} -void GeometryTileWorker::attemptPlacement() { - if (!data || !layers || hasPendingSymbolDependencies()) { +void GeometryTileWorker::performSymbolLayout() { + if (!data || !layers || !hasPendingParseResult() || hasPendingSymbolDependencies()) { return; } @@ -435,11 +469,14 @@ void GeometryTileWorker::attemptPlacement() { } firstLoad = false; - - parent.invoke(&GeometryTile::onPlacement, GeometryTile::PlacementResult { + + parent.invoke(&GeometryTile::onLayout, GeometryTile::LayoutResult { + std::move(nonSymbolBuckets), + std::move(featureIndex), + *data ? (*data)->clone() : nullptr, std::move(buckets), std::move(glyphAtlasImage), - std::move(iconAtlasImage), + std::move(iconAtlasImage) }, correlationID); } diff --git a/src/mbgl/tile/geometry_tile_worker.hpp b/src/mbgl/tile/geometry_tile_worker.hpp index 0276392679b..458ec769883 100644 --- a/src/mbgl/tile/geometry_tile_worker.hpp +++ b/src/mbgl/tile/geometry_tile_worker.hpp @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include #include @@ -43,8 +45,8 @@ class GeometryTileWorker { private: void coalesced(); - void redoLayout(); - void attemptPlacement(); + void parse(); + void performSymbolLayout(); void coalesce(); @@ -53,6 +55,7 @@ class GeometryTileWorker { void symbolDependenciesChanged(); bool hasPendingSymbolDependencies() const; + bool hasPendingParseResult() const; ActorRef self; ActorRef parent; @@ -62,12 +65,15 @@ class GeometryTileWorker { const std::atomic& obsolete; const MapMode mode; const float pixelRatio; + + std::unique_ptr featureIndex; + std::unordered_map> nonSymbolBuckets; enum State { Idle, Coalescing, - NeedLayout, - NeedPlacement + NeedsParse, + NeedsSymbolLayout }; State state = Idle; diff --git a/test/tile/vector_tile.test.cpp b/test/tile/vector_tile.test.cpp index 9d42f7ae74a..cd53c7c4a42 100644 --- a/test/tile/vector_tile.test.cpp +++ b/test/tile/vector_tile.test.cpp @@ -65,39 +65,6 @@ TEST(VectorTile, onError) { EXPECT_TRUE(tile.isComplete()); } -TEST(VectorTile, Issue7615) { - VectorTileTest test; - VectorTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, test.tileset); - - style::SymbolLayer symbolLayer("symbol", "source"); - std::vector symbolInstances; - auto symbolBucket = std::make_shared( - style::SymbolLayoutProperties::PossiblyEvaluated(), - std::map< - std::string, - std::pair>(), - 16.0f, 1.0f, 0.0f, false, false, false, std::move(symbolInstances)); - - // Simulate placement of a symbol layer. - tile.onPlacement(GeometryTile::PlacementResult { - {{ - symbolLayer.getID(), - symbolBucket - }}, - {}, - {}, - }, 0); - - // Subsequent onLayout should not cause the existing symbol bucket to be discarded. - tile.onLayout(GeometryTile::LayoutResult { - std::unordered_map>(), - nullptr, - nullptr, - }, 0); - - EXPECT_EQ(symbolBucket.get(), tile.getBucket(*symbolLayer.baseImpl)); -} - TEST(VectorTile, Issue8542) { VectorTileTest test; VectorTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, test.tileset); From e100ceff1296a47e0d5084eb9fdb0f5332486ca3 Mon Sep 17 00:00:00 2001 From: Chris Loer Date: Sat, 31 Mar 2018 18:17:16 -0700 Subject: [PATCH 2/3] [core] Consolidate GeometryTile symbol/nonSymbolBuckets Conversion to one-phase tile loading removed any need to track them separately. --- src/mbgl/tile/geometry_tile.cpp | 12 +++--------- src/mbgl/tile/geometry_tile.hpp | 12 ++++-------- src/mbgl/tile/geometry_tile_worker.cpp | 9 +++------ src/mbgl/tile/geometry_tile_worker.hpp | 2 +- 4 files changed, 11 insertions(+), 24 deletions(-) diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index f95d7220bdf..5089f950229 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -131,8 +131,7 @@ void GeometryTile::onLayout(LayoutResult result, const uint64_t resultCorrelatio pending = false; } - nonSymbolBuckets = std::move(result.nonSymbolBuckets); - symbolBuckets = std::move(result.symbolBuckets); + buckets = std::move(result.buckets); dataPendingCommit = {{ std::move(result.tileData), std::move(result.featureIndex) }}; @@ -177,11 +176,7 @@ void GeometryTile::upload(gl::Context& context) { } }; - for (auto& entry : nonSymbolBuckets) { - uploadFn(*entry.second); - } - - for (auto& entry : symbolBuckets) { + for (auto& entry : buckets) { uploadFn(*entry.second); } @@ -197,7 +192,6 @@ void GeometryTile::upload(gl::Context& context) { } Bucket* GeometryTile::getBucket(const Layer::Impl& layer) const { - const auto& buckets = layer.type == LayerType::Symbol ? symbolBuckets : nonSymbolBuckets; const auto it = buckets.find(layer.id); if (it == buckets.end()) { return nullptr; @@ -209,7 +203,7 @@ Bucket* GeometryTile::getBucket(const Layer::Impl& layer) const { void GeometryTile::commitFeatureIndex() { // We commit our pending FeatureIndex and GeometryTileData when a global placement has run, - // synchronizing the global CollisionIndex with the latest symbolBuckets/FeatureIndex/GeometryTileData + // synchronizing the global CollisionIndex with the latest buckets/FeatureIndex/GeometryTileData if (dataPendingCommit) { data = std::move(dataPendingCommit->first); featureIndex = std::move(dataPendingCommit->second); diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index 98b5f0c4908..0161e00efd2 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -65,23 +65,20 @@ class GeometryTile : public Tile, public GlyphRequestor, ImageRequestor { class LayoutResult { public: - std::unordered_map> nonSymbolBuckets; + std::unordered_map> buckets; std::unique_ptr featureIndex; std::unique_ptr tileData; - std::unordered_map> symbolBuckets; optional glyphAtlasImage; optional iconAtlasImage; - LayoutResult(std::unordered_map> nonSymbolBuckets_, + LayoutResult(std::unordered_map> buckets_, std::unique_ptr featureIndex_, std::unique_ptr tileData_, - std::unordered_map> symbolBuckets_, optional glyphAtlasImage_, optional iconAtlasImage_) - : nonSymbolBuckets(std::move(nonSymbolBuckets_)), + : buckets(std::move(buckets_)), featureIndex(std::move(featureIndex_)), tileData(std::move(tileData_)), - symbolBuckets(std::move(symbolBuckets_)), glyphAtlasImage(std::move(glyphAtlasImage_)), iconAtlasImage(std::move(iconAtlasImage_)) {} }; @@ -117,8 +114,7 @@ class GeometryTile : public Tile, public GlyphRequestor, ImageRequestor { uint64_t correlationID = 0; - std::unordered_map> nonSymbolBuckets; - std::unordered_map> symbolBuckets; + std::unordered_map> buckets; optional, std::unique_ptr>> dataPendingCommit; std::unique_ptr featureIndex; diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp index 482cbe00532..f57732117b1 100644 --- a/src/mbgl/tile/geometry_tile_worker.cpp +++ b/src/mbgl/tile/geometry_tile_worker.cpp @@ -328,7 +328,7 @@ void GeometryTileWorker::parse() { } std::unordered_map> symbolLayoutMap; - nonSymbolBuckets.clear(); + buckets.clear(); featureIndex = std::make_unique(); BucketParameters parameters { id, mode, pixelRatio }; @@ -388,7 +388,7 @@ void GeometryTileWorker::parse() { } for (const auto& layer : group) { - nonSymbolBuckets.emplace(layer->getID(), bucket); + buckets.emplace(layer->getID(), bucket); } } } @@ -448,8 +448,6 @@ void GeometryTileWorker::performSymbolLayout() { symbolLayoutsNeedPreparation = false; } - std::unordered_map> buckets; - for (auto& symbolLayout : symbolLayouts) { if (obsolete) { return; @@ -471,10 +469,9 @@ void GeometryTileWorker::performSymbolLayout() { firstLoad = false; parent.invoke(&GeometryTile::onLayout, GeometryTile::LayoutResult { - std::move(nonSymbolBuckets), + std::move(buckets), std::move(featureIndex), *data ? (*data)->clone() : nullptr, - std::move(buckets), std::move(glyphAtlasImage), std::move(iconAtlasImage) }, correlationID); diff --git a/src/mbgl/tile/geometry_tile_worker.hpp b/src/mbgl/tile/geometry_tile_worker.hpp index 458ec769883..b5417c81147 100644 --- a/src/mbgl/tile/geometry_tile_worker.hpp +++ b/src/mbgl/tile/geometry_tile_worker.hpp @@ -67,7 +67,7 @@ class GeometryTileWorker { const float pixelRatio; std::unique_ptr featureIndex; - std::unordered_map> nonSymbolBuckets; + std::unordered_map> buckets; enum State { Idle, From 1b1df6c5888977079215564e4751c06843d8d156 Mon Sep 17 00:00:00 2001 From: Chris Loer Date: Sat, 31 Mar 2018 19:47:40 -0700 Subject: [PATCH 3/3] [core] Make FeatureIndex own GeometryTileData. Prevents querying a FeatureIndex built against a separate set of data, which can lead to invalid index exceptions. The GeometryTileWorker 'data' member can still change independently of the data in the feature index, at the time 'setData' is called. The GeometryTileWorker maintains ownership of its local data (which may be used to re-parse) and clones the data for use by the FeatureIndex in the foreground. --- src/mbgl/geometry/feature_index.cpp | 17 ++++++++++------- src/mbgl/geometry/feature_index.hpp | 7 ++++--- src/mbgl/tile/geometry_tile.cpp | 22 ++++++++++------------ src/mbgl/tile/geometry_tile.hpp | 8 ++------ src/mbgl/tile/geometry_tile_worker.cpp | 3 +-- 5 files changed, 27 insertions(+), 30 deletions(-) diff --git a/src/mbgl/geometry/feature_index.cpp b/src/mbgl/geometry/feature_index.cpp index 6fb0d5e4465..c67786274ac 100644 --- a/src/mbgl/geometry/feature_index.cpp +++ b/src/mbgl/geometry/feature_index.cpp @@ -17,8 +17,9 @@ namespace mbgl { -FeatureIndex::FeatureIndex() - : grid(util::EXTENT, util::EXTENT, util::EXTENT / 16) { // 16x16 grid -> 32px cell +FeatureIndex::FeatureIndex(std::unique_ptr tileData_) + : grid(util::EXTENT, util::EXTENT, util::EXTENT / 16) // 16x16 grid -> 32px cell + , tileData(std::move(tileData_)) { } void FeatureIndex::insert(const GeometryCollection& geometries, @@ -47,12 +48,15 @@ void FeatureIndex::query( const double tileSize, const double scale, const RenderedQueryOptions& queryOptions, - const GeometryTileData& geometryTileData, const UnwrappedTileID& tileID, const std::string& sourceID, const std::vector& layers, const CollisionIndex& collisionIndex, const float additionalQueryRadius) const { + + if (!tileData) { + return; + } // Determine query radius const float pixelsToTileUnits = util::EXTENT / tileSize / scale; @@ -72,13 +76,13 @@ void FeatureIndex::query( if (indexedFeature.sortIndex == previousSortIndex) continue; previousSortIndex = indexedFeature.sortIndex; - addFeature(result, indexedFeature, queryGeometry, queryOptions, geometryTileData, tileID.canonical, layers, bearing, pixelsToTileUnits); + addFeature(result, indexedFeature, queryGeometry, queryOptions, tileID.canonical, layers, bearing, pixelsToTileUnits); } std::vector symbolFeatures = collisionIndex.queryRenderedSymbols(queryGeometry, tileID, sourceID); std::sort(symbolFeatures.begin(), symbolFeatures.end(), topDownSymbols); for (const auto& symbolFeature : symbolFeatures) { - addFeature(result, symbolFeature, queryGeometry, queryOptions, geometryTileData, tileID.canonical, layers, bearing, pixelsToTileUnits); + addFeature(result, symbolFeature, queryGeometry, queryOptions, tileID.canonical, layers, bearing, pixelsToTileUnits); } } @@ -87,7 +91,6 @@ void FeatureIndex::addFeature( const IndexedSubfeature& indexedFeature, const GeometryCoordinates& queryGeometry, const RenderedQueryOptions& options, - const GeometryTileData& geometryTileData, const CanonicalTileID& tileID, const std::vector& layers, const float bearing, @@ -113,7 +116,7 @@ void FeatureIndex::addFeature( } if (!geometryTileFeature) { - sourceLayer = geometryTileData.getLayer(indexedFeature.sourceLayerName); + sourceLayer = tileData->getLayer(indexedFeature.sourceLayerName); assert(sourceLayer); geometryTileFeature = sourceLayer->getFeature(indexedFeature.index); diff --git a/src/mbgl/geometry/feature_index.hpp b/src/mbgl/geometry/feature_index.hpp index e95bb94da6f..9e0c172342d 100644 --- a/src/mbgl/geometry/feature_index.hpp +++ b/src/mbgl/geometry/feature_index.hpp @@ -50,8 +50,10 @@ class IndexedSubfeature { class FeatureIndex { public: - FeatureIndex(); + FeatureIndex(std::unique_ptr tileData_); + const GeometryTileData* getData() { return tileData.get(); } + void insert(const GeometryCollection&, std::size_t index, const std::string& sourceLayerName, const std::string& bucketName); void query( @@ -61,7 +63,6 @@ class FeatureIndex { const double tileSize, const double scale, const RenderedQueryOptions& options, - const GeometryTileData&, const UnwrappedTileID&, const std::string&, const std::vector&, @@ -83,7 +84,6 @@ class FeatureIndex { const IndexedSubfeature&, const GeometryCoordinates& queryGeometry, const RenderedQueryOptions& options, - const GeometryTileData&, const CanonicalTileID&, const std::vector&, const float bearing, @@ -93,5 +93,6 @@ class FeatureIndex { unsigned int sortIndex = 0; std::unordered_map> bucketLayerIDs; + std::unique_ptr tileData; }; } // namespace mbgl diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index 5089f950229..a99cb91d268 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -133,7 +133,7 @@ void GeometryTile::onLayout(LayoutResult result, const uint64_t resultCorrelatio buckets = std::move(result.buckets); - dataPendingCommit = {{ std::move(result.tileData), std::move(result.featureIndex) }}; + featureIndexPendingCommit = { std::move(result.featureIndex) }; if (result.glyphAtlasImage) { glyphAtlasImage = std::move(*result.glyphAtlasImage); @@ -202,12 +202,11 @@ Bucket* GeometryTile::getBucket(const Layer::Impl& layer) const { } void GeometryTile::commitFeatureIndex() { - // We commit our pending FeatureIndex and GeometryTileData when a global placement has run, - // synchronizing the global CollisionIndex with the latest buckets/FeatureIndex/GeometryTileData - if (dataPendingCommit) { - data = std::move(dataPendingCommit->first); - featureIndex = std::move(dataPendingCommit->second); - dataPendingCommit = nullopt; + // We commit our pending FeatureIndex when a global placement has run, + // synchronizing the global CollisionIndex with the latest buckets/FeatureIndex + if (featureIndexPendingCommit) { + featureIndex = std::move(*featureIndexPendingCommit); + featureIndexPendingCommit = nullopt; } } @@ -219,7 +218,7 @@ void GeometryTile::queryRenderedFeatures( const RenderedQueryOptions& options, const CollisionIndex& collisionIndex) { - if (!featureIndex || !data) return; + if (!getData()) return; // Determine the additional radius needed factoring in property functions float additionalRadius = 0; @@ -236,7 +235,6 @@ void GeometryTile::queryRenderedFeatures( util::tileSize * id.overscaleFactor(), std::pow(2, transformState.getZoom() - id.overscaledZ), options, - *data, id.toUnwrapped(), sourceID, layers, @@ -248,8 +246,8 @@ void GeometryTile::querySourceFeatures( std::vector& result, const SourceQueryOptions& options) { - // Data not yet available - if (!data) { + // Data not yet available, or tile is empty + if (!getData()) { return; } @@ -262,7 +260,7 @@ void GeometryTile::querySourceFeatures( for (auto sourceLayer : *options.sourceLayers) { // Go throught all sourceLayers, if any // to gather all the features - auto layer = data->getLayer(sourceLayer); + auto layer = getData()->getLayer(sourceLayer); if (layer) { auto featureCount = layer->featureCount(); diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index 0161e00efd2..418db4a0b26 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -67,18 +67,15 @@ class GeometryTile : public Tile, public GlyphRequestor, ImageRequestor { public: std::unordered_map> buckets; std::unique_ptr featureIndex; - std::unique_ptr tileData; optional glyphAtlasImage; optional iconAtlasImage; LayoutResult(std::unordered_map> buckets_, std::unique_ptr featureIndex_, - std::unique_ptr tileData_, optional glyphAtlasImage_, optional iconAtlasImage_) : buckets(std::move(buckets_)), featureIndex(std::move(featureIndex_)), - tileData(std::move(tileData_)), glyphAtlasImage(std::move(glyphAtlasImage_)), iconAtlasImage(std::move(iconAtlasImage_)) {} }; @@ -95,7 +92,7 @@ class GeometryTile : public Tile, public GlyphRequestor, ImageRequestor { protected: const GeometryTileData* getData() { - return data.get(); + return featureIndex ? featureIndex->getData() : nullptr; } private: @@ -116,9 +113,8 @@ class GeometryTile : public Tile, public GlyphRequestor, ImageRequestor { std::unordered_map> buckets; - optional, std::unique_ptr>> dataPendingCommit; + optional> featureIndexPendingCommit; std::unique_ptr featureIndex; - std::unique_ptr data; optional glyphAtlasImage; optional iconAtlasImage; diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp index f57732117b1..1378ad5d3ae 100644 --- a/src/mbgl/tile/geometry_tile_worker.cpp +++ b/src/mbgl/tile/geometry_tile_worker.cpp @@ -329,7 +329,7 @@ void GeometryTileWorker::parse() { std::unordered_map> symbolLayoutMap; buckets.clear(); - featureIndex = std::make_unique(); + featureIndex = std::make_unique(*data ? (*data)->clone() : nullptr); BucketParameters parameters { id, mode, pixelRatio }; GlyphDependencies glyphDependencies; @@ -471,7 +471,6 @@ void GeometryTileWorker::performSymbolLayout() { parent.invoke(&GeometryTile::onLayout, GeometryTile::LayoutResult { std::move(buckets), std::move(featureIndex), - *data ? (*data)->clone() : nullptr, std::move(glyphAtlasImage), std::move(iconAtlasImage) }, correlationID);