From 27b21363e62c105db0b040b4c5a5ef31170ebd30 Mon Sep 17 00:00:00 2001 From: Chris Loer Date: Fri, 27 Apr 2018 17:18:29 -0700 Subject: [core] Only run placement for first layer per SymbolBucket Native version of mapbox/mapbox-gl-js#6548. Port of mapbox/mapbox-gl-js#6550. Prevents symbols that share the same layout properties from colliding against each other. Bump GL JS pin to get regression test. Rename "bucketName" -> "bucketLeaderID" to make it clearer what it represents. --- mapbox-gl-js | 2 +- src/mbgl/geometry/feature_index.cpp | 10 +++++----- src/mbgl/geometry/feature_index.hpp | 10 +++++----- src/mbgl/layout/symbol_layout.cpp | 6 +++--- src/mbgl/layout/symbol_layout.hpp | 2 +- src/mbgl/renderer/buckets/symbol_bucket.cpp | 2 ++ src/mbgl/renderer/buckets/symbol_bucket.hpp | 3 +++ src/mbgl/text/cross_tile_symbol_index.cpp | 4 ++++ src/mbgl/text/placement.cpp | 9 +++++++++ test/gl/bucket.test.cpp | 3 ++- test/text/cross_tile_symbol_index.test.cpp | 24 ++++++++++++++---------- 11 files changed, 49 insertions(+), 26 deletions(-) diff --git a/mapbox-gl-js b/mapbox-gl-js index b0ba8bded5..5f3bdb9b00 160000 --- a/mapbox-gl-js +++ b/mapbox-gl-js @@ -1 +1 @@ -Subproject commit b0ba8bded514e791ba8bd7eda4a513a762c5e5ae +Subproject commit 5f3bdb9b00915d702ae9b967c5ebc15ab33ba653 diff --git a/src/mbgl/geometry/feature_index.cpp b/src/mbgl/geometry/feature_index.cpp index 520fd313a2..fdd9558d0b 100644 --- a/src/mbgl/geometry/feature_index.cpp +++ b/src/mbgl/geometry/feature_index.cpp @@ -25,14 +25,14 @@ FeatureIndex::FeatureIndex(std::unique_ptr tileData_) void FeatureIndex::insert(const GeometryCollection& geometries, std::size_t index, const std::string& sourceLayerName, - const std::string& bucketName) { + const std::string& bucketLeaderID) { for (const auto& ring : geometries) { auto envelope = mapbox::geometry::envelope(ring); if (envelope.min.x < util::EXTENT && envelope.min.y < util::EXTENT && envelope.max.x >= 0 && envelope.max.y >= 0) { - grid.insert(IndexedSubfeature(index, sourceLayerName, bucketName, sortIndex++), + grid.insert(IndexedSubfeature(index, sourceLayerName, bucketLeaderID, sortIndex++), {convertPoint(envelope.min), convertPoint(envelope.max)}); } } @@ -141,7 +141,7 @@ void FeatureIndex::addFeature( std::unique_ptr sourceLayer; std::unique_ptr geometryTileFeature; - for (const std::string& layerID : bucketLayerIDs.at(indexedFeature.bucketName)) { + for (const std::string& layerID : bucketLayerIDs.at(indexedFeature.bucketLeaderID)) { const RenderLayer* renderLayer = getRenderLayer(layerID); if (!renderLayer) { continue; @@ -190,8 +190,8 @@ optional FeatureIndex::translateQueryGeometry( return translated; } -void FeatureIndex::setBucketLayerIDs(const std::string& bucketName, const std::vector& layerIDs) { - bucketLayerIDs[bucketName] = layerIDs; +void FeatureIndex::setBucketLayerIDs(const std::string& bucketLeaderID, const std::vector& layerIDs) { + bucketLayerIDs[bucketLeaderID] = layerIDs; } } // namespace mbgl diff --git a/src/mbgl/geometry/feature_index.hpp b/src/mbgl/geometry/feature_index.hpp index cc91791d36..739c1f282f 100644 --- a/src/mbgl/geometry/feature_index.hpp +++ b/src/mbgl/geometry/feature_index.hpp @@ -25,7 +25,7 @@ public: IndexedSubfeature(std::size_t index_, std::string sourceLayerName_, std::string bucketName_, size_t sortIndex_) : index(index_) , sourceLayerName(std::move(sourceLayerName_)) - , bucketName(std::move(bucketName_)) + , bucketLeaderID(std::move(bucketName_)) , sortIndex(sortIndex_) , bucketInstanceId(0) {} @@ -34,13 +34,13 @@ public: IndexedSubfeature(const IndexedSubfeature& other, uint32_t bucketInstanceId_) : index(other.index) , sourceLayerName(other.sourceLayerName) - , bucketName(other.bucketName) + , bucketLeaderID(other.bucketLeaderID) , sortIndex(other.sortIndex) , bucketInstanceId(bucketInstanceId_) {} size_t index; std::string sourceLayerName; - std::string bucketName; + std::string bucketLeaderID; size_t sortIndex; // Only used for symbol features @@ -53,7 +53,7 @@ public: const GeometryTileData* getData() { return tileData.get(); } - void insert(const GeometryCollection&, std::size_t index, const std::string& sourceLayerName, const std::string& bucketName); + void insert(const GeometryCollection&, std::size_t index, const std::string& sourceLayerName, const std::string& bucketLeaderID); void query( std::unordered_map>& result, @@ -74,7 +74,7 @@ public: const float bearing, const float pixelsToTileUnits); - void setBucketLayerIDs(const std::string& bucketName, const std::vector& layerIDs); + void setBucketLayerIDs(const std::string& bucketLeaderID, const std::vector& layerIDs); std::unordered_map> lookupSymbolFeatures( const std::vector& symbolFeatures, diff --git a/src/mbgl/layout/symbol_layout.cpp b/src/mbgl/layout/symbol_layout.cpp index d3126bba45..b2f6fd450f 100644 --- a/src/mbgl/layout/symbol_layout.cpp +++ b/src/mbgl/layout/symbol_layout.cpp @@ -42,7 +42,7 @@ SymbolLayout::SymbolLayout(const BucketParameters& parameters, std::unique_ptr sourceLayer_, ImageDependencies& imageDependencies, GlyphDependencies& glyphDependencies) - : bucketName(layers.at(0)->getID()), + : bucketLeaderID(layers.at(0)->getID()), sourceLayer(std::move(sourceLayer_)), overscaling(parameters.tileID.overscaleFactor()), zoom(parameters.tileID.overscaledZ), @@ -294,7 +294,7 @@ void SymbolLayout::addFeature(const std::size_t index, : layout.get(); const float textRepeatDistance = symbolSpacing / 2; - IndexedSubfeature indexedFeature(feature.index, sourceLayer->getName(), bucketName, symbolInstances.size()); + IndexedSubfeature indexedFeature(feature.index, sourceLayer->getName(), bucketLeaderID, symbolInstances.size()); auto addSymbolInstance = [&] (const GeometryCoordinates& line, Anchor& anchor) { // https://github.com/mapbox/vector-tile-spec/tree/master/2.1#41-layers @@ -420,7 +420,7 @@ std::unique_ptr SymbolLayout::place(const bool showCollisionBoxes) const bool mayOverlap = layout.get() || layout.get() || layout.get() || layout.get(); - auto bucket = std::make_unique(layout, layerPaintProperties, textSize, iconSize, zoom, sdfIcons, iconsNeedLinear, mayOverlap, std::move(symbolInstances)); + auto bucket = std::make_unique(layout, layerPaintProperties, textSize, iconSize, zoom, sdfIcons, iconsNeedLinear, mayOverlap, bucketLeaderID, std::move(symbolInstances)); for (SymbolInstance &symbolInstance : bucket->symbolInstances) { diff --git a/src/mbgl/layout/symbol_layout.hpp b/src/mbgl/layout/symbol_layout.hpp index c93d8f4106..43b577859f 100644 --- a/src/mbgl/layout/symbol_layout.hpp +++ b/src/mbgl/layout/symbol_layout.hpp @@ -43,7 +43,7 @@ public: std::map> layerPaintProperties; - const std::string bucketName; + const std::string bucketLeaderID; std::vector symbolInstances; private: diff --git a/src/mbgl/renderer/buckets/symbol_bucket.cpp b/src/mbgl/renderer/buckets/symbol_bucket.cpp index 3d40a1714d..4fe03eb453 100644 --- a/src/mbgl/renderer/buckets/symbol_bucket.cpp +++ b/src/mbgl/renderer/buckets/symbol_bucket.cpp @@ -18,11 +18,13 @@ SymbolBucket::SymbolBucket(style::SymbolLayoutProperties::PossiblyEvaluated layo bool sdfIcons_, bool iconsNeedLinear_, bool sortFeaturesByY_, + const std::string bucketName_, const std::vector&& symbolInstances_) : layout(std::move(layout_)), sdfIcons(sdfIcons_), iconsNeedLinear(iconsNeedLinear_ || iconSize.isDataDriven() || !iconSize.isZoomConstant()), sortFeaturesByY(sortFeaturesByY_), + bucketLeaderID(std::move(bucketName_)), symbolInstances(std::move(symbolInstances_)), textSizeBinder(SymbolSizeBinder::create(zoom, textSize, TextSize::defaultValue())), iconSizeBinder(SymbolSizeBinder::create(zoom, iconSize, IconSize::defaultValue())) { diff --git a/src/mbgl/renderer/buckets/symbol_bucket.hpp b/src/mbgl/renderer/buckets/symbol_bucket.hpp index e52d18372d..e4aaf5ba30 100644 --- a/src/mbgl/renderer/buckets/symbol_bucket.hpp +++ b/src/mbgl/renderer/buckets/symbol_bucket.hpp @@ -47,6 +47,7 @@ public: bool sdfIcons, bool iconsNeedLinear, bool sortFeaturesByY, + const std::string bucketLeaderID, const std::vector&&); void upload(gl::Context&) override; @@ -64,6 +65,8 @@ public: const bool iconsNeedLinear; const bool sortFeaturesByY; + const std::string bucketLeaderID; + optional sortedAngle; bool staticUploaded = false; diff --git a/src/mbgl/text/cross_tile_symbol_index.cpp b/src/mbgl/text/cross_tile_symbol_index.cpp index 01a4a02b4e..b0c3511ce3 100644 --- a/src/mbgl/text/cross_tile_symbol_index.cpp +++ b/src/mbgl/text/cross_tile_symbol_index.cpp @@ -153,6 +153,10 @@ bool CrossTileSymbolIndex::addLayer(RenderSymbolLayer& symbolLayer) { auto bucket = renderTile.tile.getBucket(*symbolLayer.baseImpl); assert(dynamic_cast(bucket)); SymbolBucket& symbolBucket = *reinterpret_cast(bucket); + if (symbolBucket.bucketLeaderID != symbolLayer.getID()) { + // Only add this layer if it's the "group leader" for the bucket + continue; + } if (!symbolBucket.bucketInstanceId) { symbolBucket.bucketInstanceId = ++maxBucketInstanceId; diff --git a/src/mbgl/text/placement.cpp b/src/mbgl/text/placement.cpp index 43e8ff4f93..9883a1f456 100644 --- a/src/mbgl/text/placement.cpp +++ b/src/mbgl/text/placement.cpp @@ -56,6 +56,11 @@ void Placement::placeLayer(RenderSymbolLayer& symbolLayer, const mat4& projMatri auto bucket = geometryTile.getBucket(*symbolLayer.baseImpl); assert(dynamic_cast(bucket)); SymbolBucket& symbolBucket = *reinterpret_cast(bucket); + + if (symbolBucket.bucketLeaderID != symbolLayer.getID()) { + // Only place this layer if it's the "group leader" for the bucket + continue; + } auto& layout = symbolBucket.layout; @@ -230,6 +235,10 @@ void Placement::updateLayerOpacities(RenderSymbolLayer& symbolLayer) { auto bucket = renderTile.tile.getBucket(*symbolLayer.baseImpl); assert(dynamic_cast(bucket)); SymbolBucket& symbolBucket = *reinterpret_cast(bucket); + if (symbolBucket.bucketLeaderID != symbolLayer.getID()) { + // Only update opacities this layer if it's the "group leader" for the bucket + continue; + } updateBucketOpacities(symbolBucket, seenCrossTileIDs); } } diff --git a/test/gl/bucket.test.cpp b/test/gl/bucket.test.cpp index 8393fd130d..9b28b2e01a 100644 --- a/test/gl/bucket.test.cpp +++ b/test/gl/bucket.test.cpp @@ -110,10 +110,11 @@ TEST(Buckets, SymbolBucket) { bool sdfIcons = false; bool iconsNeedLinear = false; bool sortFeaturesByY = false; + std::string bucketLeaderID = "test"; std::vector symbolInstances; gl::Context context; - SymbolBucket bucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, std::move(symbolInstances) }; + SymbolBucket bucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(symbolInstances) }; ASSERT_FALSE(bucket.hasIconData()); ASSERT_FALSE(bucket.hasTextData()); ASSERT_FALSE(bucket.hasCollisionBoxData()); diff --git a/test/text/cross_tile_symbol_index.test.cpp b/test/text/cross_tile_symbol_index.test.cpp index 794c2b59a1..5d255a6105 100644 --- a/test/text/cross_tile_symbol_index.test.cpp +++ b/test/text/cross_tile_symbol_index.test.cpp @@ -25,12 +25,13 @@ TEST(CrossTileSymbolLayerIndex, addBucket) { bool sdfIcons = false; bool iconsNeedLinear = false; bool sortFeaturesByY = false; + std::string bucketLeaderID = "test"; OverscaledTileID mainID(6, 0, 6, 8, 8); std::vector mainInstances; mainInstances.push_back(makeSymbolInstance(1000, 1000, u"Detroit")); mainInstances.push_back(makeSymbolInstance(2000, 2000, u"Toronto")); - SymbolBucket mainBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, std::move(mainInstances) }; + SymbolBucket mainBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(mainInstances) }; mainBucket.bucketInstanceId = ++maxBucketInstanceId; index.addBucket(mainID, mainBucket, maxCrossTileID); @@ -45,7 +46,7 @@ TEST(CrossTileSymbolLayerIndex, addBucket) { childInstances.push_back(makeSymbolInstance(2000, 2000, u"Windsor")); childInstances.push_back(makeSymbolInstance(3000, 3000, u"Toronto")); childInstances.push_back(makeSymbolInstance(4001, 4001, u"Toronto")); - SymbolBucket childBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, std::move(childInstances) }; + SymbolBucket childBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(childInstances) }; childBucket.bucketInstanceId = ++maxBucketInstanceId; index.addBucket(childID, childBucket, maxCrossTileID); @@ -61,7 +62,7 @@ TEST(CrossTileSymbolLayerIndex, addBucket) { OverscaledTileID parentID(5, 0, 5, 4, 4); std::vector parentInstances; parentInstances.push_back(makeSymbolInstance(500, 500, u"Detroit")); - SymbolBucket parentBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, std::move(parentInstances) }; + SymbolBucket parentBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(parentInstances) }; parentBucket.bucketInstanceId = ++maxBucketInstanceId; index.addBucket(parentID, parentBucket, maxCrossTileID); @@ -77,7 +78,7 @@ TEST(CrossTileSymbolLayerIndex, addBucket) { std::vector grandchildInstances; grandchildInstances.push_back(makeSymbolInstance(4000, 4000, u"Detroit")); grandchildInstances.push_back(makeSymbolInstance(4000, 4000, u"Windsor")); - SymbolBucket grandchildBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, std::move(grandchildInstances) }; + SymbolBucket grandchildBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(grandchildInstances) }; grandchildBucket.bucketInstanceId = ++maxBucketInstanceId; index.addBucket(grandchildID, grandchildBucket, maxCrossTileID); @@ -98,17 +99,18 @@ TEST(CrossTileSymbolLayerIndex, resetIDs) { bool sdfIcons = false; bool iconsNeedLinear = false; bool sortFeaturesByY = false; + std::string bucketLeaderID = "test"; OverscaledTileID mainID(6, 0, 6, 8, 8); std::vector mainInstances; mainInstances.push_back(makeSymbolInstance(1000, 1000, u"Detroit")); - SymbolBucket mainBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, std::move(mainInstances) }; + SymbolBucket mainBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(mainInstances) }; mainBucket.bucketInstanceId = ++maxBucketInstanceId; OverscaledTileID childID(7, 0, 7, 16, 16); std::vector childInstances; childInstances.push_back(makeSymbolInstance(2000, 2000, u"Detroit")); - SymbolBucket childBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, std::move(childInstances) }; + SymbolBucket childBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(childInstances) }; childBucket.bucketInstanceId = ++maxBucketInstanceId; // assigns a new id @@ -137,12 +139,13 @@ TEST(CrossTileSymbolLayerIndex, noDuplicatesWithinZoomLevel) { bool sdfIcons = false; bool iconsNeedLinear = false; bool sortFeaturesByY = false; + std::string bucketLeaderID = "test"; OverscaledTileID mainID(6, 0, 6, 8, 8); std::vector mainInstances; mainInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // A mainInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // B - SymbolBucket mainBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, std::move(mainInstances) }; + SymbolBucket mainBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(mainInstances) }; mainBucket.bucketInstanceId = ++maxBucketInstanceId; OverscaledTileID childID(7, 0, 7, 16, 16); @@ -150,7 +153,7 @@ TEST(CrossTileSymbolLayerIndex, noDuplicatesWithinZoomLevel) { childInstances.push_back(makeSymbolInstance(2000, 2000, u"")); // A' childInstances.push_back(makeSymbolInstance(2000, 2000, u"")); // B' childInstances.push_back(makeSymbolInstance(2000, 2000, u"")); // C' - SymbolBucket childBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, std::move(childInstances) }; + SymbolBucket childBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(childInstances) }; childBucket.bucketInstanceId = ++maxBucketInstanceId; // assigns new ids @@ -174,19 +177,20 @@ TEST(CrossTileSymbolLayerIndex, bucketReplacement) { bool sdfIcons = false; bool iconsNeedLinear = false; bool sortFeaturesByY = false; + std::string bucketLeaderID = "test"; OverscaledTileID tileID(6, 0, 6, 8, 8); std::vector firstInstances; firstInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // A firstInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // B - SymbolBucket firstBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, std::move(firstInstances) }; + SymbolBucket firstBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(firstInstances) }; firstBucket.bucketInstanceId = ++maxBucketInstanceId; std::vector secondInstances; secondInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // A' secondInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // B' secondInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // C' - SymbolBucket secondBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, std::move(secondInstances) }; + SymbolBucket secondBucket { layout, {}, 16.0f, 1.0f, 0, sdfIcons, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(secondInstances) }; secondBucket.bucketInstanceId = ++maxBucketInstanceId; // assigns new ids -- cgit v1.2.1