From 8b37a0704c7c20b1664fee3dfcc79139982a1ce7 Mon Sep 17 00:00:00 2001
From: Asheem Mamoowala <asheem.mamoowala@mapbox.com>
Date: Wed, 31 May 2017 11:53:04 -0700
Subject: [core] Use fixed-size std::array for ImageSource coordinates

---
 include/mbgl/style/conversion/coordinate.hpp       |  4 ++--
 include/mbgl/style/conversion/source.hpp           |  7 +++----
 include/mbgl/style/sources/image_source.hpp        | 12 ++++++------
 platform/android/src/style/android_conversion.hpp  |  3 +--
 platform/default/mbgl/storage/offline_download.cpp |  8 ++++----
 src/mbgl/renderer/buckets/raster_bucket.cpp        |  7 ++++---
 src/mbgl/renderer/buckets/raster_bucket.hpp        |  2 ++
 src/mbgl/renderer/layers/render_raster_layer.cpp   | 19 ++++---------------
 src/mbgl/renderer/sources/render_image_source.cpp  |  7 +++----
 src/mbgl/renderer/sources/render_image_source.hpp  |  7 ++-----
 src/mbgl/style/sources/image_source.cpp            | 12 ++++++------
 src/mbgl/style/sources/image_source_impl.cpp       | 10 +++-------
 src/mbgl/style/sources/image_source_impl.hpp       | 15 +++++++--------
 test/style/source.test.cpp                         |  2 +-
 14 files changed, 48 insertions(+), 67 deletions(-)

diff --git a/include/mbgl/style/conversion/coordinate.hpp b/include/mbgl/style/conversion/coordinate.hpp
index 736f5e94a2..732624e77f 100644
--- a/include/mbgl/style/conversion/coordinate.hpp
+++ b/include/mbgl/style/conversion/coordinate.hpp
@@ -13,7 +13,7 @@ public:
     template <class V>
     optional<LatLng> operator() (const V& value, Error& error) const {
         if (!isArray(value) || arrayLength(value) < 2 ) {
-            error = { "coordinate array must contain numeric longtitude and latitude values" };
+            error = { "coordinate array must contain numeric longitude and latitude values" };
             return {};
         }
         //Style spec uses GeoJSON convention for specifying coordinates
@@ -21,7 +21,7 @@ public:
         optional<double> longitude = toDouble(arrayMember(value, 0));
     
         if (!latitude || !longitude) {
-            error = { "coordinate array must contain numeric longtitude and latitude values" };
+            error = { "coordinate array must contain numeric longitude and latitude values" };
             return {};
         }
         if (*latitude < -90 || *latitude > 90 ){
diff --git a/include/mbgl/style/conversion/source.hpp b/include/mbgl/style/conversion/source.hpp
index e121e36955..e0563ac10b 100644
--- a/include/mbgl/style/conversion/source.hpp
+++ b/include/mbgl/style/conversion/source.hpp
@@ -170,14 +170,13 @@ private:
             return {};
         }
         
-        std::vector<LatLng> coordinates;
-        coordinates.reserve(4);
-        for( std::size_t i=0; i < arrayLength(*coordinatesValue); i++) {
+        std::array<LatLng, 4> coordinates;
+        for (std::size_t i=0; i < 4; i++) {
             auto latLng = conversion::convert<LatLng>(arrayMember(*coordinatesValue,i), error);
             if (!latLng) {
                 return {};
             }
-            coordinates.push_back(*latLng);
+            coordinates[i] = *latLng;
         }
         auto result = std::make_unique<ImageSource>(id, coordinates);
         result->setURL(*urlString);
diff --git a/include/mbgl/style/sources/image_source.hpp b/include/mbgl/style/sources/image_source.hpp
index fd67ad38e5..d8a2c45bd8 100644
--- a/include/mbgl/style/sources/image_source.hpp
+++ b/include/mbgl/style/sources/image_source.hpp
@@ -12,16 +12,16 @@ namespace style {
 
 class ImageSource : public Source {
 public:
-    ImageSource(std::string id, const std::vector<LatLng>);
+    ImageSource(std::string id, const std::array<LatLng, 4>);
     ~ImageSource() override;
 
-    const std::string& getURL() const;
-    void setURL(const std::string& url) ;
+    optional<std::string> getURL() const;
+    void setURL(const std::string& url);
 
-    void setImage(mbgl::UnassociatedImage&&);
+    void setImage(UnassociatedImage&&);
 
-    void setCoordinates(const std::vector<LatLng>&);
-    std::vector<LatLng> getCoordinates() const;
+    void setCoordinates(const std::array<LatLng, 4>&);
+    std::array<LatLng, 4> getCoordinates() const;
 
     class Impl;
     const Impl& impl() const;
diff --git a/platform/android/src/style/android_conversion.hpp b/platform/android/src/style/android_conversion.hpp
index 790c82a1c0..6327ed96d0 100644
--- a/platform/android/src/style/android_conversion.hpp
+++ b/platform/android/src/style/android_conversion.hpp
@@ -68,8 +68,7 @@ inline optional<float> toNumber(const mbgl::android::Value& value) {
 
 inline optional<double> toDouble(const mbgl::android::Value& value) {
     if (value.isNumber()) {
-        auto num = value.toDouble();
-        return num;
+        return value.toDouble();
     } else {
         return {};
     }
diff --git a/platform/default/mbgl/storage/offline_download.cpp b/platform/default/mbgl/storage/offline_download.cpp
index 980561e43a..7f0001f64b 100644
--- a/platform/default/mbgl/storage/offline_download.cpp
+++ b/platform/default/mbgl/storage/offline_download.cpp
@@ -121,7 +121,7 @@ OfflineRegionStatus OfflineDownload::getStatus() const {
 
         case SourceType::Image: {
             const auto& imageSource = *source->as<ImageSource>();
-            if (!imageSource.getURL().empty()) {
+            if (imageSource.getURL()) {
                 result.requiredResourceCount += 1;
             }
             break;
@@ -205,9 +205,9 @@ void OfflineDownload::activateDownload() {
 
             case SourceType::Image: {
                 const auto& imageSource = *source->as<ImageSource>();
-                std::string imageUrl = imageSource.getURL();
-                if (!imageUrl.empty()) {
-                    queueResource(Resource::image(imageUrl));
+                auto imageUrl = imageSource.getURL();
+                if (imageUrl && !imageUrl->empty()) {
+                    queueResource(Resource::image(*imageUrl));
                 }
                 break;
             }
diff --git a/src/mbgl/renderer/buckets/raster_bucket.cpp b/src/mbgl/renderer/buckets/raster_bucket.cpp
index fa105fd38d..e0bfaf7b68 100644
--- a/src/mbgl/renderer/buckets/raster_bucket.cpp
+++ b/src/mbgl/renderer/buckets/raster_bucket.cpp
@@ -1,11 +1,12 @@
-#include <mbgl/gl/context.hpp>
-#include <mbgl/programs/raster_program.hpp>
 #include <mbgl/renderer/buckets/raster_bucket.hpp>
 #include <mbgl/renderer/layers/render_raster_layer.hpp>
+#include <mbgl/programs/raster_program.hpp>
 #include <mbgl/renderer/painter.hpp>
+#include <mbgl/gl/context.hpp>
 #include <mbgl/renderer/render_tile.hpp>
 
 namespace mbgl {
+
 using namespace style;
 
 RasterBucket::RasterBucket(UnassociatedImage&& image_) : image(std::move(image_)) {
@@ -17,7 +18,7 @@ RasterBucket::RasterBucket(RasterBucket&& other) : image(std::move(other.image))
 
 void RasterBucket::upload(gl::Context& context) {
     texture = context.createTexture(image);
-    if (vertices.vertexSize() > 0) {
+    if (!vertices.empty()) {
         vertexBuffer = context.createVertexBuffer(std::move(vertices));
         indexBuffer = context.createIndexBuffer(std::move(indices));
     }
diff --git a/src/mbgl/renderer/buckets/raster_bucket.hpp b/src/mbgl/renderer/buckets/raster_bucket.hpp
index b7b1d312dc..08afa6ddfa 100644
--- a/src/mbgl/renderer/buckets/raster_bucket.hpp
+++ b/src/mbgl/renderer/buckets/raster_bucket.hpp
@@ -27,6 +27,8 @@ public:
     UnassociatedImage image;
     optional<gl::Texture> texture;
 
+    // Bucket specific vertices are used for Image Sources only
+    // Raster Tile Sources use the default buffers from Painter
     gl::VertexVector<RasterLayoutVertex> vertices;
     gl::IndexVector<gl::Triangles> indices;
     gl::SegmentVector<RasterAttributes> segments;
diff --git a/src/mbgl/renderer/layers/render_raster_layer.cpp b/src/mbgl/renderer/layers/render_raster_layer.cpp
index 3551f9fa9a..49983462a3 100644
--- a/src/mbgl/renderer/layers/render_raster_layer.cpp
+++ b/src/mbgl/renderer/layers/render_raster_layer.cpp
@@ -37,14 +37,8 @@ bool RenderRasterLayer::hasTransition() const {
 }
 
 void RenderRasterLayer::uploadBuckets(gl::Context& context, RenderSource* source) {
-    if (renderTiles.size() > 0) {
-        for (const auto& tileRef : renderTiles) {
-            const auto& bucket = tileRef.get().tile.getBucket(impl());
-            if (bucket && bucket->needsUpload()) {
-                bucket->upload(context);
-            }
-        }
-    } else {
+    RenderLayer::uploadBuckets(context, source);
+    if (renderTiles.size() == 0) {
         RenderImageSource* imageSource = dynamic_cast<RenderImageSource*>(source);
         if (imageSource) {
             imageSource->upload(context);
@@ -53,13 +47,8 @@ void RenderRasterLayer::uploadBuckets(gl::Context& context, RenderSource* source
 }
 
 void RenderRasterLayer::render(Painter& painter, PaintParameters& parameters, RenderSource* source) {
-    if (renderTiles.size() > 0) {
-        for (auto& tileRef : renderTiles) {
-            auto& tile = tileRef.get();
-            auto bucket = tile.tile.getBucket(impl());
-            bucket->render(painter, parameters, *this, tile);
-        }
-    } else {
+    RenderLayer::render(painter, parameters, source);
+    if (renderTiles.size() == 0) {
         RenderImageSource* imageSource = dynamic_cast<RenderImageSource*>(source);
         if (imageSource) {
             imageSource->render(painter, parameters, *this);
diff --git a/src/mbgl/renderer/sources/render_image_source.cpp b/src/mbgl/renderer/sources/render_image_source.cpp
index 975b08eb47..3fec926b7f 100644
--- a/src/mbgl/renderer/sources/render_image_source.cpp
+++ b/src/mbgl/renderer/sources/render_image_source.cpp
@@ -16,6 +16,8 @@ RenderImageSource::RenderImageSource(Immutable<style::ImageSource::Impl> impl_)
     : RenderSource(impl_), shouldRender(false) {
 }
 
+RenderImageSource::~RenderImageSource() = default;
+
 const style::ImageSource::Impl& RenderImageSource::impl() const {
     return static_cast<const style::ImageSource::Impl&>(*baseImpl);
 }
@@ -127,10 +129,7 @@ void RenderImageSource::updateTiles(const TileParameters& parameters) {
         auto gc = TileCoordinate::toGeometryCoordinate(tileIds[0], tc.p);
         geomCoords.push_back(gc);
     }
-    setupBucket(geomCoords);
-}
-
-void RenderImageSource::setupBucket(GeometryCoordinates& geomCoords) {
+    
     if (!bucket) {
         UnassociatedImage img = impl().getImage().clone();
         if (!img.valid()) {
diff --git a/src/mbgl/renderer/sources/render_image_source.hpp b/src/mbgl/renderer/sources/render_image_source.hpp
index 769f932f27..86c17b10fd 100644
--- a/src/mbgl/renderer/sources/render_image_source.hpp
+++ b/src/mbgl/renderer/sources/render_image_source.hpp
@@ -1,16 +1,13 @@
 #pragma once
 
-#include <mbgl/renderer/buckets/raster_bucket.hpp>
 #include <mbgl/renderer/render_source.hpp>
 #include <mbgl/renderer/render_tile.hpp>
 #include <mbgl/style/sources/image_source_impl.hpp>
-#include <mbgl/tile/geometry_tile_data.hpp>
-#include <mbgl/util/image.hpp>
-#include <mbgl/util/optional.hpp>
 
 namespace mbgl {
 class RenderLayer;
 class PaintParameters;
+class RasterBucket;
 
 namespace gl {
 class Context;
@@ -19,6 +16,7 @@ class Context;
 class RenderImageSource : public RenderSource {
 public:
     RenderImageSource(Immutable<style::ImageSource::Impl>);
+    ~RenderImageSource() override;
 
     bool isLoaded() const final;
     void upload(gl::Context&);
@@ -59,7 +57,6 @@ public:
 
 private:
     const style::ImageSource::Impl& impl() const;
-    void setupBucket(GeometryCoordinates& coordiantes);
     std::map<UnwrappedTileID, RenderTile> tiles;
 
     std::vector<UnwrappedTileID> tileIds;
diff --git a/src/mbgl/style/sources/image_source.cpp b/src/mbgl/style/sources/image_source.cpp
index 79df65697c..9313d8da4a 100644
--- a/src/mbgl/style/sources/image_source.cpp
+++ b/src/mbgl/style/sources/image_source.cpp
@@ -8,7 +8,7 @@
 namespace mbgl {
 namespace style {
 
-ImageSource::ImageSource(std::string id, const std::vector<LatLng> coords_)
+ImageSource::ImageSource(std::string id, const std::array<LatLng, 4> coords_)
     : Source(makeMutable<Impl>(std::move(id), coords_)) {
 }
 
@@ -18,12 +18,12 @@ const ImageSource::Impl& ImageSource::impl() const {
     return static_cast<const Impl&>(*baseImpl);
 }
 
-void ImageSource::setCoordinates(const std::vector<LatLng>& coords_) {
+void ImageSource::setCoordinates(const std::array<LatLng, 4>& coords_) {
     baseImpl = makeMutable<Impl>(impl(), coords_);
     observer->onSourceChanged(*this);
 }
 
-std::vector<LatLng> ImageSource::getCoordinates() const {
+std::array<LatLng, 4> ImageSource::getCoordinates() const {
     return impl().getCoordinates();
 }
 
@@ -37,7 +37,7 @@ void ImageSource::setURL(const std::string& url_) {
     }
 }
 
-void ImageSource::setImage(mbgl::UnassociatedImage&& image_) {
+void ImageSource::setImage(UnassociatedImage&& image_) {
     url = {};
     if (req) {
         req.reset();
@@ -47,8 +47,8 @@ void ImageSource::setImage(mbgl::UnassociatedImage&& image_) {
     observer->onSourceChanged(*this);
 }
 
-const std::string& ImageSource::getURL() const {
-    return *url;
+optional<std::string> ImageSource::getURL() const {
+    return url;
 }
 
 void ImageSource::loadDescription(FileSource& fileSource) {
diff --git a/src/mbgl/style/sources/image_source_impl.cpp b/src/mbgl/style/sources/image_source_impl.cpp
index 3678f641cb..98f3cc9db9 100644
--- a/src/mbgl/style/sources/image_source_impl.cpp
+++ b/src/mbgl/style/sources/image_source_impl.cpp
@@ -4,12 +4,12 @@
 namespace mbgl {
 namespace style {
 
-ImageSource::Impl::Impl(std::string id_, std::vector<LatLng> coords_)
+ImageSource::Impl::Impl(std::string id_, std::array<LatLng, 4> coords_)
     : Source::Impl(SourceType::Image, std::move(id_)),
     coords(std::move(coords_)) {
 }
 
-ImageSource::Impl::Impl(const Impl& other, std::vector<LatLng> coords_)
+ImageSource::Impl::Impl(const Impl& other, std::array<LatLng, 4> coords_)
     : Source::Impl(other),
     coords(std::move(coords_)),
     image(other.image.clone()) {
@@ -22,15 +22,11 @@ ImageSource::Impl::Impl(const Impl& rhs, UnassociatedImage image_)
 }
 ImageSource::Impl::~Impl() = default;
 
-void ImageSource::Impl::setImage(UnassociatedImage&& image_) {
-    image = std::move(image_);
-}
-
 const UnassociatedImage& ImageSource::Impl::getImage() const {
     return image;
 }
 
-std::vector<LatLng> ImageSource::Impl::getCoordinates() const {
+std::array<LatLng, 4> ImageSource::Impl::getCoordinates() const {
     return coords;
 }
 
diff --git a/src/mbgl/style/sources/image_source_impl.hpp b/src/mbgl/style/sources/image_source_impl.hpp
index 26f025db74..5fd41ac6e6 100644
--- a/src/mbgl/style/sources/image_source_impl.hpp
+++ b/src/mbgl/style/sources/image_source_impl.hpp
@@ -3,28 +3,27 @@
 #include <mbgl/style/source_impl.hpp>
 #include <mbgl/style/sources/image_source.hpp>
 #include <mbgl/util/image.hpp>
+#include <mbgl/util/geo.hpp>
 
 namespace mbgl {
-class LatLng;
 
 namespace style {
 
 class ImageSource::Impl : public Source::Impl {
 public:
-    Impl(std::string id, std::vector<LatLng> coords);
-    Impl(const Impl& rhs, std::vector<LatLng> coords);
+    Impl(std::string id, std::array<LatLng, 4> coords);
+    Impl(const Impl& rhs, std::array<LatLng, 4> coords);
     Impl(const Impl& rhs, UnassociatedImage image);
 
     ~Impl() final;
 
-    void setImage(UnassociatedImage&& );
-    const mbgl::UnassociatedImage& getImage() const;
-    std::vector<LatLng> getCoordinates() const;
+    const UnassociatedImage& getImage() const;
+    std::array<LatLng, 4> getCoordinates() const;
 
     optional<std::string> getAttribution() const final;
 private:
-    std::vector<LatLng> coords;
-    mbgl::UnassociatedImage image;
+    std::array<LatLng, 4> coords;
+    UnassociatedImage image;
 };
 
 } // namespace style
diff --git a/test/style/source.test.cpp b/test/style/source.test.cpp
index e674d69ed0..84929178d0 100644
--- a/test/style/source.test.cpp
+++ b/test/style/source.test.cpp
@@ -454,7 +454,7 @@ TEST(Source, ImageSourceImageUpdate) {
         // Should be called (test will hang if it doesn't)
         test.end();
     };
-    std::vector<LatLng> coords;
+    std::array<LatLng, 4> coords;
 
     ImageSource source("source", coords);
     source.setURL("http://url");
-- 
cgit v1.2.1