[core] Avoid clipping symbols in continuous mode

src/mbgl/layout/symbol_layout.cpp

@@ -262,8 +262,6 @@ void SymbolLayout::addFeature(const GeometryCollection &lines,
     const SymbolPlacementType iconPlacement = layout.get<IconRotationAlignment>() != AlignmentType::Map
                                                   ? SymbolPlacementType::Point
                                                   : layout.get<SymbolPlacement>();
-    const bool mayOverlap = layout.get<TextAllowOverlap>() || layout.get<IconAllowOverlap>() ||
-        layout.get<TextIgnorePlacement>() || layout.get<IconIgnorePlacement>();
     const bool isLine = layout.get<SymbolPlacement>() == SymbolPlacementType::Line;
     const float textRepeatDistance = symbolSpacing / 2;
 
@@ -289,22 +287,25 @@ void SymbolLayout::addFeature(const GeometryCollection &lines,
                 }
             }
 
-            const bool inside = !(anchor.point.x < 0 || anchor.point.x > util::EXTENT || anchor.point.y < 0 || anchor.point.y > util::EXTENT);
+            // https://github.com/mapbox/vector-tile-spec/tree/master/2.1#41-layers
+            // +-------------------+ Symbols with anchors located on tile edges
+            // |(0,0)             || are duplicated on neighbor tiles.
+            // |                  ||
+            // |                  || In continuous mode, to avoid overdraw we
+            // |                  || skip symbols located on the extent edges.
+            // |       Tile       || In still mode, we include the features in
+            // |                  || the buffers for both tiles and clip them
+            // |                  || at draw time.
+            // |                  ||
+            // +-------------------| In this scenario, the inner bounding box
+            // +-------------------+ is called 'withinPlus0', and the outer
+            //       (extent,extent) is called 'inside'.
+            const bool withinPlus0 = anchor.point.x >= 0 && anchor.point.x < util::EXTENT && anchor.point.y >= 0 && anchor.point.y < util::EXTENT;
+            const bool inside = withinPlus0 || anchor.point.x == util::EXTENT || anchor.point.y == util::EXTENT;
 
             if (avoidEdges && !inside) continue;
 
-            // Normally symbol layers are drawn across tile boundaries. Only symbols
-            // with their anchors within the tile boundaries are added to the buffers
-            // to prevent symbols from being drawn twice.
-            //
-            // Symbols in layers with overlap are sorted in the y direction so that
-            // symbols lower on the canvas are drawn on top of symbols near the top.
-            // To preserve this order across tile boundaries these symbols can't
-            // be drawn across tile boundaries. Instead they need to be included in
-            // the buffers for both tiles and clipped to tile boundaries at draw time.
-            //
-            // TODO remove the `&& false` when is #1673 implemented
-            const bool addToBuffers = (mode == MapMode::Still) || inside || (mayOverlap && false);
+            const bool addToBuffers = mode == MapMode::Still || withinPlus0;
 
             symbolInstances.emplace_back(anchor, line, shapedText, shapedIcon, layout, addToBuffers, symbolInstances.size(),
                     textBoxScale, textPadding, textPlacement,

src/mbgl/renderer/painter.cpp

@@ -125,7 +125,7 @@ void Painter::render(const Style& style, const FrameData& frame_, View& view, Sp
     spriteAtlas = style.spriteAtlas.get();
     lineAtlas = style.lineAtlas.get();
 
-    RenderData renderData = style.getRenderData(frame.debugOptions);
+    RenderData renderData = style.getRenderData(frame.debugOptions, state.getAngle());
     const std::vector<RenderItem>& order = renderData.order;
     const std::unordered_set<Source*>& sources = renderData.sources;
 

src/mbgl/renderer/painter_symbol.cpp

@@ -35,26 +35,18 @@ void Painter::renderSymbol(PaintParameters& parameters,
                      const auto& buffers,
                      const SymbolPropertyValues& values_)
     {
-        // In some cases, we disable the stencil test so that labels aren't clipped
-        // to tile boundaries.
-        //
-        // Layers with features that may be drawn overlapping aren't clipped. These
-        // layers are sorted in the y direction, and to draw the correct ordering near
-        // tile edges the icons are included in both tiles and clipped when drawing.
-        //
-        // TODO remove the `true ||` when #1673 is implemented
-        const bool drawAcrossEdges = (frame.mapMode == MapMode::Continuous) && (true || !(layout.get<TextAllowOverlap>() || layout.get<IconAllowOverlap>() ||
-              layout.get<TextIgnorePlacement>() || layout.get<IconIgnorePlacement>()));
+        // We clip symbols to their tile extent in still mode.
+        const bool needsClipping = frame.mapMode == MapMode::Still;
 
         program.draw(
             context,
             gl::Triangles(),
             values_.pitchAlignment == AlignmentType::Map
                 ? depthModeForSublayer(0, gl::DepthMode::ReadOnly)
                 : gl::DepthMode::disabled(),
-            drawAcrossEdges
-                ? gl::StencilMode::disabled()
-                : stencilModeForClipping(tile.clip),
+            needsClipping
+                ? stencilModeForClipping(tile.clip)
+                : gl::StencilMode::disabled(),
             colorModeForRenderPass(),
             std::move(uniformValues),
             *buffers.vertexBuffer,

src/mbgl/style/style.cpp

@@ -353,7 +353,7 @@ bool Style::isLoaded() const {
     return true;
 }
 
-RenderData Style::getRenderData(MapDebugOptions debugOptions) const {
+RenderData Style::getRenderData(MapDebugOptions debugOptions, float angle) const {
     RenderData result;
 
     for (const auto& source : sources) {
@@ -362,6 +362,9 @@ RenderData Style::getRenderData(MapDebugOptions debugOptions) const {
         }
     }
 
+    const bool isLeft = std::abs(angle) > M_PI_2;
+    const bool isBottom = angle < 0;
+
     for (const auto& layer : layers) {
         if (!layer->baseImpl->needsRendering(zoomHistory.lastZoom)) {
             continue;
@@ -395,16 +398,37 @@ RenderData Style::getRenderData(MapDebugOptions debugOptions) const {
             continue;
         }
 
-        for (auto& pair : source->baseImpl->getRenderTiles()) {
-            auto& tile = pair.second;
+        auto& renderTiles = source->baseImpl->getRenderTiles();
+        const bool symbolLayer = layer->is<SymbolLayer>();
+
+        // Sort symbol tiles in opposite y position, so tiles with overlapping
+        // symbols are drawn on top of each other, with lower symbols being
+        // drawn on top of higher symbols.
+        std::vector<std::reference_wrapper<RenderTile>> sortedTiles;
+        std::transform(renderTiles.begin(), renderTiles.end(), std::back_inserter(sortedTiles),
+                [](auto& pair) { return std::ref(pair.second); });
+        if (symbolLayer) {
+            std::sort(sortedTiles.begin(), sortedTiles.end(),
+                      [isLeft, isBottom](const RenderTile& a, const RenderTile& b) {
+                bool sortX = a.id.canonical.x > b.id.canonical.x;
+                bool sortW = a.id.wrap > b.id.wrap;
+                bool sortY = a.id.canonical.y > b.id.canonical.y;
+                return
+                    a.id.canonical.y != b.id.canonical.y ? (isLeft ? sortY : !sortY) :
+                    a.id.wrap != b.id.wrap ? (isBottom ? sortW : !sortW) : (isBottom ? sortX : !sortX);
+            });
+        }
+
+        for (auto& tileRef : sortedTiles) {
+            auto& tile = tileRef.get();
             if (!tile.tile.isRenderable()) {
                 continue;
             }
 
             // We're not clipping symbol layers, so when we have both parents and children of symbol
             // layers, we drop all children in favor of their parent to avoid duplicate labels.
             // See https://github.com/mapbox/mapbox-gl-native/issues/2482
-            if (layer->is<SymbolLayer>()) {
+            if (symbolLayer) {
                 bool skip = false;
                 // Look back through the buckets we decided to render to find out whether there is
                 // already a bucket from this layer that is a parent of this tile. Tiles are ordered

src/mbgl/style/style.hpp

@@ -90,7 +90,7 @@ class Style : public GlyphAtlasObserver,
     bool hasClass(const std::string&) const;
     std::vector<std::string> getClasses() const;
 
-    RenderData getRenderData(MapDebugOptions) const;
+    RenderData getRenderData(MapDebugOptions, float angle) const;
 
     std::vector<Feature> queryRenderedFeatures(const QueryParameters&) const;
 

test/api/annotations.test.cpp

@@ -47,14 +47,11 @@ TEST(Annotations, SymbolAnnotation) {
 
     auto size = test.view.size;
     auto screenBox = ScreenBox { {}, { double(size.width), double(size.height) } };
-    auto features = test.map.queryPointAnnotations(screenBox);
-    EXPECT_EQ(features.size(), 1u);
-
-    test.map.setZoom(test.map.getMaxZoom());
-    test.checkRendering("point_annotation");
-
-    features = test.map.queryPointAnnotations(screenBox);
-    EXPECT_EQ(features.size(), 1u);
+    for (uint8_t zoom = test.map.getMinZoom(); zoom <= test.map.getMaxZoom(); ++zoom) {
+        test.map.setZoom(zoom);
+        test.checkRendering("point_annotation");
+        EXPECT_EQ(test.map.queryPointAnnotations(screenBox).size(), 1u);
+    }
 }
 
 TEST(Annotations, LineAnnotation) {