[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 · May 3, 2018 · 27b2136 · 27b2136
1 parent 60ac08a
commit 27b2136
Show file tree

Hide file tree

Showing 11 changed files with 49 additions and 26 deletions.
diff --git a/mapbox-gl-js b/mapbox-gl-js
diff --git a/src/mbgl/geometry/feature_index.cpp b/src/mbgl/geometry/feature_index.cpp
@@ -25,14 +25,14 @@ FeatureIndex::FeatureIndex(std::unique_ptr<const GeometryTileData> 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<float>(envelope.min), convertPoint<float>(envelope.max)});
         }
     }
@@ -141,7 +141,7 @@ void FeatureIndex::addFeature(
     std::unique_ptr<GeometryTileLayer> sourceLayer;
     std::unique_ptr<GeometryTileFeature> 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<GeometryCoordinates> FeatureIndex::translateQueryGeometry(
     return translated;
 }
 
-void FeatureIndex::setBucketLayerIDs(const std::string& bucketName, const std::vector<std::string>& layerIDs) {
-    bucketLayerIDs[bucketName] = layerIDs;
+void FeatureIndex::setBucketLayerIDs(const std::string& bucketLeaderID, const std::vector<std::string>& layerIDs) {
+    bucketLayerIDs[bucketLeaderID] = layerIDs;
 }
 
 } // namespace mbgl
diff --git a/src/mbgl/geometry/feature_index.hpp b/src/mbgl/geometry/feature_index.hpp
@@ -25,7 +25,7 @@ class IndexedSubfeature {
     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 @@ class IndexedSubfeature {
     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 @@ class FeatureIndex {
 
     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<std::string, std::vector<Feature>>& result,
@@ -74,7 +74,7 @@ class FeatureIndex {
             const float bearing,
             const float pixelsToTileUnits);
 
-    void setBucketLayerIDs(const std::string& bucketName, const std::vector<std::string>& layerIDs);
+    void setBucketLayerIDs(const std::string& bucketLeaderID, const std::vector<std::string>& layerIDs);
 
     std::unordered_map<std::string, std::vector<Feature>> lookupSymbolFeatures(
            const std::vector<IndexedSubfeature>& symbolFeatures,

diff --git 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<GeometryTileLayer> 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<SymbolPlacement>();
 
     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<SymbolBucket> SymbolLayout::place(const bool showCollisionBoxes)
     const bool mayOverlap = layout.get<TextAllowOverlap>() || layout.get<IconAllowOverlap>() ||
         layout.get<TextIgnorePlacement>() || layout.get<IconIgnorePlacement>();
 
-    auto bucket = std::make_unique<SymbolBucket>(layout, layerPaintProperties, textSize, iconSize, zoom, sdfIcons, iconsNeedLinear, mayOverlap, std::move(symbolInstances));
+    auto bucket = std::make_unique<SymbolBucket>(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
@@ -43,7 +43,7 @@ class SymbolLayout {
     std::map<std::string,
         std::pair<style::IconPaintProperties::PossiblyEvaluated, style::TextPaintProperties::PossiblyEvaluated>> layerPaintProperties;
 
-    const std::string bucketName;
+    const std::string bucketLeaderID;
     std::vector<SymbolInstance> symbolInstances;
 
 private:

diff --git 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<SymbolInstance>&& 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
@@ -47,6 +47,7 @@ class SymbolBucket : public Bucket {
                  bool sdfIcons,
                  bool iconsNeedLinear,
                  bool sortFeaturesByY,
+                 const std::string bucketLeaderID,
                  const std::vector<SymbolInstance>&&);
 
     void upload(gl::Context&) override;
@@ -64,6 +65,8 @@ class SymbolBucket : public Bucket {
     const bool iconsNeedLinear;
     const bool sortFeaturesByY;
 
+    const std::string bucketLeaderID;
+
     optional<float> sortedAngle;
 
     bool staticUploaded = false;

diff --git 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<SymbolBucket*>(bucket));
         SymbolBucket& symbolBucket = *reinterpret_cast<SymbolBucket*>(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
@@ -56,6 +56,11 @@ void Placement::placeLayer(RenderSymbolLayer& symbolLayer, const mat4& projMatri
         auto bucket = geometryTile.getBucket(*symbolLayer.baseImpl);
         assert(dynamic_cast<SymbolBucket*>(bucket));
         SymbolBucket& symbolBucket = *reinterpret_cast<SymbolBucket*>(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<SymbolBucket*>(bucket));
         SymbolBucket& symbolBucket = *reinterpret_cast<SymbolBucket*>(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
@@ -110,10 +110,11 @@ TEST(Buckets, SymbolBucket) {
     bool sdfIcons = false;
     bool iconsNeedLinear = false;
     bool sortFeaturesByY = false;
+    std::string bucketLeaderID = "test";
     std::vector<SymbolInstance> 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
@@ -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<SymbolInstance> 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<SymbolInstance> 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<SymbolInstance> 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<SymbolInstance> 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<SymbolInstance> 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,20 +139,21 @@ 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<SymbolInstance> 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);
     std::vector<SymbolInstance> childInstances;
     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<SymbolInstance> 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<SymbolInstance> 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