[core] Bring more stability to the symbol placement

CHANGELOG.md

@@ -12,6 +12,10 @@
 
   New feature provides means of modifying style of a MapSnapshotter. The new API enables several use-cases, such as: adding route overlays, removing extra information (layers) from a base style, adding custom images that are missing from a style.
 
+- [core] Improve stability of symbol placement when the map is tilted ([#16287](https://github.com/mapbox/mapbox-gl-native/pull/16287))
+
+  These changes improve performance and bring more stability to the symbol placement for the tilted view, which is mainly used for navigation scenarios.
+
 ### 🐞 Bug fixes
 
 - [core] Fix iterators in addRegularDash() ([#16249](https://github.com/mapbox/mapbox-gl-native/pull/16249))

metrics/binary-size/android-arm64-v8a/metrics.json

@@ -3,7 +3,7 @@
         [
             "android-arm64-v8a",
             "/tmp/attach/install/android-arm64-v8a-release/lib/libmapbox-gl.so",
-            1952143
+            1972354
         ]
     ]
 }

metrics/binary-size/android-armeabi-v7a/metrics.json

@@ -3,7 +3,7 @@
         [
             "android-armeabi-v7a",
             "/tmp/attach/install/android-armeabi-v7a-release/lib/libmapbox-gl.so",
-            1647895
+            1664652
         ]
     ]
 }

src/mbgl/renderer/render_orchestrator.cpp

@@ -281,6 +281,7 @@ std::unique_ptr<RenderTree> RenderOrchestrator::createRenderTree(
         renderSources.emplace(entry.first, std::move(renderSource));
     }
     transformState = updateParameters->transformState;
+    const bool tiltedView = transformState.getPitch() != 0.0f;
 
     // Create parameters for the render tree.
     auto renderTreeParameters = std::make_unique<RenderTreeParameters>(updateParameters->transformState,
@@ -386,14 +387,20 @@ std::unique_ptr<RenderTree> RenderOrchestrator::createRenderTree(
             symbolBucketsAdded = symbolBucketsAdded || (result & CrossTileSymbolIndex::AddLayerResult::BucketsAdded);
             symbolBucketsChanged = symbolBucketsChanged || (result != CrossTileSymbolIndex::AddLayerResult::NoChanges);
         }
-        // We want new symbols to show up faster, however simple setting `placementChanged` to `true` would
-        // initiate placement too often as new buckets ususally come from several rendered tiles in a row within
-        // a short period of time. Instead, we squeeze placement update period to coalesce buckets updates from several
-        // tiles.
-        optional<Duration> maximumPlacementUpdatePeriod;
-        if (symbolBucketsAdded) maximumPlacementUpdatePeriod = optional<Duration>(Milliseconds(30));
+
+        optional<Duration> placementUpdatePeriodOverride;
+        if (symbolBucketsAdded && !tiltedView) {
+            // If the view is not tilted, we want *the new* symbols to show up faster, however simple setting
+            // `placementChanged` to `true` would initiate placement too often as new buckets usually come from several
+            // rendered tiles in a row within a short period of time. Instead, we squeeze placement update period to
+            // coalesce buckets updates from several tiles. On contrary, with the tilted view it's more important to
+            // make placement rarely for performance reasons and as the new symbols are normally "far away" and the user
+            // is not that interested to see them ASAP.
+            placementUpdatePeriodOverride = optional<Duration>(Milliseconds(30));
+        }
+
         renderTreeParameters->placementChanged = !placementController.placementIsRecent(
-            updateParameters->timePoint, updateParameters->transformState.getZoom(), maximumPlacementUpdatePeriod);
+            updateParameters->timePoint, updateParameters->transformState.getZoom(), placementUpdatePeriodOverride);
         symbolBucketsChanged |= renderTreeParameters->placementChanged;
 
         std::set<std::string> usedSymbolLayers;

src/mbgl/text/collision_index.cpp

@@ -16,19 +16,27 @@
 
 namespace mbgl {
 
+namespace {
 // When a symbol crosses the edge that causes it to be included in
 // collision detection, it will cause changes in the symbols around
 // it. This constant specifies how many pixels to pad the edge of
 // the viewport for collision detection so that the bulk of the changes
 // occur offscreen. Making this constant greater increases label
 // stability, but it's expensive.
-static const float viewportPaddingDefault = 100;
+const float viewportPaddingDefault = 100;
 // Viewport padding must be much larger for static tiles to avoid clipped labels.
-static const float viewportPaddingForStaticTiles = 1024;
+const float viewportPaddingForStaticTiles = 1024;
+
+inline float getViewportPadding(const TransformState& transformState, MapMode mapMode) {
+    if (mapMode == MapMode::Tile) return viewportPaddingForStaticTiles;
+    return (transformState.getPitch() != 0.0f) ? viewportPaddingDefault * 2 : viewportPaddingDefault;
+}
+
+} // namespace
 
 CollisionIndex::CollisionIndex(const TransformState& transformState_, MapMode mapMode)
     : transformState(transformState_),
-      viewportPadding(mapMode == MapMode::Tile ? viewportPaddingForStaticTiles : viewportPaddingDefault),
+      viewportPadding(getViewportPadding(transformState_, mapMode)),
       collisionGrid(transformState.getSize().width + 2 * viewportPadding,
                     transformState.getSize().height + 2 * viewportPadding,
                     25),

src/mbgl/text/placement.cpp

@@ -68,14 +68,10 @@ void PlacementController::setPlacement(Immutable<Placement> placement_) {
     stale = false;
 }
 
-bool PlacementController::placementIsRecent(TimePoint now, const float zoom, optional<Duration> maximumDuration) const {
+bool PlacementController::placementIsRecent(TimePoint now, const float zoom, optional<Duration> periodOverride) const {
     if (!placement->transitionsEnabled()) return false;
 
-    auto updatePeriod = placement->getUpdatePeriod(zoom);
-
-    if (maximumDuration) {
-        updatePeriod = std::min(*maximumDuration, updatePeriod);
-    }
+    auto updatePeriod = periodOverride ? *periodOverride : placement->getUpdatePeriod(zoom);
 
     return placement->getCommitTime() + updatePeriod > now;
 }
@@ -315,12 +311,12 @@ void Placement::placeSymbolBucket(const BucketPlacementData& params, std::set<ui
                         const auto prevAnchor = prevOffset->second.anchor;
                         auto found = std::find(variableTextAnchors.begin(), variableTextAnchors.end(), prevAnchor);
                         if (found != variableTextAnchors.begin() && found != variableTextAnchors.end()) {
-                            std::vector<style::TextVariableAnchorType> filtered;
-                            filtered.reserve(variableTextAnchors.size());
-                            filtered.push_back(prevAnchor);
-                            for (auto anchor : variableTextAnchors) {
-                                if (anchor != prevAnchor) {
-                                    filtered.push_back(anchor);
+                            std::vector<style::TextVariableAnchorType> filtered{prevAnchor};
+                            if (!isTiltedView()) {
+                                for (auto anchor : variableTextAnchors) {
+                                    if (anchor != prevAnchor) {
+                                        filtered.push_back(anchor);
+                                    }
                                 }
                             }
                             variableTextAnchors = std::move(filtered);
@@ -596,7 +592,8 @@ void Placement::placeSymbolBucket(const BucketPlacementData& params, std::set<ui
             {collisionIndex, UnwrappedTileID(z, x + 1, y), {-util::EXTENT, 0.0f}}  // right
         }};
 
-        auto collisionBoxIntersectsTileEdges = [&](const CollisionBox& collisionBox, Point<float> shift) -> bool {
+        auto collisionBoxIntersectsTileEdges = [&](const CollisionBox& collisionBox,
+                                                   Point<float> shift) noexcept->bool {
             bool intersects =
                 collisionIndex.intersectsTileEdges(collisionBox, shift, renderTile.matrix, pixelRatio, *tileBorders);
             // Check if this symbol intersects the neighbor tile borders. If so, it also shall be placed with priority.
@@ -608,7 +605,14 @@ void Placement::placeSymbolBucket(const BucketPlacementData& params, std::set<ui
             return intersects;
         };
 
-        auto symbolIntersectsTileEdges = [&](const SymbolInstance& symbol) -> bool {
+        auto symbolIntersectsTileEdges = [
+            &locationCache,
+            &collisionBoxIntersectsTileEdges,
+            variableAnchor,
+            pitchTextWithMap,
+            rotateTextWithMap,
+            bearing = state.getBearing()
+        ](const SymbolInstance& symbol) noexcept->bool {
             auto it = locationCache.find(symbol.crossTileID);
             if (it != locationCache.end()) return it->second;
 
@@ -627,7 +631,7 @@ void Placement::placeSymbolBucket(const BucketPlacementData& params, std::set<ui
                                                            symbol.textBoxScale,
                                                            rotateTextWithMap,
                                                            pitchTextWithMap,
-                                                           state.getBearing());
+                                                           bearing);
                 }
                 intersects = collisionBoxIntersectsTileEdges(textCollisionBox, offset);
             }
@@ -641,28 +645,49 @@ void Placement::placeSymbolBucket(const BucketPlacementData& params, std::set<ui
             return intersects;
         };
 
-        std::stable_sort(symbolInstances.begin(),
-                         symbolInstances.end(),
-                         [&symbolIntersectsTileEdges](const SymbolInstance& a, const SymbolInstance& b) {
-                             assert(!a.textCollisionFeature.alongLine);
-                             assert(!b.textCollisionFeature.alongLine);
-                             auto intersectsA = symbolIntersectsTileEdges(a);
-                             auto intersectsB = symbolIntersectsTileEdges(b);
-                             if (intersectsA) {
-                                 if (!intersectsB) return true;
-                                 // Both symbols are inrecepting the tile borders, we need a universal cross-tile rule
-                                 // to define which of them shall be placed first - use anchor `y` point.
-                                 return a.anchor.point.y < b.anchor.point.y;
-                             }
-                             return false;
-                         });
+        std::stable_sort(
+            symbolInstances.begin(),
+            symbolInstances.end(),
+            [&symbolIntersectsTileEdges](const SymbolInstance& a, const SymbolInstance& b) noexcept {
+                assert(!a.textCollisionFeature.alongLine);
+                assert(!b.textCollisionFeature.alongLine);
+                auto intersectsA = symbolIntersectsTileEdges(a);
+                auto intersectsB = symbolIntersectsTileEdges(b);
+                if (intersectsA) {
+                    if (!intersectsB) return true;
+                    // Both symbols are inrecepting the tile borders, we need a universal cross-tile rule
+                    // to define which of them shall be placed first - use anchor `y` point.
+                    return a.anchor.point.y < b.anchor.point.y;
+                }
+                return false;
+            });
 
         for (const SymbolInstance& symbol : symbolInstances) {
             placeSymbol(symbol);
         }
-
     } else {
-        for (const SymbolInstance& symbol : bucket.getSymbols(params.sortKeyRange)) {
+        SymbolInstanceReferences sortedSymbols = bucket.getSymbols(params.sortKeyRange);
+        auto* previousPlacement = getPrevPlacement();
+        if (previousPlacement && isTiltedView()) {
+            std::stable_sort(
+                sortedSymbols.begin(),
+                sortedSymbols.end(),
+                [previousPlacement](const SymbolInstance& a, const SymbolInstance& b) noexcept {
+                    auto* aPlacement = previousPlacement->getSymbolPlacement(a);
+                    auto* bPlacement = previousPlacement->getSymbolPlacement(b);
+                    if (!aPlacement) {
+                        // a < b, if 'a' is new and if 'b' was previously hidden.
+                        return bPlacement && !bPlacement->placed();
+                    }
+                    if (!bPlacement) {
+                        // a < b, if 'b' is new and 'a' was previously shown.
+                        return aPlacement && aPlacement->placed();
+                    }
+                    // a < b, if 'a' was shown and 'b' was hidden.
+                    return aPlacement->placed() && !bPlacement->placed();
+                });
+        }
+        for (const SymbolInstance& symbol : sortedSymbols) {
             placeSymbol(symbol);
         }
     }
@@ -1187,6 +1212,10 @@ void Placement::markUsedOrientation(SymbolBucket& bucket,
     }
 }
 
+bool Placement::isTiltedView() const {
+    return updateParameters->transformState.getPitch() != 0.0f;
+}
+
 float Placement::symbolFadeChange(TimePoint now) const {
     if (transitionsEnabled() &&
         transitionOptions.duration.value_or(util::DEFAULT_TRANSITION_DURATION) > Milliseconds(0)) {
@@ -1203,6 +1232,12 @@ float Placement::zoomAdjustment(const float zoom) const {
     return std::max(0.0, (placementZoom - zoom) / 1.5);
 }
 
+const JointPlacement* Placement::getSymbolPlacement(const SymbolInstance& symbol) const {
+    assert(symbol.crossTileID != 0);
+    auto found = placements.find(symbol.crossTileID);
+    return (found == placements.end()) ? &found->second : nullptr;
+}
+
 Duration Placement::getUpdatePeriod(const float zoom) const {
     // Even if transitionOptions.duration is set to a value < 300ms, we still wait for this default transition duration
     // before attempting another placement operation.

src/mbgl/text/placement.hpp

@@ -49,6 +49,8 @@ class JointPlacement {
         : text(text_), icon(icon_), skipFade(skipFade_)
     {}
 
+    bool placed() const { return text || icon; }
+
     const bool text;
     const bool icon;
     // skipFade = outside viewport, but within CollisionIndex::viewportPadding px of the edge
@@ -94,7 +96,7 @@ class PlacementController {
     void setPlacement(Immutable<Placement>);
     const Immutable<Placement>& getPlacement() const { return placement; }
     void setPlacementStale() { stale = true; }
-    bool placementIsRecent(TimePoint now, const float zoom, optional<Duration> maximumDuration = nullopt) const;
+    bool placementIsRecent(TimePoint now, const float zoom, optional<Duration> periodOverride = nullopt) const;
     bool hasTransitions(TimePoint now) const;
 
 private:
@@ -119,6 +121,7 @@ class Placement {
     Duration getUpdatePeriod(const float zoom) const;
 
     float zoomAdjustment(const float zoom) const;
+    const JointPlacement* getSymbolPlacement(const SymbolInstance&) const;
 
     const RetainedQueryData& getQueryData(uint32_t bucketInstanceId) const;
 
@@ -134,6 +137,7 @@ class Placement {
                                style::TextWritingModeType orientation) const;
     void markUsedOrientation(SymbolBucket&, style::TextWritingModeType, const SymbolInstance&) const;
     const Placement* getPrevPlacement() const { return prevPlacement ? prevPlacement->get() : nullptr; }
+    bool isTiltedView() const;
 
     std::shared_ptr<const UpdateParameters> updateParameters;
     CollisionIndex collisionIndex;