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 0c8a92d8b7b..a99cb91d268 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -125,36 +125,16 @@ 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; } - 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; - } + + buckets = std::move(result.buckets); + + featureIndexPendingCommit = { std::move(result.featureIndex) }; + if (result.glyphAtlasImage) { glyphAtlasImage = std::move(*result.glyphAtlasImage); } @@ -196,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); } @@ -216,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; @@ -227,17 +202,11 @@ 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 when a global placement has run, + // synchronizing the global CollisionIndex with the latest buckets/FeatureIndex + if (featureIndexPendingCommit) { + featureIndex = std::move(*featureIndexPendingCommit); + featureIndexPendingCommit = nullopt; } } @@ -249,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; @@ -266,7 +235,6 @@ void GeometryTile::queryRenderedFeatures( util::tileSize * id.overscaleFactor(), std::pow(2, transformState.getZoom() - id.overscaledZ), options, - *data, id.toUnwrapped(), sourceID, layers, @@ -278,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; } @@ -292,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 8843d6e5791..418db4a0b26 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -65,33 +65,21 @@ 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; - - LayoutResult(std::unordered_map> nonSymbolBuckets_, - std::unique_ptr featureIndex_, - std::unique_ptr tileData_) - : 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_)), + LayoutResult(std::unordered_map> buckets_, + std::unique_ptr featureIndex_, + optional glyphAtlasImage_, + optional iconAtlasImage_) + : buckets(std::move(buckets_)), + featureIndex(std::move(featureIndex_)), 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); @@ -104,7 +92,7 @@ class GeometryTile : public Tile, public GlyphRequestor, ImageRequestor { protected: const GeometryTileData* getData() { - return data.get(); + return featureIndex ? featureIndex->getData() : nullptr; } private: @@ -123,17 +111,14 @@ class GeometryTile : public Tile, public GlyphRequestor, ImageRequestor { uint64_t correlationID = 0; - std::unordered_map> nonSymbolBuckets; + std::unordered_map> buckets; + + optional> featureIndexPendingCommit; 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..1378ad5d3ae 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(); + buckets.clear(); + featureIndex = std::make_unique(*data ? (*data)->clone() : nullptr); BucketParameters parameters { id, mode, pixelRatio }; GlyphDependencies glyphDependencies; @@ -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; } @@ -414,8 +448,6 @@ void GeometryTileWorker::attemptPlacement() { symbolLayoutsNeedPreparation = false; } - std::unordered_map> buckets; - for (auto& symbolLayout : symbolLayouts) { if (obsolete) { return; @@ -435,11 +467,12 @@ void GeometryTileWorker::attemptPlacement() { } firstLoad = false; - - parent.invoke(&GeometryTile::onPlacement, GeometryTile::PlacementResult { + + parent.invoke(&GeometryTile::onLayout, GeometryTile::LayoutResult { std::move(buckets), + std::move(featureIndex), 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..b5417c81147 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> buckets; 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);