[core] Cleanup in response to review comments.

3 files changed, 110 insertions, 98 deletions

diff --git a/src/mbgl/text/collision_feature.hpp b/src/mbgl/text/collision_feature.hpp
index 4ecd25c717..df1b12819c 100644
--- a/src/mbgl/text/collision_feature.hpp
+++ b/src/mbgl/text/collision_feature.hpp
@@ -56,19 +56,26 @@ public:
         : 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&,
diff --git a/src/mbgl/text/collision_index.cpp b/src/mbgl/text/collision_index.cpp
index 72a2d1af4e..fee28b5873 100644
--- a/src/mbgl/text/collision_index.cpp
+++ b/src/mbgl/text/collision_index.cpp
@@ -241,6 +241,25 @@ void CollisionIndex::insertFeature(CollisionFeature& feature, bool ignorePlaceme
     }
 }
 
+bool polygonIntersectsBox(const LineString<float>& polygon, const GridIndex<IndexedSubfeature>::BBox& bbox) {
+    // This is just a wrapper that allows us to use the integer-based util::polygonIntersectsPolygon
+    // Conversion limits our query accuracy to single-pixel resolution
+    GeometryCoordinates integerPolygon;
+    for (const auto& point : polygon) {
+        integerPolygon.push_back(convertPoint<int16_t>(point));
+    }
+    int16_t minX1 = bbox.min.x;
+    int16_t maxY1 = bbox.max.y;
+    int16_t minY1 = bbox.min.y;
+    int16_t maxX1 = bbox.max.x;
+
+    auto bboxPoints = GeometryCoordinates {
+        { minX1, minY1 }, { maxX1, minY1 }, { maxX1, maxY1 }, { minX1, maxY1 }
+    };
+    
+    return util::polygonIntersectsPolygon(integerPolygon, bboxPoints);
+}
+
 std::vector<IndexedSubfeature> CollisionIndex::queryRenderedSymbols(const GeometryCoordinates& queryGeometry, const UnwrappedTileID& tileID, const std::string& sourceID) const {
     std::vector<IndexedSubfeature> result;
     if (queryGeometry.empty() || (collisionGrid.empty() && ignoredGrid.empty())) {
@@ -253,14 +272,10 @@ std::vector<IndexedSubfeature> CollisionIndex::queryRenderedSymbols(const Geomet
     transformState.matrixFor(posMatrix, tileID);
     matrix::multiply(posMatrix, projMatrix, posMatrix);
 
-    // Two versions of the query here just because util::polygonIntersectsPolygon requires
-    // GeometryCoordinates (based on int16_t). Otherwise, work with floats.
-    GeometryCoordinates projectedPolygon;
+    // queryGeometry is specified in integer tile units, but in projecting we switch to float pixels
     LineString<float> projectedQuery;
-
     for (const auto& point : queryGeometry) {
         auto projected = projectPoint(posMatrix, convertPoint<float>(point));
-        projectedPolygon.push_back(convertPoint<int16_t>(projected));
         projectedQuery.push_back(projected);
     }
     
@@ -273,8 +288,7 @@ std::vector<IndexedSubfeature> CollisionIndex::queryRenderedSymbols(const Geomet
 
     for (auto& queryResult : features) {
         auto& feature = queryResult.first;
-        CanonicalTileID featureTileID(feature.z, feature.x, feature.y);
-        if (feature.sourceID == sourceID && featureTileID == tileID.canonical) {
+        if (feature.sourceID == sourceID && feature.tileID == tileID.canonical) {
             // We only have to filter on the canonical ID because even if the feature is showing multiple times
             // we treat it as one feature.
             thisTileFeatures.push_back(queryResult);
@@ -284,8 +298,7 @@ std::vector<IndexedSubfeature> CollisionIndex::queryRenderedSymbols(const Geomet
     std::vector<QueryResult> ignoredFeatures = ignoredGrid.queryWithBoxes(envelope);
     for (auto& queryResult : ignoredFeatures) {
         auto& feature = queryResult.first;
-        CanonicalTileID featureTileID(feature.z, feature.x, feature.y);
-        if (feature.sourceID == sourceID && featureTileID == tileID.canonical) {
+        if (feature.sourceID == sourceID && feature.tileID == tileID.canonical) {
             thisTileFeatures.push_back(queryResult);
         }
     }
@@ -301,18 +314,10 @@ std::vector<IndexedSubfeature> CollisionIndex::queryRenderedSymbols(const Geomet
             continue;
 
         seenFeatures.insert(feature.index);
-
-        int16_t minX1 = bbox.min.x;
-        int16_t maxY1 = bbox.max.y;
-        int16_t minY1 = bbox.min.y;
-        int16_t maxX1 = bbox.max.x;
-
-        auto bboxPoints = GeometryCoordinates {
-            { minX1, minY1 }, { maxX1, minY1 }, { maxX1, maxY1 }, { minX1, maxY1 }
-        };
         
-        if (!util::polygonIntersectsPolygon(projectedPolygon, bboxPoints))
+        if (!polygonIntersectsBox(projectedQuery, bbox)) {
             continue;
+        }
 
         result.push_back(feature);
     }
diff --git a/src/mbgl/text/placement.hpp b/src/mbgl/text/placement.hpp
index 4d5fad06a1..c6d1f259a5 100644
--- a/src/mbgl/text/placement.hpp
+++ b/src/mbgl/text/placement.hpp
@@ -9,82 +9,82 @@
 
 namespace mbgl {
 
-    class RenderSymbolLayer;
-    class SymbolBucket;
-
-    class OpacityState {
-        public:
-            OpacityState(bool placed, bool offscreen);
-            OpacityState(const OpacityState& prevOpacityState, float increment, bool placed);
-            bool isHidden() const;
-            float opacity;
-            bool placed;
-    };
-
-    class JointOpacityState {
-        public:
-            JointOpacityState(bool placedIcon, bool placedText, bool offscreen);
-            JointOpacityState(const JointOpacityState& prevOpacityState, float increment, bool placedIcon, bool placedText);
-            bool isHidden() const;
-            OpacityState icon;
-            OpacityState text;
-    };
-
-    class JointPlacement {
-        public:
-            JointPlacement(bool text_, bool icon_, bool offscreen_)
-                : text(text_), icon(icon_), offscreen(offscreen_)
-            {}
-        
-            const bool text;
-            const bool icon;
-            // offscreen = outside viewport, but within CollisionIndex::viewportPadding px of the edge
-            // Because these symbols aren't onscreen yet, we can skip the "fade in" animation,
-            // and if a subsequent viewport change brings them into view, they'll be fully
-            // visible right away.
-            const bool offscreen;
-    };
-
-    class Placement {
-        public:
-            Placement(const TransformState&, MapMode mapMode);
-            void placeLayer(RenderSymbolLayer&, const mat4&, bool showCollisionBoxes);
-            bool commit(const Placement& prevPlacement, TimePoint);
-            void updateLayerOpacities(RenderSymbolLayer&);
-            JointOpacityState getOpacity(uint32_t crossTileSymbolID) const;
-            float symbolFadeChange(TimePoint now) const;
-            bool hasTransitions(TimePoint now) const;
-
-            const CollisionIndex& getCollisionIndex() const;
-        
-            bool stillRecent(TimePoint now) const;
-            void setRecent(TimePoint now);
-            void setStale();
-        private:
-
-            void placeLayerBucket(
-                    SymbolBucket&,
-                    const mat4& posMatrix,
-                    const mat4& textLabelPlaneMatrix,
-                    const mat4& iconLabelPlaneMatrix,
-                    const float scale,
-                    const float pixelRatio,
-                    const bool showCollisionBoxes,
-                    std::unordered_set<uint32_t>& seenCrossTileIDs);
-
-            void updateBucketOpacities(SymbolBucket&, std::set<uint32_t>&);
-        
-            CollisionIndex collisionIndex;
-
-            TransformState state;
-            MapMode mapMode;
-            TimePoint commitTime;
-
-            std::unordered_map<uint32_t, JointPlacement> placements;
-            std::unordered_map<uint32_t, JointOpacityState> opacities;
-        
-            TimePoint recentUntil;
-            bool stale = false;
-    };
+class RenderSymbolLayer;
+class SymbolBucket;
+
+class OpacityState {
+public:
+    OpacityState(bool placed, bool offscreen);
+    OpacityState(const OpacityState& prevOpacityState, float increment, bool placed);
+    bool isHidden() const;
+    float opacity;
+    bool placed;
+};
+
+class JointOpacityState {
+public:
+    JointOpacityState(bool placedIcon, bool placedText, bool offscreen);
+    JointOpacityState(const JointOpacityState& prevOpacityState, float increment, bool placedIcon, bool placedText);
+    bool isHidden() const;
+    OpacityState icon;
+    OpacityState text;
+};
+
+class JointPlacement {
+public:
+    JointPlacement(bool text_, bool icon_, bool offscreen_)
+        : text(text_), icon(icon_), offscreen(offscreen_)
+    {}
+
+    const bool text;
+    const bool icon;
+    // offscreen = outside viewport, but within CollisionIndex::viewportPadding px of the edge
+    // Because these symbols aren't onscreen yet, we can skip the "fade in" animation,
+    // and if a subsequent viewport change brings them into view, they'll be fully
+    // visible right away.
+    const bool offscreen;
+};
+
+class Placement {
+public:
+    Placement(const TransformState&, MapMode mapMode);
+    void placeLayer(RenderSymbolLayer&, const mat4&, bool showCollisionBoxes);
+    bool commit(const Placement& prevPlacement, TimePoint);
+    void updateLayerOpacities(RenderSymbolLayer&);
+    JointOpacityState getOpacity(uint32_t crossTileSymbolID) const;
+    float symbolFadeChange(TimePoint now) const;
+    bool hasTransitions(TimePoint now) const;
+
+    const CollisionIndex& getCollisionIndex() const;
+
+    bool stillRecent(TimePoint now) const;
+    void setRecent(TimePoint now);
+    void setStale();
+private:
+
+    void placeLayerBucket(
+            SymbolBucket&,
+            const mat4& posMatrix,
+            const mat4& textLabelPlaneMatrix,
+            const mat4& iconLabelPlaneMatrix,
+            const float scale,
+            const float pixelRatio,
+            const bool showCollisionBoxes,
+            std::unordered_set<uint32_t>& seenCrossTileIDs);
+
+    void updateBucketOpacities(SymbolBucket&, std::set<uint32_t>&);
+
+    CollisionIndex collisionIndex;
+
+    TransformState state;
+    MapMode mapMode;
+    TimePoint commitTime;
+
+    std::unordered_map<uint32_t, JointPlacement> placements;
+    std::unordered_map<uint32_t, JointOpacityState> opacities;
+
+    TimePoint recentUntil;
+    bool stale = false;
+};
 
 } // namespace mbgl