Cherry pick global symbol querying to release-boba

circle.yml

@@ -4,7 +4,6 @@ workflows:
   version: 2
   default:
     jobs:
-      - nitpick
       - clang-tidy:
           filters:
             branches:

platform/node/test/ignores.json

@@ -6,8 +6,6 @@
   "query-tests/geometry/multilinestring": "needs investigation",
   "query-tests/geometry/multipolygon": "needs investigation",
   "query-tests/geometry/polygon": "needs investigation",
-  "query-tests/symbol/panned-after-insert": "https://github.com/mapbox/mapbox-gl-native/issues/10408",
-  "query-tests/symbol/rotated-after-insert": "https://github.com/mapbox/mapbox-gl-native/issues/10408",
   "query-tests/world-wrapping/box": "skip - needs issue",
   "query-tests/world-wrapping/point": "skip - needs issue",
   "render-tests/background-color/transition": "https://github.com/mapbox/mapbox-gl-native/issues/10619",

src/mbgl/annotation/render_annotation_source.cpp

@@ -64,9 +64,8 @@ std::unordered_map<std::string, std::vector<Feature>>
 RenderAnnotationSource::queryRenderedFeatures(const ScreenLineString& geometry,
                                               const TransformState& transformState,
                                               const std::vector<const RenderLayer*>& layers,
-                                              const RenderedQueryOptions& options,
-                                              const CollisionIndex& collisionIndex) const {
-    return tilePyramid.queryRenderedFeatures(geometry, transformState, layers, options, collisionIndex);
+                                              const RenderedQueryOptions& options) const {
+    return tilePyramid.queryRenderedFeatures(geometry, transformState, layers, options);
 }
 
 std::vector<Feature> RenderAnnotationSource::querySourceFeatures(const SourceQueryOptions&) const {

src/mbgl/annotation/render_annotation_source.hpp

@@ -27,8 +27,7 @@ class RenderAnnotationSource : public RenderSource {
     queryRenderedFeatures(const ScreenLineString& geometry,
                           const TransformState& transformState,
                           const std::vector<const RenderLayer*>& layers,
-                          const RenderedQueryOptions& options,
-                          const CollisionIndex& collisionIndex) const final;
+                          const RenderedQueryOptions& options) const final;
 
     std::vector<Feature>
     querySourceFeatures(const SourceQueryOptions&) const final;

src/mbgl/geometry/feature_index.cpp

@@ -32,14 +32,6 @@ void FeatureIndex::insert(const GeometryCollection& geometries,
     }
 }
 
-static bool topDown(const IndexedSubfeature& a, const IndexedSubfeature& b) {
-    return a.sortIndex > b.sortIndex;
-}
-
-static bool topDownSymbols(const IndexedSubfeature& a, const IndexedSubfeature& b) {
-    return a.sortIndex < b.sortIndex;
-}
-
 void FeatureIndex::query(
         std::unordered_map<std::string, std::vector<Feature>>& result,
         const GeometryCoordinates& queryGeometry,
@@ -48,9 +40,7 @@ void FeatureIndex::query(
         const double scale,
         const RenderedQueryOptions& queryOptions,
         const UnwrappedTileID& tileID,
-        const std::string& sourceID,
         const std::vector<const RenderLayer*>& layers,
-        const CollisionIndex& collisionIndex,
         const float additionalQueryRadius) const {
 
     if (!tileData) {
@@ -67,31 +57,65 @@ void FeatureIndex::query(
                                                            convertPoint<float>(box.max + additionalRadius) });
 
 
-    std::sort(features.begin(), features.end(), topDown);
+    std::sort(features.begin(), features.end(), [](const IndexedSubfeature& a, const IndexedSubfeature& b) {
+        return a.sortIndex > b.sortIndex;
+    });
     size_t previousSortIndex = std::numeric_limits<size_t>::max();
     for (const auto& indexedFeature : features) {
 
         // If this feature is the same as the previous feature, skip it.
         if (indexedFeature.sortIndex == previousSortIndex) continue;
         previousSortIndex = indexedFeature.sortIndex;
 
-        addFeature(result, indexedFeature, queryGeometry, queryOptions, tileID.canonical, layers, bearing, pixelsToTileUnits);
+        addFeature(result, indexedFeature, queryOptions, tileID.canonical, layers, queryGeometry, bearing, pixelsToTileUnits);
     }
+}
+
+std::unordered_map<std::string, std::vector<Feature>> FeatureIndex::lookupSymbolFeatures(const std::vector<IndexedSubfeature>& symbolFeatures,
+                                                                                         const RenderedQueryOptions& queryOptions,
+                                                                                         const std::vector<const RenderLayer*>& layers,
+                                                                                         const OverscaledTileID& tileID,
+                                                                                        const std::shared_ptr<std::vector<size_t>>& featureSortOrder) const {
+    std::unordered_map<std::string, std::vector<Feature>> result;
+    if (!tileData) {
+        return result;
+    }
+    std::vector<IndexedSubfeature> sortedFeatures(symbolFeatures.begin(), symbolFeatures.end());
+
+    std::sort(sortedFeatures.begin(), sortedFeatures.end(), [featureSortOrder](const IndexedSubfeature& a, const IndexedSubfeature& b) {
+        // Same idea as the non-symbol sort order, but symbol features may have changed their sort order
+        // since their corresponding IndexedSubfeature was added to the CollisionIndex
+        // The 'featureSortOrder' is relatively inefficient for querying but cheap to build on every bucket sort
+        if (featureSortOrder) {
+            // queryRenderedSymbols documentation says we'll return features in
+            // "top-to-bottom" rendering order (aka last-to-first).
+            // Actually there can be multiple symbol instances per feature, so
+            // we sort each feature based on the first matching symbol instance.
+            auto sortedA = std::find(featureSortOrder->begin(), featureSortOrder->end(), a.index);
+            auto sortedB = std::find(featureSortOrder->begin(), featureSortOrder->end(), b.index);
+            assert(sortedA != featureSortOrder->end());
+            assert(sortedB != featureSortOrder->end());
+            return sortedA > sortedB;
+        } else {
+            // Bucket hasn't been re-sorted based on angle, so use same "reverse of appearance in source data"
+            // logic as non-symboles
+            return a.sortIndex > b.sortIndex;
+        }
+    });
 
-    std::vector<IndexedSubfeature> symbolFeatures = collisionIndex.queryRenderedSymbols(queryGeometry, tileID, sourceID);
-    std::sort(symbolFeatures.begin(), symbolFeatures.end(), topDownSymbols);
-    for (const auto& symbolFeature : symbolFeatures) {
-        addFeature(result, symbolFeature, queryGeometry, queryOptions, tileID.canonical, layers, bearing, pixelsToTileUnits);
+    for (const auto& symbolFeature : sortedFeatures) {
+        addFeature(result, symbolFeature, queryOptions, tileID.canonical, layers, GeometryCoordinates(), 0, 0);
     }
+    return result;
 }
 
 void FeatureIndex::addFeature(
     std::unordered_map<std::string, std::vector<Feature>>& result,
     const IndexedSubfeature& indexedFeature,
-    const GeometryCoordinates& queryGeometry,
     const RenderedQueryOptions& options,
     const CanonicalTileID& tileID,
     const std::vector<const RenderLayer*>& layers,
+    const GeometryCoordinates& queryGeometry,
     const float bearing,
     const float pixelsToTileUnits) const {
 

src/mbgl/geometry/feature_index.hpp

@@ -25,27 +25,24 @@ class IndexedSubfeature {
         , sourceLayerName(std::move(sourceLayerName_))
         , bucketName(std::move(bucketName_))
         , sortIndex(sortIndex_)
-        , tileID(0, 0, 0)
+        , bucketInstanceId(0)
     {}
 
-    IndexedSubfeature(std::size_t index_, std::string sourceLayerName_, std::string bucketName_, size_t sortIndex_,
-                      std::string sourceID_, CanonicalTileID tileID_)
-        : index(index_)
-        , sourceLayerName(std::move(sourceLayerName_))
-        , bucketName(std::move(bucketName_))
-        , sortIndex(std::move(sortIndex_))
-        , sourceID(std::move(sourceID_))
-        , tileID(std::move(tileID_))
-    {}
 
+    IndexedSubfeature(const IndexedSubfeature& other, uint32_t bucketInstanceId_)
+        : index(other.index)
+        , sourceLayerName(other.sourceLayerName)
+        , bucketName(other.bucketName)
+        , sortIndex(other.sortIndex)
+        , bucketInstanceId(bucketInstanceId_)
+    {}
     size_t index;
     std::string sourceLayerName;
     std::string bucketName;
     size_t sortIndex;
 
     // Only used for symbol features
-    std::string sourceID;
-    CanonicalTileID tileID;
+    uint32_t bucketInstanceId;
 };
 
 class FeatureIndex {
@@ -64,9 +61,7 @@ class FeatureIndex {
             const double scale,
             const RenderedQueryOptions& options,
             const UnwrappedTileID&,
-            const std::string&,
             const std::vector<const RenderLayer*>&,
-            const CollisionIndex&,
             const float additionalQueryRadius) const;
 
     static optional<GeometryCoordinates> translateQueryGeometry(
@@ -77,15 +72,22 @@ class FeatureIndex {
             const float pixelsToTileUnits);
 
     void setBucketLayerIDs(const std::string& bucketName, const std::vector<std::string>& layerIDs);
+
+    std::unordered_map<std::string, std::vector<Feature>> lookupSymbolFeatures(
+           const std::vector<IndexedSubfeature>& symbolFeatures,
+           const RenderedQueryOptions& options,
+           const std::vector<const RenderLayer*>& layers,
+           const OverscaledTileID& tileID,
+           const std::shared_ptr<std::vector<size_t>>& featureSortOrder) const;
 
 private:
     void addFeature(
             std::unordered_map<std::string, std::vector<Feature>>& result,
             const IndexedSubfeature&,
-            const GeometryCoordinates& queryGeometry,
             const RenderedQueryOptions& options,
             const CanonicalTileID&,
             const std::vector<const RenderLayer*>&,
+            const GeometryCoordinates& queryGeometry,
             const float bearing,
             const float pixelsToTileUnits) const;
 

src/mbgl/layout/symbol_layout.cpp

@@ -180,8 +180,7 @@ bool SymbolLayout::hasSymbolInstances() const {
 }
 
 void SymbolLayout::prepare(const GlyphMap& glyphMap, const GlyphPositions& glyphPositions,
-                           const ImageMap& imageMap, const ImagePositions& imagePositions,
-                           const OverscaledTileID& tileID, const std::string& sourceID) {
+                           const ImageMap& imageMap, const ImagePositions& imagePositions) {
     const bool textAlongLine = layout.get<TextRotationAlignment>() == AlignmentType::Map &&
         layout.get<SymbolPlacement>() == SymbolPlacementType::Line;
 
@@ -252,7 +251,7 @@ void SymbolLayout::prepare(const GlyphMap& glyphMap, const GlyphPositions& glyph
 
         // if either shapedText or icon position is present, add the feature
         if (shapedTextOrientations.first || shapedIcon) {
-            addFeature(std::distance(features.begin(), it), feature, shapedTextOrientations, shapedIcon, glyphPositionMap, tileID, sourceID);
+            addFeature(std::distance(features.begin(), it), feature, shapedTextOrientations, shapedIcon, glyphPositionMap);
         }
 
         feature.geometry.clear();
@@ -265,9 +264,7 @@ void SymbolLayout::addFeature(const std::size_t index,
                               const SymbolFeature& feature,
                               const std::pair<Shaping, Shaping>& shapedTextOrientations,
                               optional<PositionedIcon> shapedIcon,
-                              const GlyphPositionMap& glyphPositionMap,
-                              const OverscaledTileID& tileID,
-                              const std::string& sourceID) {
+                              const GlyphPositionMap& glyphPositionMap) {
     const float minScale = 0.5f;
     const float glyphSize = 24.0f;
 
@@ -296,8 +293,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(),
-                                     sourceID, tileID.canonical);
+    IndexedSubfeature indexedFeature(feature.index, sourceLayer->getName(), bucketName, symbolInstances.size());
 
     auto addSymbolInstance = [&] (const GeometryCoordinates& line, Anchor& anchor) {
         // https://github.com/mapbox/vector-tile-spec/tree/master/2.1#41-layers

src/mbgl/layout/symbol_layout.hpp

@@ -34,8 +34,7 @@ class SymbolLayout {
                  GlyphDependencies&);
 
     void prepare(const GlyphMap&, const GlyphPositions&,
-                 const ImageMap&, const ImagePositions&,
-                 const OverscaledTileID&, const std::string&);
+                 const ImageMap&, const ImagePositions&);
 
     std::unique_ptr<SymbolBucket> place(const bool showCollisionBoxes);
 
@@ -52,9 +51,7 @@ class SymbolLayout {
                     const SymbolFeature&,
                     const std::pair<Shaping, Shaping>& shapedTextOrientations,
                     optional<PositionedIcon> shapedIcon,
-                    const GlyphPositionMap&,
-                    const OverscaledTileID&,
-                    const std::string&);
+                    const GlyphPositionMap&);
 
     bool anchorIsTooClose(const std::u16string& text, const float repeatDistance, const Anchor&);
     std::map<std::u16string, std::vector<Anchor>> compareText;

src/mbgl/renderer/buckets/symbol_bucket.cpp

@@ -200,8 +200,12 @@ void SymbolBucket::sortFeatures(const float angle) {
     text.triangles.clear();
     icon.triangles.clear();
 
+    featureSortOrder = std::make_unique<std::vector<size_t>>();
+    featureSortOrder->reserve(symbolInstanceIndexes.size());
+
     for (auto i : symbolInstanceIndexes) {
         const SymbolInstance& symbolInstance = symbolInstances[i];
+        featureSortOrder->push_back(symbolInstance.featureIndex);
 
         if (symbolInstance.placedTextIndex) {
             addPlacedSymbol(text.triangles, text.placedSymbols[*symbolInstance.placedTextIndex]);

src/mbgl/renderer/buckets/symbol_bucket.hpp

@@ -131,6 +131,8 @@ class SymbolBucket : public Bucket {
 
     uint32_t bucketInstanceId = 0;
     bool justReloaded = false;
+
+    std::shared_ptr<std::vector<size_t>> featureSortOrder;
 };
 
 } // namespace mbgl

src/mbgl/renderer/render_source.hpp

@@ -64,8 +64,7 @@ class RenderSource : protected TileObserver {
     queryRenderedFeatures(const ScreenLineString& geometry,
                           const TransformState& transformState,
                           const std::vector<const RenderLayer*>& layers,
-                          const RenderedQueryOptions& options,
-                          const CollisionIndex& collisionIndex) const = 0;
+                          const RenderedQueryOptions& options) const = 0;
 
     virtual std::vector<Feature>
     querySourceFeatures(const SourceQueryOptions&) const = 0;

src/mbgl/renderer/renderer_impl.cpp

@@ -401,10 +401,6 @@ void Renderer::Impl::render(const UpdateParameters& updateParameters) {
         }
 
         placementChanged = newPlacement->commit(*placement, parameters.timePoint);
-        // commitFeatureIndexes depends on the assumption that no new FeatureIndex has been loaded since placement
-        // started. If we violate this assumption, then we need to either make CollisionIndex completely independendent of
-        // FeatureIndex, or find a way for its entries to point to multiple FeatureIndexes.
-        commitFeatureIndexes();
         crossTileSymbolIndex.pruneUnusedLayers(usedSymbolLayers);
         if (placementChanged || symbolBucketsChanged) {
             placement = std::move(newPlacement);
@@ -681,6 +677,39 @@ std::vector<Feature> Renderer::Impl::queryRenderedFeatures(const ScreenLineStrin
 
     return queryRenderedFeatures(geometry, options, layers);
 }
+
+void Renderer::Impl::queryRenderedSymbols(std::unordered_map<std::string, std::vector<Feature>>& resultsByLayer,
+                                          const ScreenLineString& geometry,
+                                          const std::vector<const RenderLayer*>& layers,
+                                          const RenderedQueryOptions& options) const {
+
+    auto renderedSymbols = placement->getCollisionIndex().queryRenderedSymbols(geometry);
+    std::vector<std::reference_wrapper<const RetainedQueryData>> bucketQueryData;
+    for (auto entry : renderedSymbols) {
+        bucketQueryData.push_back(placement->getQueryData(entry.first));
+    }
+    // Although symbol query is global, symbol results are only sortable within a bucket
+    // For a predictable global sort order, we sort the buckets based on their corresponding tile position
+    std::sort(bucketQueryData.begin(), bucketQueryData.end(), [](const RetainedQueryData& a, const RetainedQueryData& b) {
+        return
+            std::tie(a.tileID.canonical.z, a.tileID.canonical.y, a.tileID.wrap, a.tileID.canonical.x) <
+            std::tie(b.tileID.canonical.z, b.tileID.canonical.y, b.tileID.wrap, b.tileID.canonical.x);
+    });
+
+    for (auto wrappedQueryData : bucketQueryData) {
+        auto& queryData = wrappedQueryData.get();
+        auto bucketSymbols = queryData.featureIndex->lookupSymbolFeatures(renderedSymbols[queryData.bucketInstanceId],
+                                                                          options,
+                                                                          layers,
+                                                                          queryData.tileID,
+                                                                          queryData.featureSortOrder);
+
+        for (auto layer : bucketSymbols) {
+            auto& resultFeatures = resultsByLayer[layer.first];
+            std::move(layer.second.begin(), layer.second.end(), std::inserter(resultFeatures, resultFeatures.end()));
+        }
+    }
+}
 
 std::vector<Feature> Renderer::Impl::queryRenderedFeatures(const ScreenLineString& geometry, const RenderedQueryOptions& options, const std::vector<const RenderLayer*>& layers) const {
     std::unordered_set<std::string> sourceIDs;
@@ -691,10 +720,12 @@ std::vector<Feature> Renderer::Impl::queryRenderedFeatures(const ScreenLineStrin
     std::unordered_map<std::string, std::vector<Feature>> resultsByLayer;
     for (const auto& sourceID : sourceIDs) {
         if (RenderSource* renderSource = getRenderSource(sourceID)) {
-            auto sourceResults = renderSource->queryRenderedFeatures(geometry, transformState, layers, options, placement->getCollisionIndex());
+            auto sourceResults = renderSource->queryRenderedFeatures(geometry, transformState, layers, options);
             std::move(sourceResults.begin(), sourceResults.end(), std::inserter(resultsByLayer, resultsByLayer.begin()));
         }
     }
+
+    queryRenderedSymbols(resultsByLayer, geometry, layers, options);
 
     std::vector<Feature> result;
 
@@ -793,15 +824,6 @@ bool Renderer::Impl::hasTransitions(TimePoint timePoint) const {
     return false;
 }
 
-void Renderer::Impl::commitFeatureIndexes() {
-    for (auto& source : renderSources) {
-        for (auto& renderTile : source.second->getRenderTiles()) {
-            Tile& tile = renderTile.get().tile;
-            tile.commitFeatureIndex();
-        }
-    }
-}
-
 void Renderer::Impl::updateFadingTiles() {
     fadingTiles = false;
     for (auto& source : renderSources) {