[core] Cleanup in response to review comments.

include/mbgl/tile/tile_id.hpp

@@ -30,9 +30,9 @@ class CanonicalTileID {
     CanonicalTileID scaledTo(uint8_t z) const;
     std::array<CanonicalTileID, 4> children() const;
 
-    const uint8_t z;
-    const uint32_t x;
-    const uint32_t y;
+    uint8_t z;
+    uint32_t x;
+    uint32_t y;
 };
 
 ::std::ostream& operator<<(::std::ostream& os, const CanonicalTileID& rhs);

src/mbgl/geometry/feature_index.hpp

@@ -25,21 +25,17 @@ class IndexedSubfeature {
         , sourceLayerName(std::move(sourceLayerName_))
         , bucketName(std::move(bucketName_))
         , sortIndex(sortIndex_)
-        , z(0)
-        , x(0)
-        , y(0)
+        , tileID(0, 0, 0)
     {}
 
     IndexedSubfeature(std::size_t index_, std::string sourceLayerName_, std::string bucketName_, size_t sortIndex_,
-                      std::string sourceID_, uint8_t z_, uint32_t x_, uint32_t y_)
+                      std::string sourceID_, CanonicalTileID tileID_)
         : index(index_)
         , sourceLayerName(std::move(sourceLayerName_))
         , bucketName(std::move(bucketName_))
         , sortIndex(std::move(sortIndex_))
         , sourceID(std::move(sourceID_))
-        , z(z_)
-        , x(x_)
-        , y(y_)
+        , tileID(std::move(tileID_))
     {}
 
     size_t index;
@@ -49,9 +45,7 @@ class IndexedSubfeature {
 
     // Only used for symbol features
     std::string sourceID;
-    uint8_t z;
-    uint32_t x;
-    uint32_t y;
+    CanonicalTileID tileID;
 };
 
 class FeatureIndex {

src/mbgl/gl/context.hpp

@@ -61,7 +61,7 @@ class Context : private util::noncopyable {
     optional<std::pair<BinaryProgramFormat, std::string>> getBinaryProgram(ProgramID) const;
 
     template <class Vertex, class DrawMode>
-    VertexBuffer<Vertex, DrawMode> createVertexBuffer(VertexVector<Vertex, DrawMode>&& v, const BufferUsage usage=BufferUsage::StaticDraw) {
+    VertexBuffer<Vertex, DrawMode> createVertexBuffer(VertexVector<Vertex, DrawMode>&& v, const BufferUsage usage = BufferUsage::StaticDraw) {
         return VertexBuffer<Vertex, DrawMode> {
             v.vertexSize(),
             createVertexBuffer(v.data(), v.byteSize(), usage)
@@ -75,7 +75,7 @@ class Context : private util::noncopyable {
     }
 
     template <class DrawMode>
-    IndexBuffer<DrawMode> createIndexBuffer(IndexVector<DrawMode>&& v, const BufferUsage usage=BufferUsage::StaticDraw) {
+    IndexBuffer<DrawMode> createIndexBuffer(IndexVector<DrawMode>&& v, const BufferUsage usage = BufferUsage::StaticDraw) {
         return IndexBuffer<DrawMode> {
             v.indexSize(),
             createIndexBuffer(v.data(), v.byteSize(), usage)

src/mbgl/layout/symbol_instance.cpp

@@ -32,7 +32,7 @@ SymbolInstance::SymbolInstance(Anchor& anchor_,
 
     // Create the collision features that will be used to check whether this symbol instance can be placed
     textCollisionFeature(line_, anchor, shapedTextOrientations.first, textBoxScale, textPadding, textPlacement, indexedFeature, overscaling),
-    iconCollisionFeature(line_, anchor, shapedIcon, iconBoxScale, iconPadding, SymbolPlacementType::Point, indexedFeature),
+    iconCollisionFeature(line_, anchor, shapedIcon, iconBoxScale, iconPadding, indexedFeature),
     featureIndex(featureIndex_),
     textOffset(textOffset_),
     iconOffset(iconOffset_),

src/mbgl/layout/symbol_layout.cpp

@@ -293,7 +293,7 @@ void SymbolLayout::addFeature(const std::size_t index,
 
     const float textRepeatDistance = symbolSpacing / 2;
     IndexedSubfeature indexedFeature(feature.index, sourceLayer->getName(), bucketName, symbolInstances.size(),
-                                     sourceID, tileID.canonical.z, tileID.canonical.x, tileID.canonical.y);
+                                     sourceID, tileID.canonical);
 
     auto addSymbolInstance = [&] (const GeometryCoordinates& line, Anchor& anchor) {
         // https://github.com/mapbox/vector-tile-spec/tree/master/2.1#41-layers
@@ -320,7 +320,7 @@ void SymbolLayout::addFeature(const std::size_t index,
                     symbolInstances.size(),
                     textBoxScale, textPadding, textPlacement, textOffset,
                     iconBoxScale, iconPadding, iconOffset,
-                    glyphPositionMap, indexedFeature, index, feature.text ? *feature.text : std::u16string{}, overscaling);
+                    glyphPositionMap, indexedFeature, index, feature.text.value_or(std::u16string()), overscaling);
         }
     };
 

src/mbgl/layout/symbol_projection.cpp

@@ -294,7 +294,7 @@ namespace mbgl {
         std::vector<PlacedGlyph> placedGlyphs;
         if (symbol.glyphOffsets.size() > 1) {
 
-            const optional<std::pair<PlacedGlyph,PlacedGlyph>> firstAndLastGlyph =
+            const optional<std::pair<PlacedGlyph, PlacedGlyph>> firstAndLastGlyph =
                 placeFirstAndLastGlyph(fontScale, lineOffsetX, lineOffsetY, flip, projectedAnchorPoint, symbol.anchorPoint, symbol, labelPlaneMatrix, false);
             if (!firstAndLastGlyph) {
                 return PlacementResult::NotEnoughRoom;

src/mbgl/programs/collision_box_program.hpp

@@ -56,51 +56,50 @@ class CollisionBoxProgram : public Program<
     }
 
 	template <class DrawMode>
-		void draw(gl::Context& context,
-				DrawMode drawMode,
-				gl::DepthMode depthMode,
-				gl::StencilMode stencilMode,
-				gl::ColorMode colorMode,
-				const UniformValues& uniformValues,
-				const gl::VertexBuffer<CollisionBoxLayoutAttributes::Vertex>& layoutVertexBuffer,
-                const gl::VertexBuffer<CollisionBoxDynamicAttributes::Vertex>& dynamicVertexBuffer,
-				const gl::IndexBuffer<DrawMode>& indexBuffer,
-				const SegmentVector<Attributes>& segments,
-				const PaintPropertyBinders& paintPropertyBinders,
-				const typename PaintProperties::PossiblyEvaluated& currentProperties,
-				float currentZoom,
-				const std::string& layerID) {
-			typename AllUniforms::Values allUniformValues = uniformValues
-				.concat(paintPropertyBinders.uniformValues(currentZoom, currentProperties));
-
-			typename Attributes::Bindings allAttributeBindings = CollisionBoxLayoutAttributes::bindings(layoutVertexBuffer)
-                .concat(CollisionBoxDynamicAttributes::bindings(dynamicVertexBuffer))
-				.concat(paintPropertyBinders.attributeBindings(currentProperties));
-
-            assert(layoutVertexBuffer.vertexCount == dynamicVertexBuffer.vertexCount);
-
-			for (auto& segment : segments) {
-				auto vertexArrayIt = segment.vertexArrays.find(layerID);
-
-				if (vertexArrayIt == segment.vertexArrays.end()) {
-					vertexArrayIt = segment.vertexArrays.emplace(layerID, context.createVertexArray()).first;
-				}
-
-				program.draw(
-						context,
-						std::move(drawMode),
-						std::move(depthMode),
-						std::move(stencilMode),
-						std::move(colorMode),
-						allUniformValues,
-						vertexArrayIt->second,
-						Attributes::offsetBindings(allAttributeBindings, segment.vertexOffset),
-						indexBuffer,
-						segment.indexOffset,
-						segment.indexLength);
-			}
-		}
+    void draw(gl::Context& context,
+              DrawMode drawMode,
+              gl::DepthMode depthMode,
+              gl::StencilMode stencilMode,
+              gl::ColorMode colorMode,
+              const UniformValues& uniformValues,
+              const gl::VertexBuffer<CollisionBoxLayoutAttributes::Vertex>& layoutVertexBuffer,
+              const gl::VertexBuffer<CollisionBoxDynamicAttributes::Vertex>& dynamicVertexBuffer,
+              const gl::IndexBuffer<DrawMode>& indexBuffer,
+              const SegmentVector<Attributes>& segments,
+              const PaintPropertyBinders& paintPropertyBinders,
+              const typename PaintProperties::PossiblyEvaluated& currentProperties,
+              float currentZoom,
+              const std::string& layerID) {
+        typename AllUniforms::Values allUniformValues = uniformValues
+            .concat(paintPropertyBinders.uniformValues(currentZoom, currentProperties));
+
+        typename Attributes::Bindings allAttributeBindings = CollisionBoxLayoutAttributes::bindings(layoutVertexBuffer)
+            .concat(CollisionBoxDynamicAttributes::bindings(dynamicVertexBuffer))
+            .concat(paintPropertyBinders.attributeBindings(currentProperties));
+
+        assert(layoutVertexBuffer.vertexCount == dynamicVertexBuffer.vertexCount);
+
+        for (auto& segment : segments) {
+            auto vertexArrayIt = segment.vertexArrays.find(layerID);
+
+            if (vertexArrayIt == segment.vertexArrays.end()) {
+                vertexArrayIt = segment.vertexArrays.emplace(layerID, context.createVertexArray()).first;
+            }
 
+            program.draw(
+                    context,
+                    std::move(drawMode),
+                    std::move(depthMode),
+                    std::move(stencilMode),
+                    std::move(colorMode),
+                    allUniformValues,
+                    vertexArrayIt->second,
+                    Attributes::offsetBindings(allAttributeBindings, segment.vertexOffset),
+                    indexBuffer,
+                    segment.indexOffset,
+                    segment.indexLength);
+        }
+    }
 };
 
 
@@ -135,49 +134,48 @@ class CollisionCircleProgram : public Program<
     }
 
     template <class DrawMode>
-        void draw(gl::Context& context,
-                DrawMode drawMode,
-                gl::DepthMode depthMode,
-                gl::StencilMode stencilMode,
-                gl::ColorMode colorMode,
-                const UniformValues& uniformValues,
-                const gl::VertexBuffer<CollisionBoxLayoutAttributes::Vertex>& layoutVertexBuffer,
-                const gl::VertexBuffer<CollisionBoxDynamicAttributes::Vertex>& dynamicVertexBuffer,
-                const gl::IndexBuffer<DrawMode>& indexBuffer,
-                const SegmentVector<Attributes>& segments,
-                const PaintPropertyBinders& paintPropertyBinders,
-                const typename PaintProperties::PossiblyEvaluated& currentProperties,
-                float currentZoom,
-                const std::string& layerID) {
-            typename AllUniforms::Values allUniformValues = uniformValues
-                .concat(paintPropertyBinders.uniformValues(currentZoom, currentProperties));
-
-            typename Attributes::Bindings allAttributeBindings = CollisionBoxLayoutAttributes::bindings(layoutVertexBuffer)
-                .concat(CollisionBoxDynamicAttributes::bindings(dynamicVertexBuffer))
-                .concat(paintPropertyBinders.attributeBindings(currentProperties));
-
-            for (auto& segment : segments) {
-                auto vertexArrayIt = segment.vertexArrays.find(layerID);
-
-                if (vertexArrayIt == segment.vertexArrays.end()) {
-                    vertexArrayIt = segment.vertexArrays.emplace(layerID, context.createVertexArray()).first;
-                }
-
-                program.draw(
-                        context,
-                        std::move(drawMode),
-                        std::move(depthMode),
-                        std::move(stencilMode),
-                        std::move(colorMode),
-                        allUniformValues,
-                        vertexArrayIt->second,
-                        Attributes::offsetBindings(allAttributeBindings, segment.vertexOffset),
-                        indexBuffer,
-                        segment.indexOffset,
-                        segment.indexLength);
+    void draw(gl::Context& context,
+              DrawMode drawMode,
+              gl::DepthMode depthMode,
+              gl::StencilMode stencilMode,
+              gl::ColorMode colorMode,
+              const UniformValues& uniformValues,
+              const gl::VertexBuffer<CollisionBoxLayoutAttributes::Vertex>& layoutVertexBuffer,
+              const gl::VertexBuffer<CollisionBoxDynamicAttributes::Vertex>& dynamicVertexBuffer,
+              const gl::IndexBuffer<DrawMode>& indexBuffer,
+              const SegmentVector<Attributes>& segments,
+              const PaintPropertyBinders& paintPropertyBinders,
+              const typename PaintProperties::PossiblyEvaluated& currentProperties,
+              float currentZoom,
+              const std::string& layerID) {
+        typename AllUniforms::Values allUniformValues = uniformValues
+            .concat(paintPropertyBinders.uniformValues(currentZoom, currentProperties));
+
+        typename Attributes::Bindings allAttributeBindings = CollisionBoxLayoutAttributes::bindings(layoutVertexBuffer)
+            .concat(CollisionBoxDynamicAttributes::bindings(dynamicVertexBuffer))
+            .concat(paintPropertyBinders.attributeBindings(currentProperties));
+
+        for (auto& segment : segments) {
+            auto vertexArrayIt = segment.vertexArrays.find(layerID);
+
+            if (vertexArrayIt == segment.vertexArrays.end()) {
+                vertexArrayIt = segment.vertexArrays.emplace(layerID, context.createVertexArray()).first;
             }
-        }
 
+            program.draw(
+                    context,
+                    std::move(drawMode),
+                    std::move(depthMode),
+                    std::move(stencilMode),
+                    std::move(colorMode),
+                    allUniformValues,
+                    vertexArrayIt->second,
+                    Attributes::offsetBindings(allAttributeBindings, segment.vertexOffset),
+                    indexBuffer,
+                    segment.indexOffset,
+                    segment.indexLength);
+        }
+    }
 };
 
 using CollisionBoxVertex = CollisionBoxProgram::LayoutVertex;

src/mbgl/renderer/buckets/symbol_bucket.cpp

@@ -179,6 +179,7 @@ void SymbolBucket::sortFeatures(const float angle) {
 
     // To avoid sorting the actual symbolInstance array we sort an array of indexes.
     std::vector<size_t> symbolInstanceIndexes;
+    symbolInstanceIndexes.reserve(symbolInstances.size());
     for (size_t i = 0; i < symbolInstances.size(); i++) {
         symbolInstanceIndexes.push_back(i);
     }

src/mbgl/renderer/render_layer.hpp

@@ -81,10 +81,14 @@ class RenderLayer {
 
     friend std::string layoutKey(const RenderLayer&);
 
+protected:
+    // renderTiles are exposed directly to CrossTileSymbolIndex and Placement so they
+    // can update opacities in the symbol buckets immediately before rendering
+    friend class CrossTileSymbolIndex;
+    friend class Placement;
     // Stores current set of tiles to be rendered for this layer.
     std::vector<std::reference_wrapper<RenderTile>> renderTiles;
 
-protected:
     // Stores what render passes this layer is currently enabled for. This depends on the
     // evaluated StyleProperties object and is updated accordingly.
     RenderPass passes = RenderPass::None;

src/mbgl/renderer/renderer_impl.cpp

@@ -23,7 +23,6 @@
 #include <mbgl/style/source_impl.hpp>
 #include <mbgl/style/transition_options.hpp>
 #include <mbgl/text/glyph_manager.hpp>
-#include <mbgl/text/cross_tile_symbol_index.hpp>
 #include <mbgl/tile/tile.hpp>
 #include <mbgl/util/math.hpp>
 #include <mbgl/util/string.hpp>
@@ -58,7 +57,6 @@ Renderer::Impl::Impl(RendererBackend& backend_,
     , sourceImpls(makeMutable<std::vector<Immutable<style::Source::Impl>>>())
     , layerImpls(makeMutable<std::vector<Immutable<style::Layer::Impl>>>())
     , renderLight(makeMutable<Light::Impl>())
-    , crossTileSymbolIndex(std::make_unique<CrossTileSymbolIndex>())
     , placement(std::make_unique<Placement>(TransformState{}, MapMode::Static)) {
     glyphManager->setObserver(this);
 }
@@ -368,11 +366,11 @@ void Renderer::Impl::render(const UpdateParameters& updateParameters) {
     bool symbolBucketsChanged = false;
     if (parameters.mapMode != MapMode::Continuous) {
         // TODO: Think about right way for symbol index to handle still rendering
-        crossTileSymbolIndex->reset();
+        crossTileSymbolIndex.reset();
     }
     for (auto it = order.rbegin(); it != order.rend(); ++it) {
         if (it->layer.is<RenderSymbolLayer>()) {
-            if (crossTileSymbolIndex->addLayer(*it->layer.as<RenderSymbolLayer>())) symbolBucketsChanged = true;
+            if (crossTileSymbolIndex.addLayer(*it->layer.as<RenderSymbolLayer>())) symbolBucketsChanged = true;
         }
     }
 
@@ -622,7 +620,7 @@ void Renderer::Impl::render(const UpdateParameters& updateParameters) {
 
     observer->onDidFinishRenderingFrame(
         loaded ? RendererObserver::RenderMode::Full : RendererObserver::RenderMode::Partial,
-        updateParameters.mode == MapMode::Continuous && (hasTransitions(parameters.timePoint))
+        updateParameters.mode == MapMode::Continuous && hasTransitions(parameters.timePoint)
     );
 
     if (!loaded) {

src/mbgl/renderer/renderer_impl.hpp

@@ -9,6 +9,7 @@
 #include <mbgl/style/layer.hpp>
 #include <mbgl/map/transform_state.hpp>
 #include <mbgl/map/zoom_history.hpp>
+#include <mbgl/text/cross_tile_symbol_index.hpp>
 #include <mbgl/text/glyph_manager_observer.hpp>
 #include <mbgl/text/placement.hpp>
 
@@ -105,7 +106,7 @@ class Renderer::Impl : public GlyphManagerObserver,
     std::unordered_map<std::string, std::unique_ptr<RenderLayer>> renderLayers;
     RenderLight renderLight;
 
-    std::unique_ptr<CrossTileSymbolIndex> crossTileSymbolIndex;
+    CrossTileSymbolIndex crossTileSymbolIndex;
     std::unique_ptr<Placement> placement;
 
     bool contextLost = false;

src/mbgl/text/collision_feature.hpp

@@ -56,19 +56,26 @@ class CollisionFeature {
         : CollisionFeature(line, anchor, shapedText.top, shapedText.bottom, shapedText.left, shapedText.right, boxScale, padding, placement, indexedFeature_, overscaling) {}
 
     // for icons
+    // Icons collision features are always SymbolPlacementType::Point, which means the collision feature
+    // will be viewport-rotation-aligned even if the icon is map-rotation-aligned (e.g. `icon-rotation-alignment: map`
+    // _or_ `symbol-placement: line`). We're relying on most icons being "close enough" to square that having
+    // incorrect rotation alignment doesn't throw off collision detection too much.
+    // See: https://github.com/mapbox/mapbox-gl-js/issues/4861
     CollisionFeature(const GeometryCoordinates& line,
                      const Anchor& anchor,
                      optional<PositionedIcon> shapedIcon,
                      const float boxScale,
                      const float padding,
-                     const style::SymbolPlacementType placement,
                      const IndexedSubfeature& indexedFeature_)
         : CollisionFeature(line, anchor,
                            (shapedIcon ? shapedIcon->top() : 0),
                            (shapedIcon ? shapedIcon->bottom() : 0),
                            (shapedIcon ? shapedIcon->left() : 0),
                            (shapedIcon ? shapedIcon->right() : 0),
-                           boxScale, padding, placement, indexedFeature_, 1) {}
+                           boxScale,
+                           padding,
+                           style::SymbolPlacementType::Point,
+                           indexedFeature_, 1) {}
 
     CollisionFeature(const GeometryCoordinates& line,
                      const Anchor&,