fix review findings

7 files changed, 47 insertions, 30 deletions

diff --git a/src/mbgl/layout/symbol_instance.cpp b/src/mbgl/layout/symbol_instance.cpp
index a8b568d1cb..98f546325a 100644
--- a/src/mbgl/layout/symbol_instance.cpp
+++ b/src/mbgl/layout/symbol_instance.cpp
@@ -89,11 +89,10 @@ SymbolInstance::SymbolInstance(Anchor& anchor_,
                                const float textRotation,
                                float radialTextOffset_,
                                bool allowVerticalPlacement,
-                               const uint8_t iconFlag) :
+                               const SymbolContent& iconType) :
     sharedData(std::move(sharedData_)),
     anchor(anchor_),
-    // 'hasText' depends on finding at least one glyph in the shaping that's also in the GlyphPositionMap
-    hasText(!sharedData->empty()),
+    symbolContent(static_cast<uint8_t>(iconType)),
     // Create the collision features that will be used to check whether this symbol instance can be placed
     // As a collision approximation, we can use either the vertical or any of the horizontal versions of the feature
     textCollisionFeature(sharedData->line, anchor, getAnyShaping(shapedTextOrientations), textBoxScale_, textPadding, textPlacement, indexedFeature, overscaling, textRotation),
@@ -107,9 +106,8 @@ SymbolInstance::SymbolInstance(Anchor& anchor_,
     textBoxScale(textBoxScale_),
     radialTextOffset(radialTextOffset_),
     singleLine(shapedTextOrientations.singleLine) {
-    assert(iconFlag < 3);
-    hasIcon = iconFlag & 0x03;
-    hasSdfIcon = iconFlag & 0x02;
+     // 'hasText' depends on finding at least one glyph in the shaping that's also in the GlyphPositionMap
+    if(!sharedData->empty()) symbolContent |= static_cast<uint8_t>(SymbolContent::Text);
     if (allowVerticalPlacement && shapedTextOrientations.vertical) {
         const float verticalPointLabelAngle = 90.0f;
         verticalTextCollisionFeature = CollisionFeature(line(), anchor, shapedTextOrientations.vertical, textBoxScale_, textPadding, textPlacement, indexedFeature, overscaling, textRotation + verticalPointLabelAngle);
@@ -166,6 +164,18 @@ const optional<SymbolQuad>& SymbolInstance::iconQuad() const {
     assert(sharedData);
     return sharedData->iconQuad;
 }
+    
+bool SymbolInstance::hasText() const {
+    return symbolContent & static_cast<uint8_t>(SymbolContent::Text);
+}
+    
+bool SymbolInstance::hasIcon() const {
+    return (symbolContent & static_cast<uint8_t>(SymbolContent::IconRGBA)) || hasSdfIcon();
+}
+    
+bool SymbolInstance::hasSdfIcon() const {
+    return symbolContent & static_cast<uint8_t>(SymbolContent::IconSDF);
+}
 
 const optional<SymbolQuad>& SymbolInstance::verticalIconQuad() const {
     assert(sharedData);
diff --git a/src/mbgl/layout/symbol_instance.hpp b/src/mbgl/layout/symbol_instance.hpp
index be3c4abb8a..01e40c1970 100644
--- a/src/mbgl/layout/symbol_instance.hpp
+++ b/src/mbgl/layout/symbol_instance.hpp
@@ -42,6 +42,13 @@ struct SymbolInstanceSharedData {
     optional<SymbolQuad> verticalIconQuad;
 };
 
+enum class SymbolContent : uint8_t {
+    None = 0,
+    Text = 1 << 0,
+    IconRGBA= 1 << 1,
+    IconSDF = 1 << 2
+};
+
 class SymbolInstance {
 public:
     SymbolInstance(Anchor& anchor_,
@@ -65,7 +72,7 @@ public:
                    const float textRotation,
                    float radialTextOffset,
                    bool allowVerticalPlacement,
-                   const uint8_t iconFlags = 0x00);
+                   const SymbolContent& iconType = SymbolContent::None);
 
     optional<size_t> getDefaultHorizontalPlacedTextIndex() const;
     const GeometryCoordinates& line() const;
@@ -73,6 +80,9 @@ public:
     const SymbolQuads& leftJustifiedGlyphQuads() const;
     const SymbolQuads& centerJustifiedGlyphQuads() const;
     const SymbolQuads& verticalGlyphQuads() const;
+    bool hasText() const;
+    bool hasIcon() const;
+    bool hasSdfIcon() const;
     const optional<SymbolQuad>& iconQuad() const;
     const optional<SymbolQuad>& verticalIconQuad() const;
     void releaseSharedData();
@@ -82,9 +92,7 @@ private:
 
 public:
     Anchor anchor;
-    bool hasText : 1;
-    bool hasIcon : 1;
-    bool hasSdfIcon : 1;
+    uint8_t symbolContent;
 
     std::size_t rightJustifiedGlyphQuadsSize;
     std::size_t centerJustifiedGlyphQuadsSize;
diff --git a/src/mbgl/layout/symbol_layout.cpp b/src/mbgl/layout/symbol_layout.cpp
index 1ed292bd71..f77703e1f4 100644
--- a/src/mbgl/layout/symbol_layout.cpp
+++ b/src/mbgl/layout/symbol_layout.cpp
@@ -398,20 +398,18 @@ void SymbolLayout::prepareSymbols(const GlyphMap& glyphMap, const GlyphPositions
         }
 
         // if feature has icon, get sprite atlas position
-        
-        // Bitwise flag for icon status. 0x00 => not an icon, 0x01 => normal icon, 0x02 => sdf icon
-        uint8_t iconFlag{0x00};
+        SymbolContent iconType{SymbolContent::None};
         if (feature.icon) {
             auto image = imageMap.find(*feature.icon);
             if (image != imageMap.end()) {
-                iconFlag = 0x01;
+                iconType = SymbolContent::IconRGBA;
                 shapedIcon = PositionedIcon::shapeIcon(
                     imagePositions.at(*feature.icon),
                     layout->evaluate<IconOffset>(zoom, feature),
                     layout->evaluate<IconAnchor>(zoom, feature),
                     layout->evaluate<IconRotate>(zoom, feature) * util::DEG2RAD);
                 if (image->second->sdf) {
-                    iconFlag = 0x02;
+                    iconType = SymbolContent::IconSDF;
                 }
                 if (image->second->pixelRatio != pixelRatio) {
                     iconsNeedLinear = true;
@@ -423,7 +421,7 @@ void SymbolLayout::prepareSymbols(const GlyphMap& glyphMap, const GlyphPositions
 
         // if either shapedText or icon position is present, add the feature
         if (getDefaultHorizontalShaping(shapedTextOrientations) || shapedIcon) {
-            addFeature(std::distance(features.begin(), it), feature, shapedTextOrientations, std::move(shapedIcon), glyphPositions, textOffset, iconFlag);
+            addFeature(std::distance(features.begin(), it), feature, shapedTextOrientations, std::move(shapedIcon), glyphPositions, textOffset, iconType);
         }
 
         feature.geometry.clear();
@@ -438,7 +436,7 @@ void SymbolLayout::addFeature(const std::size_t layoutFeatureIndex,
                               optional<PositionedIcon> shapedIcon,
                               const GlyphPositions& glyphPositions,
                               Point<float> offset,
-                              uint8_t iconFlag ) {
+                              const SymbolContent& iconType) {
     const float minScale = 0.5f;
     const float glyphSize = 24.0f;
 
@@ -505,7 +503,7 @@ void SymbolLayout::addFeature(const std::size_t layoutFeatureIndex,
                     iconBoxScale, iconPadding, iconOffset, indexedFeature,
                     layoutFeatureIndex, feature.index,
                     feature.formattedText ? feature.formattedText->rawText() : std::u16string(),
-                    overscaling, iconRotation, textRotation, radialTextOffset, allowVerticalPlacement, iconFlag);
+                    overscaling, iconRotation, textRotation, radialTextOffset, allowVerticalPlacement, iconType);
         }
     };
 
@@ -631,8 +629,8 @@ void SymbolLayout::createBucket(const ImagePositions&, std::unique_ptr<FeatureIn
                                                  std::move(placementModes));
 
     for (SymbolInstance &symbolInstance : bucket->symbolInstances) {
-        const bool hasText = symbolInstance.hasText;
-        const bool hasIcon = symbolInstance.hasIcon;
+        const bool hasText = symbolInstance.hasText();
+        const bool hasIcon = symbolInstance.hasIcon();
         const bool singleLine = symbolInstance.singleLine;
 
         const auto& feature = features.at(symbolInstance.layoutFeatureIndex);
@@ -643,7 +641,7 @@ void SymbolLayout::createBucket(const ImagePositions&, std::unique_ptr<FeatureIn
         // is used when dynamic vertices for icon-text-fit image have to be updated.
         if (hasIcon) {
             const Range<float> sizeData = bucket->iconSizeBinder->getVertexSizeData(feature);
-            auto& iconBuffer = symbolInstance.hasSdfIcon ? bucket->sdfIcon : bucket->icon;
+            auto& iconBuffer = symbolInstance.hasSdfIcon() ? bucket->sdfIcon : bucket->icon;
             const auto placeIcon = [&] (const SymbolQuad& iconQuad, auto& index, const WritingModeType writingMode) {
                 iconBuffer.placedSymbols.emplace_back(symbolInstance.anchor.point, symbolInstance.anchor.segment, sizeData.min, sizeData.max,
                         symbolInstance.iconOffset, writingMode, symbolInstance.line(), std::vector<float>());
diff --git a/src/mbgl/layout/symbol_layout.hpp b/src/mbgl/layout/symbol_layout.hpp
index eeba351401..d7daafc4c6 100644
--- a/src/mbgl/layout/symbol_layout.hpp
+++ b/src/mbgl/layout/symbol_layout.hpp
@@ -54,7 +54,7 @@ private:
                     optional<PositionedIcon> shapedIcon,
                     const GlyphPositions&,
                     Point<float> textOffset,
-                    uint8_t iconFlag);
+                    const SymbolContent& iconType);
 
     bool anchorIsTooClose(const std::u16string& text, const float repeatDistance, const Anchor&);
     std::map<std::u16string, std::vector<Anchor>> compareText;
diff --git a/src/mbgl/renderer/buckets/symbol_bucket.cpp b/src/mbgl/renderer/buckets/symbol_bucket.cpp
index 7b652c05e6..b2f469927c 100644
--- a/src/mbgl/renderer/buckets/symbol_bucket.cpp
+++ b/src/mbgl/renderer/buckets/symbol_bucket.cpp
@@ -235,7 +235,7 @@ void SymbolBucket::sortFeatures(const float angle) {
             addPlacedSymbol(text.triangles, text.placedSymbols[*symbolInstance.placedVerticalTextIndex]);
         }
 
-        auto& iconBuffer = symbolInstance.hasSdfIcon ? sdfIcon : icon;
+        auto& iconBuffer = symbolInstance.hasSdfIcon() ? sdfIcon : icon;
         if (symbolInstance.placedIconIndex) {
             addPlacedSymbol(iconBuffer.triangles,
                             iconBuffer.placedSymbols[*symbolInstance.placedIconIndex]);
diff --git a/src/mbgl/text/placement.cpp b/src/mbgl/text/placement.cpp
index 03d3576b33..9ba8e71bff 100644
--- a/src/mbgl/text/placement.cpp
+++ b/src/mbgl/text/placement.cpp
@@ -367,7 +367,7 @@ void Placement::placeBucket(
                 shift = {0.0f, 0.0f};
             }
 
-            const auto& iconBuffer = symbolInstance.hasSdfIcon ? bucket.sdfIcon : bucket.icon;
+            const auto& iconBuffer = symbolInstance.hasSdfIcon() ? bucket.sdfIcon : bucket.icon;
             const PlacedSymbol& placedSymbol = iconBuffer.placedSymbols.at(*symbolInstance.placedIconIndex);
             const float fontSize = evaluateSizeForFeature(partiallyEvaluatedIconSize, placedSymbol);
             const auto& placeIconFeature = [&] (const CollisionFeature& collisionFeature) {
@@ -390,8 +390,8 @@ void Placement::placeBucket(
             offscreen &= placedIcon.second;
         }
 
-        const bool iconWithoutText = !symbolInstance.hasText || layout.get<style::TextOptional>();
-        const bool textWithoutIcon = !symbolInstance.hasIcon || layout.get<style::IconOptional>();
+        const bool iconWithoutText = !symbolInstance.hasText() || layout.get<style::TextOptional>();
+        const bool textWithoutIcon = !symbolInstance.hasIcon() || layout.get<style::IconOptional>();
 
         // combine placements for icon and text
         if (!iconWithoutText && !textWithoutIcon) {
@@ -733,7 +733,7 @@ void Placement::updateBucketOpacities(SymbolBucket& bucket, const TransformState
 
         seenCrossTileIDs.insert(symbolInstance.crossTileID);
 
-        if (symbolInstance.hasText) {
+        if (symbolInstance.hasText()) {
             size_t textOpacityVerticesSize = 0u;
             const auto& opacityVertex = SymbolSDFTextProgram::opacityVertex(opacityState.text.placed, opacityState.text.opacity);
             if (symbolInstance.placedRightTextIndex) {
@@ -772,9 +772,9 @@ void Placement::updateBucketOpacities(SymbolBucket& bucket, const TransformState
                 markUsedJustification(bucket, prevOffset->second.anchor, symbolInstance, previousOrientation);
             }
         }
-        if (symbolInstance.hasIcon) {
+        if (symbolInstance.hasIcon()) {
             const auto& opacityVertex = SymbolIconProgram::opacityVertex(opacityState.icon.placed, opacityState.icon.opacity);
-            auto& iconBuffer = symbolInstance.hasSdfIcon ? bucket.sdfIcon : bucket.icon;
+            auto& iconBuffer = symbolInstance.hasSdfIcon() ? bucket.sdfIcon : bucket.icon;
             
             if (symbolInstance.placedIconIndex) {
                 iconBuffer.opacityVertices.extend(4, opacityVertex);
@@ -935,7 +935,7 @@ void Placement::markUsedOrientation(SymbolBucket& bucket, style::TextWritingMode
         bucket.text.placedSymbols.at(*symbolInstance.placedVerticalTextIndex).placedOrientation = vertical;
     }
 
-    auto& iconBuffer = symbolInstance.hasSdfIcon ? bucket.sdfIcon : bucket.icon;
+    auto& iconBuffer = symbolInstance.hasSdfIcon() ? bucket.sdfIcon : bucket.icon;
     if (symbolInstance.placedIconIndex) {
         iconBuffer.placedSymbols.at(*symbolInstance.placedIconIndex).placedOrientation = horizontal;
     }
diff --git a/test/gl/bucket.test.cpp b/test/gl/bucket.test.cpp
index d06011d706..f30606ee42 100644
--- a/test/gl/bucket.test.cpp
+++ b/test/gl/bucket.test.cpp
@@ -123,6 +123,7 @@ TEST(Buckets, SymbolBucket) {
     gl::Context context{ backend };
     SymbolBucket bucket { std::move(layout), {}, 16.0f, 1.0f, 0, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(symbolInstances), 1.0f, false, {}};
     ASSERT_FALSE(bucket.hasIconData());
+    ASSERT_FALSE(bucket.hasSdfIconData());
     ASSERT_FALSE(bucket.hasTextData());
     ASSERT_FALSE(bucket.hasCollisionBoxData());
     ASSERT_FALSE(bucket.hasData());