[core] Safeguard PositionedIcon usage via optional

11 files changed, 82 insertions, 62 deletions

diff --git a/src/mbgl/layout/symbol_instance.cpp b/src/mbgl/layout/symbol_instance.cpp
index 2a3bf068c1..8816f4c95c 100644
--- a/src/mbgl/layout/symbol_instance.cpp
+++ b/src/mbgl/layout/symbol_instance.cpp
@@ -8,7 +8,7 @@ using namespace style;
 SymbolInstance::SymbolInstance(Anchor& anchor,
                                const GeometryCoordinates& line,
                                const std::pair<Shaping, Shaping>& shapedTextOrientations,
-                               const PositionedIcon& shapedIcon,
+                               optional<PositionedIcon> shapedIcon,
                                const SymbolLayoutProperties::Evaluated& layout,
                                const float layoutTextSize,
                                const bool addToBuffers,
@@ -27,18 +27,15 @@ SymbolInstance::SymbolInstance(Anchor& anchor,
     hasText(shapedTextOrientations.first || shapedTextOrientations.second),
     hasIcon(shapedIcon),
 
-
     // Create the collision features that will be used to check whether this symbol instance can be placed
     textCollisionFeature(line, anchor, shapedTextOrientations.second ?: shapedTextOrientations.first, textBoxScale, textPadding, textPlacement, indexedFeature),
     iconCollisionFeature(line, anchor, shapedIcon, iconBoxScale, iconPadding, iconPlacement, indexedFeature),
     featureIndex(featureIndex_) {
-        
-    
 
     // Create the quads used for rendering the icon and glyphs.
     if (addToBuffers) {
         if (shapedIcon) {
-            iconQuad = getIconQuad(anchor, shapedIcon, line, layout, layoutTextSize, iconPlacement, shapedTextOrientations.first);
+            iconQuad = getIconQuad(anchor, *shapedIcon, line, layout, layoutTextSize, iconPlacement, shapedTextOrientations.first);
         }
         if (shapedTextOrientations.first) {
             auto quads = getGlyphQuads(anchor, shapedTextOrientations.first, textBoxScale, line, layout, textPlacement, face);
@@ -59,8 +56,6 @@ SymbolInstance::SymbolInstance(Anchor& anchor,
     } else {
         writingModes = WritingModeType::None;
     }
-    
-    
 }
 
 } // namespace mbgl
diff --git a/src/mbgl/layout/symbol_instance.hpp b/src/mbgl/layout/symbol_instance.hpp
index efe6cb05aa..eadbf67475 100644
--- a/src/mbgl/layout/symbol_instance.hpp
+++ b/src/mbgl/layout/symbol_instance.hpp
@@ -15,7 +15,7 @@ public:
     SymbolInstance(Anchor& anchor,
                    const GeometryCoordinates& line,
                    const std::pair<Shaping, Shaping>& shapedTextOrientations,
-                   const PositionedIcon& shapedIcon,
+                   optional<PositionedIcon> shapedIcon,
                    const style::SymbolLayoutProperties::Evaluated&,
                    const float layoutTextSize,
                    const bool inside,
diff --git a/src/mbgl/layout/symbol_layout.cpp b/src/mbgl/layout/symbol_layout.cpp
index 907e60a598..0b05d4f5e1 100644
--- a/src/mbgl/layout/symbol_layout.cpp
+++ b/src/mbgl/layout/symbol_layout.cpp
@@ -221,7 +221,7 @@ void SymbolLayout::prepare(const GlyphPositionMap& glyphs, const IconAtlasMap& i
         if (feature.geometry.empty()) continue;
 
         std::pair<Shaping, Shaping> shapedTextOrientations;
-        PositionedIcon shapedIcon;
+        optional<PositionedIcon> shapedIcon;
         GlyphPositions face;
 
         // if feature has text, shape the text
@@ -262,7 +262,7 @@ void SymbolLayout::prepare(const GlyphPositionMap& glyphs, const IconAtlasMap& i
             if (icons != iconMap.end()) {
                 auto image = icons->second.find(*feature.icon);
                 if (image != icons->second.end()) {
-                    shapedIcon = shapeIcon(image->second,
+                    shapedIcon = PositionedIcon::shapeIcon(image->second,
                         layout.evaluate<IconOffset>(zoom, feature),
                         layout.evaluate<IconRotate>(zoom, feature) * util::DEG2RAD);
                     if (image->second.sdf) {
@@ -292,7 +292,7 @@ void SymbolLayout::prepare(const GlyphPositionMap& glyphs, const IconAtlasMap& i
 void SymbolLayout::addFeature(const std::size_t index,
                               const SymbolFeature& feature,
                               const std::pair<Shaping, Shaping>& shapedTextOrientations,
-                              const PositionedIcon& shapedIcon,
+                              optional<PositionedIcon> shapedIcon,
                               const GlyphPositions& glyphs) {
     const float minScale = 0.5f;
     const float glyphSize = 24.0f;
@@ -363,8 +363,8 @@ void SymbolLayout::addFeature(const std::size_t index,
                                          textMaxAngle,
                                          (shapedTextOrientations.second ?: shapedTextOrientations.first).left,
                                          (shapedTextOrientations.second ?: shapedTextOrientations.first).right,
-                                         shapedIcon.left,
-                                         shapedIcon.right,
+                                         (shapedIcon ? shapedIcon->left() : 0),
+                                         (shapedIcon ? shapedIcon->right() : 0),
                                          glyphSize,
                                          textMaxBoxScale,
                                          overscaling);
diff --git a/src/mbgl/layout/symbol_layout.hpp b/src/mbgl/layout/symbol_layout.hpp
index 4ec84f0f58..f49f93eaf1 100644
--- a/src/mbgl/layout/symbol_layout.hpp
+++ b/src/mbgl/layout/symbol_layout.hpp
@@ -55,7 +55,7 @@ private:
     void addFeature(const size_t,
                     const SymbolFeature&,
                     const std::pair<Shaping, Shaping>& shapedTextOrientations,
-                    const PositionedIcon& shapedIcon,
+                    optional<PositionedIcon> shapedIcon,
                     const GlyphPositions& face);
 
     bool anchorIsTooClose(const std::u16string& text, const float repeatDistance, const Anchor&);
diff --git a/src/mbgl/text/collision_feature.cpp b/src/mbgl/text/collision_feature.cpp
index 67f669ab61..885ba5c426 100644
--- a/src/mbgl/text/collision_feature.cpp
+++ b/src/mbgl/text/collision_feature.cpp
@@ -13,7 +13,7 @@ CollisionFeature::CollisionFeature(const GeometryCoordinates& line,
                                    const float padding,
                                    const style::SymbolPlacementType placement,
                                    IndexedSubfeature indexedFeature_,
-                                   const bool straight)
+                                   const AlignmentType alignment)
         : indexedFeature(std::move(indexedFeature_)) {
     if (top == 0 && bottom == 0 && left == 0 && right == 0) return;
 
@@ -32,7 +32,7 @@ CollisionFeature::CollisionFeature(const GeometryCoordinates& line,
 
         GeometryCoordinate anchorPoint = convertPoint<int16_t>(anchor.point);
 
-        if (straight) {
+        if (alignment == AlignmentType::Straight) {
             // used for icon labels that are aligned with the line, but don't curve along it
             const GeometryCoordinate vector = convertPoint<int16_t>(util::unit(convertPoint<double>(line[anchor.segment + 1] - line[anchor.segment])) * length);
             const GeometryCoordinates newLine({ anchorPoint - vector, anchorPoint + vector });
diff --git a/src/mbgl/text/collision_feature.hpp b/src/mbgl/text/collision_feature.hpp
index 1436e0815b..006a47eb74 100644
--- a/src/mbgl/text/collision_feature.hpp
+++ b/src/mbgl/text/collision_feature.hpp
@@ -33,6 +33,11 @@ public:
 
 class CollisionFeature {
 public:
+    enum class AlignmentType : bool {
+        Straight = 0,
+        Curved
+    };
+
     // for text
     CollisionFeature(const GeometryCoordinates& line,
                      const Anchor& anchor,
@@ -41,29 +46,34 @@ public:
                      const float padding,
                      const style::SymbolPlacementType placement,
                      const IndexedSubfeature& indexedFeature_)
-        : CollisionFeature(line, anchor, shapedText.top, shapedText.bottom, shapedText.left, shapedText.right, boxScale, padding, placement, indexedFeature_, false) {}
+        : CollisionFeature(line, anchor, shapedText.top, shapedText.bottom, shapedText.left, shapedText.right, boxScale, padding, placement, indexedFeature_, AlignmentType::Curved) {}
 
     // for icons
     CollisionFeature(const GeometryCoordinates& line,
                      const Anchor& anchor,
-                     const PositionedIcon& shapedIcon,
+                     optional<PositionedIcon> shapedIcon,
                      const float boxScale,
                      const float padding,
                      const style::SymbolPlacementType placement,
                      const IndexedSubfeature& indexedFeature_)
-        : CollisionFeature(line, anchor, shapedIcon.top, shapedIcon.bottom, shapedIcon.left, shapedIcon.right, boxScale, padding, placement, indexedFeature_, true) {}
+        : CollisionFeature(line, anchor,
+                           (shapedIcon ? shapedIcon->top() : 0),
+                           (shapedIcon ? shapedIcon->bottom() : 0),
+                           (shapedIcon ? shapedIcon->left() : 0),
+                           (shapedIcon ? shapedIcon->right() : 0),
+                           boxScale, padding, placement, indexedFeature_, AlignmentType::Straight) {}
 
     CollisionFeature(const GeometryCoordinates& line,
-                     const Anchor& anchor,
+                     const Anchor&,
                      const float top,
                      const float bottom,
                      const float left,
                      const float right,
                      const float boxScale,
                      const float padding,
-                     const style::SymbolPlacementType placement,
+                     const style::SymbolPlacementType,
                      IndexedSubfeature,
-                     const bool straight);
+                     const AlignmentType);
 
     std::vector<CollisionBox> boxes;
     IndexedSubfeature indexedFeature;
diff --git a/src/mbgl/text/quads.cpp b/src/mbgl/text/quads.cpp
index 188f88655d..bf7791581a 100644
--- a/src/mbgl/text/quads.cpp
+++ b/src/mbgl/text/quads.cpp
@@ -22,12 +22,12 @@ SymbolQuad getIconQuad(const Anchor& anchor,
                        const float layoutTextSize,
                        const style::SymbolPlacementType placement, 
                        const Shaping& shapedText) {
-    auto image = *(shapedIcon.image);
+    auto image = *shapedIcon.image();
 
     const float border = 1.0;
-    auto left = shapedIcon.left - border;
+    auto left = shapedIcon.left() - border;
     auto right = left + image.pos.w / image.relativePixelRatio;
-    auto top = shapedIcon.top - border;
+    auto top = shapedIcon.top() - border;
     auto bottom = top + image.pos.h / image.relativePixelRatio;
     Point<float> tl;
     Point<float> tr;
@@ -67,7 +67,7 @@ SymbolQuad getIconQuad(const Anchor& anchor,
         bl = {left, bottom};
     }
 
-    float angle = shapedIcon.angle;
+    float angle = shapedIcon.angle();
     if (placement == style::SymbolPlacementType::Line) {
         assert(static_cast<unsigned int>(anchor.segment) < line.size());
         const GeometryCoordinate &prev= line[anchor.segment];
diff --git a/src/mbgl/text/shaping.cpp b/src/mbgl/text/shaping.cpp
index aa760ea4fe..5fae03b4c7 100644
--- a/src/mbgl/text/shaping.cpp
+++ b/src/mbgl/text/shaping.cpp
@@ -10,7 +10,11 @@
 
 namespace mbgl {
 
-PositionedIcon shapeIcon(const SpriteAtlasElement& image, const std::array<float, 2>& iconOffset, const float iconRotation) {
+optional<PositionedIcon> PositionedIcon::shapeIcon(const SpriteAtlasElement& image, const std::array<float, 2>& iconOffset, const float iconRotation) {
+    if (!image.pos.hasArea()) {
+        return {};
+    }
+
     float dx = iconOffset[0];
     float dy = iconOffset[1];
     float x1 = dx - image.width/ 2.0f;
@@ -18,7 +22,7 @@ PositionedIcon shapeIcon(const SpriteAtlasElement& image, const std::array<float
     float y1 = dy - image.height / 2.0f;
     float y2 = y1 + image.height;
 
-    return PositionedIcon(image, y1, y2, x1, x2, iconRotation);
+    return { PositionedIcon { image, y1, y2, x1, x2, iconRotation } };
 }
 
 void align(Shaping& shaping,
diff --git a/src/mbgl/text/shaping.hpp b/src/mbgl/text/shaping.hpp
index fc404e3a97..5e932e6df7 100644
--- a/src/mbgl/text/shaping.hpp
+++ b/src/mbgl/text/shaping.hpp
@@ -12,33 +12,38 @@ class SymbolFeature;
 class BiDi;
 
 class PositionedIcon {
-public:
-    PositionedIcon() = default;
+private:
     PositionedIcon(const SpriteAtlasElement& image_,
                    float top_,
                    float bottom_,
                    float left_,
                    float right_,
                    float angle_)
-        : image(image_),
-          top(top_),
-          bottom(bottom_),
-          left(left_),
-          right(right_),
-          angle(angle_) {}
-
-    optional<SpriteAtlasElement> image;
-    float top = 0;
-    float bottom = 0;
-    float left = 0;
-    float right = 0;
-    float angle = 0;
-
-    explicit operator bool() const { return image && (*image).pos.hasArea(); }
+        : _image(image_),
+          _top(top_),
+          _bottom(bottom_),
+          _left(left_),
+          _right(right_),
+          _angle(angle_) {}
+
+    optional<SpriteAtlasElement> _image;
+    float _top;
+    float _bottom;
+    float _left;
+    float _right;
+    float _angle;
+
+public:
+    static optional<PositionedIcon> shapeIcon(const class SpriteAtlasElement&, const std::array<float, 2>& iconOffset, const float iconRotation);
+
+    optional<class SpriteAtlasElement> image() const { return _image; }
+    float top() const { return _top; }
+    float bottom() const { return _bottom; }
+    float left() const { return _left; }
+    float right() const { return _right; }
+    float angle() const { return _angle; }
 };
 
-PositionedIcon shapeIcon(const SpriteAtlasElement&, const std::array<float, 2>& iconOffset, const float iconRotation);
-    
 const Shaping getShaping(const std::u16string& string,
                          float maxWidth,
                          float lineHeight,
diff --git a/test/text/quads.test.cpp b/test/text/quads.test.cpp
index 18fbedc2dd..91f2ea7af8 100644
--- a/test/text/quads.test.cpp
+++ b/test/text/quads.test.cpp
@@ -18,12 +18,15 @@ TEST(getIconQuads, normal) {
         { 0, 0 },
         1.0f
     };
-    PositionedIcon shapedIcon(image, -5.0, 6.0, -7.0, 8.0, 0);
+
+    auto shapedIcon = PositionedIcon::shapeIcon(image, {{ -6.5f, -4.5f }}, 0);
+    ASSERT_TRUE(shapedIcon);
+
     GeometryCoordinates line;
     Shaping shapedText;
 
     SymbolQuad quad =
-        getIconQuad(anchor, shapedIcon, line, layout, 16.0f, SymbolPlacementType::Point, shapedText);
+        getIconQuad(anchor, *shapedIcon, line, layout, 16.0f, SymbolPlacementType::Point, shapedText);
 
     ASSERT_EQ(quad.anchorPoint.x, 2);
     ASSERT_EQ(quad.anchorPoint.y, 3);
@@ -48,7 +51,10 @@ TEST(getIconQuads, style) {
         { 0, 0 },
         1.0f
     };
-    PositionedIcon shapedIcon(image, -10.0, 10.0, -10.0, 10.0, 0);
+
+    auto shapedIcon = PositionedIcon::shapeIcon(image, {{ -9.5f, -9.5f }}, 0);
+    ASSERT_TRUE(shapedIcon);
+
     GeometryCoordinates line;
     Shaping shapedText;
     shapedText.top = -10.0f;
@@ -61,7 +67,7 @@ TEST(getIconQuads, style) {
     {
         SymbolLayoutProperties::Evaluated layout;
         SymbolQuad quad =
-            getIconQuad(anchor, shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
+            getIconQuad(anchor, *shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
 
         ASSERT_EQ(quad.anchorPoint.x, 0);
         ASSERT_EQ(quad.anchorPoint.y, 0);
@@ -84,7 +90,7 @@ TEST(getIconQuads, style) {
         layout.get<TextSize>() = 24.0f;
         layout.get<IconTextFit>() = IconTextFitType::Width;
         SymbolQuad quad =
-            getIconQuad(anchor, shapedIcon, line, layout, 24.0f, SymbolPlacementType::Point, shapedText);
+            getIconQuad(anchor, *shapedIcon, line, layout, 24.0f, SymbolPlacementType::Point, shapedText);
 
         ASSERT_EQ(quad.tl.x, -60);
         ASSERT_EQ(quad.tl.y, 0);
@@ -102,7 +108,7 @@ TEST(getIconQuads, style) {
         layout.get<TextSize>() = 12.0f;
         layout.get<IconTextFit>() = IconTextFitType::Width;
         SymbolQuad quad =
-            getIconQuad(anchor, shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
+            getIconQuad(anchor, *shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
 
         ASSERT_EQ(quad.tl.x, -30);
         ASSERT_EQ(quad.tl.y, -5);
@@ -124,7 +130,7 @@ TEST(getIconQuads, style) {
         layout.get<IconTextFitPadding>()[2] = 5.0f;
         layout.get<IconTextFitPadding>()[3] = 10.0f;
         SymbolQuad quad =
-            getIconQuad(anchor, shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
+            getIconQuad(anchor, *shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
 
         ASSERT_EQ(quad.tl.x, -40);
         ASSERT_EQ(quad.tl.y, -10);
@@ -142,7 +148,7 @@ TEST(getIconQuads, style) {
         layout.get<TextSize>() = 24.0f;
         layout.get<IconTextFit>() = IconTextFitType::Height;
         SymbolQuad quad =
-            getIconQuad(anchor, shapedIcon, line, layout, 24.0f, SymbolPlacementType::Point, shapedText);
+            getIconQuad(anchor, *shapedIcon, line, layout, 24.0f, SymbolPlacementType::Point, shapedText);
 
         ASSERT_EQ(quad.tl.x, -30);
         ASSERT_EQ(quad.tl.y, -10);
@@ -160,7 +166,7 @@ TEST(getIconQuads, style) {
         layout.get<TextSize>() = 12.0f;
         layout.get<IconTextFit>() = IconTextFitType::Height;
         SymbolQuad quad =
-            getIconQuad(anchor, shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
+            getIconQuad(anchor, *shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
 
         ASSERT_EQ(quad.tl.x, -20);
         ASSERT_EQ(quad.tl.y, -5);
@@ -182,7 +188,7 @@ TEST(getIconQuads, style) {
         layout.get<IconTextFitPadding>()[2] = 5.0f;
         layout.get<IconTextFitPadding>()[3] = 10.0f;
         SymbolQuad quad =
-            getIconQuad(anchor, shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
+            getIconQuad(anchor, *shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
 
         ASSERT_EQ(quad.tl.x, -30);
         ASSERT_EQ(quad.tl.y, -10);
@@ -200,7 +206,7 @@ TEST(getIconQuads, style) {
         layout.get<TextSize>() = 24.0f;
         layout.get<IconTextFit>() = IconTextFitType::Both;
         SymbolQuad quad =
-            getIconQuad(anchor, shapedIcon, line, layout, 24.0f, SymbolPlacementType::Point, shapedText);
+            getIconQuad(anchor, *shapedIcon, line, layout, 24.0f, SymbolPlacementType::Point, shapedText);
 
         ASSERT_EQ(quad.tl.x, -60);
         ASSERT_EQ(quad.tl.y, -10);
@@ -218,7 +224,7 @@ TEST(getIconQuads, style) {
         layout.get<TextSize>() = 12.0f;
         layout.get<IconTextFit>() = IconTextFitType::Both;
         SymbolQuad quad =
-            getIconQuad(anchor, shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
+            getIconQuad(anchor, *shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
 
         ASSERT_EQ(quad.tl.x, -30);
         ASSERT_EQ(quad.tl.y, -5);
@@ -240,7 +246,7 @@ TEST(getIconQuads, style) {
         layout.get<IconTextFitPadding>()[2] = 5.0f;
         layout.get<IconTextFitPadding>()[3] = 10.0f;
         SymbolQuad quad =
-            getIconQuad(anchor, shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
+            getIconQuad(anchor, *shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
 
         ASSERT_EQ(quad.tl.x, -40);
         ASSERT_EQ(quad.tl.y, -10);
@@ -262,7 +268,7 @@ TEST(getIconQuads, style) {
         layout.get<IconTextFitPadding>()[2] = 10.0f;
         layout.get<IconTextFitPadding>()[3] = 15.0f;
         SymbolQuad quad =
-            getIconQuad(anchor, shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
+            getIconQuad(anchor, *shapedIcon, line, layout, 12.0f, SymbolPlacementType::Point, shapedText);
 
         ASSERT_EQ(quad.tl.x, -45);
         ASSERT_EQ(quad.tl.y, -5);
diff --git a/test/tile/annotation_tile.test.cpp b/test/tile/annotation_tile.test.cpp
index 4e28a3101c..cba2b61b59 100644
--- a/test/tile/annotation_tile.test.cpp
+++ b/test/tile/annotation_tile.test.cpp
@@ -58,7 +58,7 @@ TEST(AnnotationTile, Issue8289) {
     auto collisionTile = std::make_unique<CollisionTile>(PlacementConfig());
 
     IndexedSubfeature subfeature { 0, "", "", 0 };
-    CollisionFeature feature(GeometryCoordinates(), Anchor(0, 0, 0, 0), -5, 5, -5, 5, 1, 0, style::SymbolPlacementType::Point, subfeature, false);
+    CollisionFeature feature(GeometryCoordinates(), Anchor(0, 0, 0, 0), -5, 5, -5, 5, 1, 0, style::SymbolPlacementType::Point, subfeature, CollisionFeature::AlignmentType::Curved);
     collisionTile->insertFeature(feature, 0, true);
     collisionTile->placeFeature(feature, false, false);