From 76658d8717f79f7dfe64a62a8be970367b2ba205 Mon Sep 17 00:00:00 2001
From: Tim Sylvester <tim@wetdogweather.com>
Date: Tue, 16 May 2023 09:59:12 -0700
Subject: [PATCH] Drawable Renderer (#1570), Fix color normalization problems,
 don't produce zero-alpha drawables

---
 include/mbgl/gfx/drawable.hpp                  | 10 ++++++++++
 include/mbgl/gl/drawable_gl.hpp                |  9 ---------
 src/mbgl/gl/drawable_gl.cpp                    |  2 +-
 src/mbgl/gl/drawable_gl_builder.cpp            |  4 ++--
 src/mbgl/renderer/layers/render_fill_layer.cpp |  8 ++++----
 5 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/include/mbgl/gfx/drawable.hpp b/include/mbgl/gfx/drawable.hpp
index 1049ec4f2..bb33cab27 100644
--- a/include/mbgl/gfx/drawable.hpp
+++ b/include/mbgl/gfx/drawable.hpp
@@ -3,6 +3,7 @@
 #include <mbgl/gfx/uniform_buffer.hpp>
 #include <mbgl/gfx/vertex_attribute.hpp>
 #include <mbgl/tile/tile_id.hpp>
+#include <mbgl/util/color.hpp>
 #include <mbgl/util/identity.hpp>
 
 #include <cstdint>
@@ -102,6 +103,15 @@ public:
     // Reset a single color attribute for all vertexes
     virtual void resetColor(const Color&) = 0;
 
+    /// Convert from the odd partially-normalized color component array produced by `Color::toArray` into normalized RGBA.
+    static gfx::VertexAttribute::float4 colorAttrRGBA(const Color& color) {
+        const auto components = color.toArray();
+        return {static_cast<float>(components[0] / 255.0),
+                static_cast<float>(components[1] / 255.0),
+                static_cast<float>(components[2] / 255.0),
+                static_cast<float>(components[3])};
+    }
+
 protected:
     std::string name;
     util::SimpleIdentity uniqueID;
diff --git a/include/mbgl/gl/drawable_gl.hpp b/include/mbgl/gl/drawable_gl.hpp
index 0d46b7392..f1b757005 100644
--- a/include/mbgl/gl/drawable_gl.hpp
+++ b/include/mbgl/gl/drawable_gl.hpp
@@ -2,7 +2,6 @@
 
 #include <mbgl/gfx/drawable.hpp>
 #include <mbgl/gl/vertex_attribute_gl.hpp>
-#include <mbgl/util/color.hpp>
 
 #include <memory>
 
@@ -48,14 +47,6 @@ public:
     /// Reset a single color attribute for all vertexes
     void resetColor(const Color&) override;
 
-    static gfx::VertexAttribute::float4 colorAttrValue(const Color& color) {
-        const auto components = color.toArray();
-        return {static_cast<float>(components[0] / 255.0),
-                static_cast<float>(components[1] / 255.0),
-                static_cast<float>(components[2] / 255.0),
-                static_cast<float>(components[3])};
-    }
-
 protected:
     class Impl;
     const std::unique_ptr<Impl> impl;
diff --git a/src/mbgl/gl/drawable_gl.cpp b/src/mbgl/gl/drawable_gl.cpp
index 0b2e65e8b..b6a7e57a4 100644
--- a/src/mbgl/gl/drawable_gl.cpp
+++ b/src/mbgl/gl/drawable_gl.cpp
@@ -82,7 +82,7 @@ gfx::UniformBufferArray& DrawableGL::mutableUniformBuffers() {
 void DrawableGL::resetColor(const Color& newColor) {
     if (const auto& colorAttr = impl->vertexAttributes.get("a_color")) {
         colorAttr->clear();
-        colorAttr->set(0, colorAttrValue(newColor));
+        colorAttr->set(0, Drawable::colorAttrRGBA(newColor));
     }
 }
 
diff --git a/src/mbgl/gl/drawable_gl_builder.cpp b/src/mbgl/gl/drawable_gl_builder.cpp
index b2e8f1688..0d740b489 100644
--- a/src/mbgl/gl/drawable_gl_builder.cpp
+++ b/src/mbgl/gl/drawable_gl_builder.cpp
@@ -30,12 +30,12 @@ void DrawableGLBuilder::init() {
             }
 
             if (impl->colors.empty()) {
-                colorAttr->set(0, DrawableGL::colorAttrValue(getColor()));
+                colorAttr->set(0, gfx::Drawable::colorAttrRGBA(getColor()));
             } else {
                 std::size_t index = 0;
                 colorAttr->reserve(impl->colors.size());
                 for (const auto& color : impl->colors) {
-                    colorAttr->set(index++, DrawableGL::colorAttrValue(color));
+                    colorAttr->set(index++, gfx::Drawable::colorAttrRGBA(color));
                 }
             }
         }
diff --git a/src/mbgl/renderer/layers/render_fill_layer.cpp b/src/mbgl/renderer/layers/render_fill_layer.cpp
index 66eb14168..5da921c9a 100644
--- a/src/mbgl/renderer/layers/render_fill_layer.cpp
+++ b/src/mbgl/renderer/layers/render_fill_layer.cpp
@@ -343,21 +343,21 @@ void RenderFillLayer::update(const int32_t layerIndex,
                 const auto& evaluated = getEvaluated<FillLayerProperties>(renderData->layerProperties);
                 const auto evalColor = evaluated.get<FillColor>().constantOr(Color());
                 const auto fillOpacity = evaluated.get<FillOpacity>().constantOr(1);
-                // const auto fillColor = evalColor * (fillOpacity > 0.0f ? fillOpacity : 1.0f);
+                const auto fillColor = evalColor * (fillOpacity > 0.0f ? fillOpacity : 1.0f);
 
-                const auto fillRenderPass = (evalColor.a >= 1.0f && fillOpacity >= 1.0f
+                const auto fillRenderPass = (fillColor.a >= 1.0f
                                              /* && parameters.currentLayer >= parameters.opaquePassCutoff*/)
                                                 ? RenderPass::Opaque
                                                 : RenderPass::Translucent;
 
-                if (fillRenderPass != renderPass) {
+                if (fillRenderPass != renderPass || fillColor.a <= 0.0f) {
                     removeTile();
                     continue;
                 }
 
                 vertexAttrs.clear();
                 if (auto& attr = vertexAttrs.getOrAdd("a_color")) {
-                    attr->set(0, util::cast<float>(evalColor.toArray()));
+                    attr->set(0, gfx::Drawable::colorAttrRGBA(evalColor));
                 }
                 if (auto& attr = vertexAttrs.getOrAdd("a_opacity")) {
                     attr->set<gfx::VertexAttribute::float2>(0, {fillOpacity, fillOpacity});