From 87edff4047ddaf5a49b31c060bfae55d74d6c0cb Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Tue, 14 Jun 2016 12:36:46 -0700 Subject: [PATCH 1/3] [core] Use variant in TileSource A tile source can either specify a URL to TileJSON, or inline TileJSON. --- .../default/mbgl/storage/offline_download.cpp | 21 ++- src/mbgl/style/parser.cpp | 154 +--------------- src/mbgl/style/parser.hpp | 6 - src/mbgl/style/sources/raster_source.cpp | 30 ++- src/mbgl/style/sources/raster_source.hpp | 7 +- src/mbgl/style/sources/vector_source.cpp | 16 +- src/mbgl/style/sources/vector_source.hpp | 6 +- src/mbgl/style/tile_source.cpp | 174 ++++++++++++++++-- src/mbgl/style/tile_source.hpp | 29 ++- test/style/source.cpp | 52 +++--- test/style/style_parser.cpp | 30 --- test/style/tile_source.cpp | 37 ++++ test/test.gypi | 1 + 13 files changed, 299 insertions(+), 264 deletions(-) create mode 100644 test/style/tile_source.cpp diff --git a/platform/default/mbgl/storage/offline_download.cpp b/platform/default/mbgl/storage/offline_download.cpp index 8a156dd6491..3d0ebaa68f8 100644 --- a/platform/default/mbgl/storage/offline_download.cpp +++ b/platform/default/mbgl/storage/offline_download.cpp @@ -107,14 +107,17 @@ OfflineRegionStatus OfflineDownload::getStatus() const { case SourceType::Vector: case SourceType::Raster: { style::TileSource* tileSource = static_cast(source.get()); - if (tileSource->getTileset()) { - result.requiredResourceCount += tileResources(source->type, tileSource->getTileSize(), *tileSource->getTileset()).size(); + const variant& urlOrTileset = tileSource->getURLOrTileset(); + + if (urlOrTileset.is()) { + result.requiredResourceCount += tileResources(source->type, tileSource->getTileSize(), urlOrTileset.get()).size(); } else { result.requiredResourceCount += 1; - optional sourceResponse = offlineDatabase.get(Resource::source(tileSource->getURL())); + const std::string& url = urlOrTileset.get(); + optional sourceResponse = offlineDatabase.get(Resource::source(url)); if (sourceResponse) { result.requiredResourceCount += tileResources(source->type, tileSource->getTileSize(), - *style::parseTileJSON(*sourceResponse->data, tileSource->getURL(), source->type, tileSource->getTileSize())).size(); + style::TileSource::parseTileJSON(*sourceResponse->data, url, source->type, tileSource->getTileSize())).size(); } else { result.requiredResourceCountIsPrecise = false; } @@ -161,16 +164,18 @@ void OfflineDownload::activateDownload() { case SourceType::Vector: case SourceType::Raster: { const style::TileSource* tileSource = static_cast(source.get()); + const variant& urlOrTileset = tileSource->getURLOrTileset(); const uint16_t tileSize = tileSource->getTileSize(); - if (tileSource->getTileset()) { - ensureTiles(type, tileSize, *tileSource->getTileset()); + + if (urlOrTileset.is()) { + ensureTiles(type, tileSize, urlOrTileset.get()); } else { - std::string url = tileSource->getURL(); + const std::string& url = urlOrTileset.get(); status.requiredResourceCountIsPrecise = false; requiredSourceURLs.insert(url); ensureResource(Resource::source(url), [=] (Response sourceResponse) { - ensureTiles(type, tileSize, *style::parseTileJSON(*sourceResponse.data, url, type, tileSize)); + ensureTiles(type, tileSize, style::TileSource::parseTileJSON(*sourceResponse.data, url, type, tileSize)); requiredSourceURLs.erase(url); if (requiredSourceURLs.empty()) { diff --git a/src/mbgl/style/parser.cpp b/src/mbgl/style/parser.cpp index 07db3f0091f..09689f22f1d 100644 --- a/src/mbgl/style/parser.cpp +++ b/src/mbgl/style/parser.cpp @@ -13,7 +13,6 @@ #include #include -#include #include #include @@ -21,89 +20,11 @@ #include #include -#include #include namespace mbgl { namespace style { -namespace { - -void parseTileJSONMember(const JSValue& value, std::vector& target, const char* name) { - if (!value.HasMember(name)) { - return; - } - - const JSValue& property = value[name]; - if (!property.IsArray()) { - return; - } - - for (rapidjson::SizeType i = 0; i < property.Size(); i++) { - if (!property[i].IsString()) { - return; - } - } - - for (rapidjson::SizeType i = 0; i < property.Size(); i++) { - target.emplace_back(std::string(property[i].GetString(), property[i].GetStringLength())); - } -} - -void parseTileJSONMember(const JSValue& value, std::string& target, const char* name) { - if (!value.HasMember(name)) { - return; - } - - const JSValue& property = value[name]; - if (!property.IsString()) { - return; - } - - target = { property.GetString(), property.GetStringLength() }; -} - -void parseTileJSONMember(const JSValue& value, uint8_t& target, const char* name) { - if (!value.HasMember(name)) { - return; - } - - const JSValue& property = value[name]; - if (!property.IsUint()) { - return; - } - - unsigned int uint = property.GetUint(); - if (uint > std::numeric_limits::max()) { - return; - } - - target = uint; -} - -void parseTileJSONMember(const JSValue& value, std::array& target, const char* name) { - if (!value.HasMember(name)) { - return; - } - - const JSValue& property = value[name]; - if (!property.IsArray() || property.Size() > 4) { - return; - } - - for (rapidjson::SizeType i = 0; i < property.Size(); i++) { - if (!property[i].IsNumber()) { - return; - } - } - - for (rapidjson::SizeType i = 0; i < property.Size(); i++) { - target[i] = property[i].GetDouble(); - } -} - -} // end namespace - Parser::~Parser() = default; void Parser::parse(const std::string& json) { @@ -174,44 +95,16 @@ void Parser::parseSources(const JSValue& value) { } const std::string id { nameVal.GetString(), nameVal.GetStringLength() }; - - std::unique_ptr tileset; std::unique_ptr source; - // Sources can have URLs, either because they reference an external TileJSON file, or - // because reference a GeoJSON file. They don't have to have one though when all source - // parameters are specified inline. - std::string url; - if (sourceVal.HasMember("url")) { - const JSValue& urlVal = sourceVal["url"]; - if (urlVal.IsString()) { - url = { urlVal.GetString(), urlVal.GetStringLength() }; - } else { - Log::Error(Event::ParseStyle, "source url must be a string"); - continue; - } - } else { - tileset = parseTileJSON(sourceVal); - } - - uint16_t tileSize = util::tileSize; - if (sourceVal.HasMember("tileSize")) { - const JSValue& tileSizeVal = sourceVal["tileSize"]; - if (tileSizeVal.IsNumber() && tileSizeVal.GetUint64() <= std::numeric_limits::max()) { - tileSize = tileSizeVal.GetUint64(); - } else { - Log::Error(Event::ParseStyle, "invalid tileSize"); - continue; - } - } - switch (*type) { - case SourceType::Raster: - source = std::make_unique(id, url, tileSize, std::move(tileset)); + case SourceType::Raster: { + source = RasterSource::parse(id, sourceVal); break; + } case SourceType::Vector: - source = std::make_unique(id, url, std::move(tileset)); + source = VectorSource::parse(id, sourceVal); break; case SourceType::GeoJSON: @@ -230,45 +123,6 @@ void Parser::parseSources(const JSValue& value) { } } -std::unique_ptr parseTileJSON(const std::string& json, const std::string& sourceURL, SourceType type, uint16_t tileSize) { - rapidjson::GenericDocument, rapidjson::CrtAllocator> document; - document.Parse<0>(json.c_str()); - - if (document.HasParseError()) { - std::stringstream message; - message << document.GetErrorOffset() << " - " << rapidjson::GetParseError_En(document.GetParseError()); - throw std::runtime_error(message.str()); - } - - std::unique_ptr result = parseTileJSON(document); - - // TODO: Remove this hack by delivering proper URLs in the TileJSON to begin with. - if (util::mapbox::isMapboxURL(sourceURL)) { - for (auto& url : result->tiles) { - url = util::mapbox::canonicalizeTileURL(url, type, tileSize); - } - } - - return result; -} - -std::unique_ptr parseTileJSON(const JSValue& value) { - auto tileset = std::make_unique(); - parseTileJSONMember(value, tileset->tiles, "tiles"); - parseTileJSONMember(value, tileset->zoomRange.min, "minzoom"); - parseTileJSONMember(value, tileset->zoomRange.max, "maxzoom"); - parseTileJSONMember(value, tileset->attribution, "attribution"); - - std::array array; - parseTileJSONMember(value, array, "center"); - tileset->center = { array[0], array[1] }; - tileset->zoom = array[2]; - parseTileJSONMember(value, array, "bounds"); - tileset->bounds = LatLngBounds::hull({ array[0], array[1] }, { array[2], array[3] }); - - return tileset; -} - void Parser::parseLayers(const JSValue& value) { std::vector ids; diff --git a/src/mbgl/style/parser.hpp b/src/mbgl/style/parser.hpp index a04f73bde4f..6dac6dedcde 100644 --- a/src/mbgl/style/parser.hpp +++ b/src/mbgl/style/parser.hpp @@ -14,14 +14,8 @@ #include namespace mbgl { - -class Tileset; - namespace style { -std::unique_ptr parseTileJSON(const std::string& json, const std::string& sourceURL, SourceType, uint16_t tileSize); -std::unique_ptr parseTileJSON(const JSValue&); - Filter parseFilter(const JSValue&); class Parser { diff --git a/src/mbgl/style/sources/raster_source.cpp b/src/mbgl/style/sources/raster_source.cpp index 85d33dc72b5..9f873a30651 100644 --- a/src/mbgl/style/sources/raster_source.cpp +++ b/src/mbgl/style/sources/raster_source.cpp @@ -1,19 +1,39 @@ #include #include +#include namespace mbgl { namespace style { +std::unique_ptr RasterSource::parse(std::string id, const JSValue& value) { + optional> urlOrTileset = TileSource::parseURLOrTileset(value); + if (!urlOrTileset) { + return nullptr; + } + + uint16_t tileSize = util::tileSize; + if (value.HasMember("tileSize")) { + const JSValue& tileSizeVal = value["tileSize"]; + if (tileSizeVal.IsNumber() && tileSizeVal.GetUint64() <= std::numeric_limits::max()) { + tileSize = tileSizeVal.GetUint64(); + } else { + Log::Error(Event::ParseStyle, "invalid tileSize"); + return nullptr; + } + } + + return std::make_unique(std::move(id), std::move(*urlOrTileset), tileSize); +} + RasterSource::RasterSource(std::string id_, - std::string url_, - uint16_t tileSize_, - std::unique_ptr&& tileset_) - : TileSource(SourceType::Raster, std::move(id_), std::move(url_), tileSize_, std::move(tileset_)) { + variant urlOrTileset_, + uint16_t tileSize_) + : TileSource(SourceType::Raster, std::move(id_), std::move(urlOrTileset_), tileSize_) { } std::unique_ptr RasterSource::createTile(const OverscaledTileID& tileID, const UpdateParameters& parameters) { - return std::make_unique(tileID, parameters, *tileset); + return std::make_unique(tileID, parameters, tileset); } } // namespace style diff --git a/src/mbgl/style/sources/raster_source.hpp b/src/mbgl/style/sources/raster_source.hpp index 4702052b774..27b2276df5c 100644 --- a/src/mbgl/style/sources/raster_source.hpp +++ b/src/mbgl/style/sources/raster_source.hpp @@ -7,10 +7,9 @@ namespace style { class RasterSource : public TileSource { public: - RasterSource(std::string id, - std::string url, - uint16_t tileSize, - std::unique_ptr&&); + static std::unique_ptr parse(std::string id, const JSValue&); + + RasterSource(std::string id, variant, uint16_t tileSize); private: std::unique_ptr createTile(const OverscaledTileID&, const UpdateParameters&) final; diff --git a/src/mbgl/style/sources/vector_source.cpp b/src/mbgl/style/sources/vector_source.cpp index 888c1896e13..3f8f840b380 100644 --- a/src/mbgl/style/sources/vector_source.cpp +++ b/src/mbgl/style/sources/vector_source.cpp @@ -4,15 +4,21 @@ namespace mbgl { namespace style { -VectorSource::VectorSource(std::string id_, - std::string url_, - std::unique_ptr&& tileset_) - : TileSource(SourceType::Vector, std::move(id_), std::move(url_), util::tileSize, std::move(tileset_)) { +std::unique_ptr VectorSource::parse(std::string id, const JSValue& value) { + optional> urlOrTileset = TileSource::parseURLOrTileset(value); + if (!urlOrTileset) { + return nullptr; + } + return std::make_unique(std::move(id), std::move(*urlOrTileset)); +} + +VectorSource::VectorSource(std::string id_, variant urlOrTileset_) + : TileSource(SourceType::Vector, std::move(id_), std::move(urlOrTileset_), util::tileSize) { } std::unique_ptr VectorSource::createTile(const OverscaledTileID& tileID, const UpdateParameters& parameters) { - return std::make_unique(tileID, id, parameters, *tileset); + return std::make_unique(tileID, id, parameters, tileset); } } // namespace style diff --git a/src/mbgl/style/sources/vector_source.hpp b/src/mbgl/style/sources/vector_source.hpp index c96b7bd1e42..ced7d80471f 100644 --- a/src/mbgl/style/sources/vector_source.hpp +++ b/src/mbgl/style/sources/vector_source.hpp @@ -7,9 +7,9 @@ namespace style { class VectorSource : public TileSource { public: - VectorSource(std::string id, - std::string url, - std::unique_ptr&&); + static std::unique_ptr parse(std::string id, const JSValue&); + + VectorSource(std::string id, variant); private: std::unique_ptr createTile(const OverscaledTileID&, const UpdateParameters&) final; diff --git a/src/mbgl/style/tile_source.cpp b/src/mbgl/style/tile_source.cpp index a4deb17c4f6..022693f4990 100644 --- a/src/mbgl/style/tile_source.cpp +++ b/src/mbgl/style/tile_source.cpp @@ -2,31 +2,163 @@ #include #include #include +#include #include +#include + +#include +#include + +#include namespace mbgl { namespace style { +namespace { + +void parseTileJSONMember(const JSValue& value, std::vector& target, const char* name) { + if (!value.HasMember(name)) { + return; + } + + const JSValue& property = value[name]; + if (!property.IsArray()) { + return; + } + + for (rapidjson::SizeType i = 0; i < property.Size(); i++) { + if (!property[i].IsString()) { + return; + } + } + + for (rapidjson::SizeType i = 0; i < property.Size(); i++) { + target.emplace_back(std::string(property[i].GetString(), property[i].GetStringLength())); + } +} + +void parseTileJSONMember(const JSValue& value, std::string& target, const char* name) { + if (!value.HasMember(name)) { + return; + } + + const JSValue& property = value[name]; + if (!property.IsString()) { + return; + } + + target = { property.GetString(), property.GetStringLength() }; +} + +void parseTileJSONMember(const JSValue& value, uint8_t& target, const char* name) { + if (!value.HasMember(name)) { + return; + } + + const JSValue& property = value[name]; + if (!property.IsUint()) { + return; + } + + unsigned int uint = property.GetUint(); + if (uint > std::numeric_limits::max()) { + return; + } + + target = uint; +} + +void parseTileJSONMember(const JSValue& value, std::array& target, const char* name) { + if (!value.HasMember(name)) { + return; + } + + const JSValue& property = value[name]; + if (!property.IsArray() || property.Size() > 4) { + return; + } + + for (rapidjson::SizeType i = 0; i < property.Size(); i++) { + if (!property[i].IsNumber()) { + return; + } + } + + for (rapidjson::SizeType i = 0; i < property.Size(); i++) { + target[i] = property[i].GetDouble(); + } +} + +Tileset parseTileJSON(const JSValue& value) { + Tileset result; + + parseTileJSONMember(value, result.tiles, "tiles"); + parseTileJSONMember(value, result.zoomRange.min, "minzoom"); + parseTileJSONMember(value, result.zoomRange.max, "maxzoom"); + parseTileJSONMember(value, result.attribution, "attribution"); + + std::array array; + parseTileJSONMember(value, array, "center"); + result.center = { array[0], array[1] }; + result.zoom = array[2]; + parseTileJSONMember(value, array, "bounds"); + result.bounds = LatLngBounds::hull({ array[0], array[1] }, { array[2], array[3] }); + + return result; +} + +} // end namespace + +Tileset TileSource::parseTileJSON(const std::string& json, const std::string& sourceURL, SourceType type, uint16_t tileSize) { + rapidjson::GenericDocument, rapidjson::CrtAllocator> document; + document.Parse<0>(json.c_str()); + + if (document.HasParseError()) { + std::stringstream message; + message << document.GetErrorOffset() << " - " << rapidjson::GetParseError_En(document.GetParseError()); + throw std::runtime_error(message.str()); + } + + Tileset result = mbgl::style::parseTileJSON(document); + + // TODO: Remove this hack by delivering proper URLs in the TileJSON to begin with. + if (util::mapbox::isMapboxURL(sourceURL)) { + for (auto& url : result.tiles) { + url = util::mapbox::canonicalizeTileURL(url, type, tileSize); + } + } + + return result; +} + +optional> TileSource::parseURLOrTileset(const JSValue& value) { + if (!value.HasMember("url")) { + return { mbgl::style::parseTileJSON(value) }; + } + + const JSValue& urlVal = value["url"]; + if (!urlVal.IsString()) { + Log::Error(Event::ParseStyle, "source url must be a string"); + return {}; + } + + return { std::string(urlVal.GetString(), urlVal.GetStringLength()) }; +} + TileSource::TileSource(SourceType type_, std::string id_, - std::string url_, - uint16_t tileSize_, - std::unique_ptr&& tileset_) + variant urlOrTileset_, + uint16_t tileSize_) : Source(type_, std::move(id_)), - tileSize(tileSize_), - url(std::move(url_)), - tileset(std::move(tileset_)) { + urlOrTileset(std::move(urlOrTileset_)), + tileSize(tileSize_) { } TileSource::~TileSource() = default; -Range TileSource::getZoomRange() { - return tileset->zoomRange; -} - void TileSource::load(FileSource& fileSource) { - if (url.empty()) { - // If the URL is empty, the TileJSON was specified inline in the stylesheet. + if (urlOrTileset.is()) { + tileset = urlOrTileset.get(); loaded = true; return; } @@ -35,8 +167,8 @@ void TileSource::load(FileSource& fileSource) { return; } - // URL may either be a TileJSON file, or a GeoJSON file. - req = fileSource.request(Resource::source(url), [this](Response res) { + const std::string& url = urlOrTileset.get(); + req = fileSource.request(Resource::source(url), [this, url](Response res) { if (res.error) { observer->onSourceError(*this, std::make_exception_ptr(std::runtime_error(res.error->message))); } else if (res.notModified) { @@ -44,20 +176,20 @@ void TileSource::load(FileSource& fileSource) { } else if (res.noContent) { observer->onSourceError(*this, std::make_exception_ptr(std::runtime_error("unexpectedly empty TileJSON"))); } else { - std::unique_ptr newTileset; + Tileset newTileset; // Create a new copy of the Tileset object that holds the base values we've parsed // from the stylesheet. Then merge in the values parsed from the TileJSON we retrieved // via the URL. try { - newTileset = style::parseTileJSON(*res.data, url, type, tileSize); + newTileset = parseTileJSON(*res.data, url, type, tileSize); } catch (...) { observer->onSourceError(*this, std::current_exception()); return; } // Check whether previous information specifies different tile - if (tileset && tileset->tiles != newTileset->tiles) { + if (tileset.tiles != newTileset.tiles) { // Tile URLs changed: force tiles to be reloaded. invalidateTiles(); @@ -76,12 +208,18 @@ void TileSource::load(FileSource& fileSource) { // Center/bounds changed: We're not using these values currently } - tileset = std::move(newTileset); + tileset = newTileset; loaded = true; + observer->onSourceLoaded(*this); } }); } +Range TileSource::getZoomRange() { + assert(loaded); + return tileset.zoomRange; +} + } // namespace style } // namespace mbgl diff --git a/src/mbgl/style/tile_source.hpp b/src/mbgl/style/tile_source.hpp index bc97713a047..66d956a6ef6 100644 --- a/src/mbgl/style/tile_source.hpp +++ b/src/mbgl/style/tile_source.hpp @@ -1,10 +1,13 @@ #pragma once #include +#include +#include +#include +#include namespace mbgl { -class Tileset; class AsyncRequest; namespace style { @@ -15,25 +18,33 @@ namespace style { */ class TileSource : public Source { public: + // A tile source can either specify a URL to TileJSON, or inline TileJSON. + static optional> parseURLOrTileset(const JSValue&); + static Tileset parseTileJSON(const std::string& json, const std::string& sourceURL, SourceType, uint16_t tileSize); + TileSource(SourceType, std::string id, - std::string url, - uint16_t tileSize, - std::unique_ptr&&); + variant urlOrTileset, + uint16_t tileSize); ~TileSource() override; void load(FileSource&) final; - const std::string& getURL() const { return url; } - uint16_t getTileSize() const final { return tileSize; } - const Tileset* getTileset() const { return tileset.get(); } + uint16_t getTileSize() const final { + return tileSize; + } + + const variant& getURLOrTileset() const { + return urlOrTileset; + } protected: Range getZoomRange() final; + const variant urlOrTileset; const uint16_t tileSize; - const std::string url; - std::unique_ptr tileset; + + Tileset tileset; std::unique_ptr req; }; diff --git a/test/style/source.cpp b/test/style/source.cpp index 82763c99f18..9de208f3431 100644 --- a/test/style/source.cpp +++ b/test/style/source.cpp @@ -87,7 +87,7 @@ TEST(Source, LoadingFail) { test.end(); }; - VectorSource source("source", "url", nullptr); + VectorSource source("source", "url"); source.setObserver(&test.observer); source.load(test.fileSource); @@ -110,7 +110,7 @@ TEST(Source, LoadingCorrupt) { test.end(); }; - VectorSource source("source", "url", nullptr); + VectorSource source("source", "url"); source.setObserver(&test.observer); source.load(test.fileSource); @@ -135,10 +135,10 @@ TEST(Source, RasterTileEmpty) { FAIL() << "Should never be called"; }; - auto tileset = std::make_unique(); - tileset->tiles = { "tiles" }; + Tileset tileset; + tileset.tiles = { "tiles" }; - RasterSource source("source", "", 512, std::move(tileset)); + RasterSource source("source", tileset, 512); source.setObserver(&test.observer); source.load(test.fileSource); source.update(test.updateParameters); @@ -164,10 +164,10 @@ TEST(Source, VectorTileEmpty) { FAIL() << "Should never be called"; }; - auto tileset = std::make_unique(); - tileset->tiles = { "tiles" }; + Tileset tileset; + tileset.tiles = { "tiles" }; - VectorSource source("source", "", std::move(tileset)); + VectorSource source("source", tileset); source.setObserver(&test.observer); source.load(test.fileSource); source.update(test.updateParameters); @@ -193,10 +193,10 @@ TEST(Source, RasterTileFail) { test.end(); }; - auto tileset = std::make_unique(); - tileset->tiles = { "tiles" }; + Tileset tileset; + tileset.tiles = { "tiles" }; - RasterSource source("source", "", 512, std::move(tileset)); + RasterSource source("source", tileset, 512); source.setObserver(&test.observer); source.load(test.fileSource); source.update(test.updateParameters); @@ -222,10 +222,10 @@ TEST(Source, VectorTileFail) { test.end(); }; - auto tileset = std::make_unique(); - tileset->tiles = { "tiles" }; + Tileset tileset; + tileset.tiles = { "tiles" }; - VectorSource source("source", "", std::move(tileset)); + VectorSource source("source", tileset); source.setObserver(&test.observer); source.load(test.fileSource); source.update(test.updateParameters); @@ -250,10 +250,10 @@ TEST(Source, RasterTileCorrupt) { test.end(); }; - auto tileset = std::make_unique(); - tileset->tiles = { "tiles" }; + Tileset tileset; + tileset.tiles = { "tiles" }; - RasterSource source("source", "", 512, std::move(tileset)); + RasterSource source("source", tileset, 512); source.setObserver(&test.observer); source.load(test.fileSource); source.update(test.updateParameters); @@ -282,10 +282,10 @@ TEST(Source, VectorTileCorrupt) { layer->setSource("source", "water"); test.style.addLayer(std::move(layer)); - auto tileset = std::make_unique(); - tileset->tiles = { "tiles" }; + Tileset tileset; + tileset.tiles = { "tiles" }; - VectorSource source("source", "", std::move(tileset)); + VectorSource source("source", tileset); source.setObserver(&test.observer); source.load(test.fileSource); source.update(test.updateParameters); @@ -309,10 +309,10 @@ TEST(Source, RasterTileCancel) { FAIL() << "Should never be called"; }; - auto tileset = std::make_unique(); - tileset->tiles = { "tiles" }; + Tileset tileset; + tileset.tiles = { "tiles" }; - RasterSource source("source", "", 512, std::move(tileset)); + RasterSource source("source", tileset, 512); source.setObserver(&test.observer); source.load(test.fileSource); source.update(test.updateParameters); @@ -336,10 +336,10 @@ TEST(Source, VectorTileCancel) { FAIL() << "Should never be called"; }; - auto tileset = std::make_unique(); - tileset->tiles = { "tiles" }; + Tileset tileset; + tileset.tiles = { "tiles" }; - VectorSource source("source", "", std::move(tileset)); + VectorSource source("source", tileset); source.setObserver(&test.observer); source.load(test.fileSource); source.update(test.updateParameters); diff --git a/test/style/style_parser.cpp b/test/style/style_parser.cpp index f3fef243521..b7806c11cc1 100644 --- a/test/style/style_parser.cpp +++ b/test/style/style_parser.cpp @@ -87,36 +87,6 @@ INSTANTIATE_TEST_CASE_P(StyleParser, StyleParserTest, ::testing::ValuesIn([] { return names; }())); -TEST(StyleParser, ParseTileJSONRaster) { - auto result = style::parseTileJSON( - util::read_file("test/fixtures/style_parser/tilejson.raster.json"), - "mapbox://mapbox.satellite", - SourceType::Raster, - 256); - - EXPECT_EQ(0, result->zoomRange.min); - EXPECT_EQ(15, result->zoomRange.max); - EXPECT_EQ("attribution", result->attribution); -#if !defined(__ANDROID__) && !defined(__APPLE__) - EXPECT_EQ("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.webp", result->tiles[0]); -#else - EXPECT_EQ("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.png", result->tiles[0]); -#endif -} - -TEST(StyleParser, ParseTileJSONVector) { - auto result = style::parseTileJSON( - util::read_file("test/fixtures/style_parser/tilejson.vector.json"), - "mapbox://mapbox.streets", - SourceType::Vector, - 256); - - EXPECT_EQ(0, result->zoomRange.min); - EXPECT_EQ(15, result->zoomRange.max); - EXPECT_EQ("attribution", result->attribution); - EXPECT_EQ("mapbox://tiles/mapbox.streets/{z}/{x}/{y}.vector.pbf", result->tiles[0]); -} - TEST(StyleParser, FontStacks) { style::Parser parser; parser.parse(util::read_file("test/fixtures/style_parser/font_stacks.json")); diff --git a/test/style/tile_source.cpp b/test/style/tile_source.cpp new file mode 100644 index 00000000000..e0709e3be13 --- /dev/null +++ b/test/style/tile_source.cpp @@ -0,0 +1,37 @@ +#include + +#include +#include + +using namespace mbgl; +using namespace mbgl::style; + +TEST(TileSource, ParseTileJSONRaster) { + auto result = TileSource::parseTileJSON( + util::read_file("test/fixtures/style_parser/tilejson.raster.json"), + "mapbox://mapbox.satellite", + SourceType::Raster, + 256); + + EXPECT_EQ(0, result.zoomRange.min); + EXPECT_EQ(15, result.zoomRange.max); + EXPECT_EQ("attribution", result.attribution); +#if !defined(__ANDROID__) && !defined(__APPLE__) + EXPECT_EQ("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.webp", result.tiles[0]); +#else + EXPECT_EQ("mapbox://tiles/mapbox.satellite/{z}/{x}/{y}{ratio}.png", result.tiles[0]); +#endif +} + +TEST(TileSource, ParseTileJSONVector) { + auto result = TileSource::parseTileJSON( + util::read_file("test/fixtures/style_parser/tilejson.vector.json"), + "mapbox://mapbox.streets", + SourceType::Vector, + 256); + + EXPECT_EQ(0, result.zoomRange.min); + EXPECT_EQ(15, result.zoomRange.max); + EXPECT_EQ("attribution", result.attribution); + EXPECT_EQ("mapbox://tiles/mapbox.streets/{z}/{x}/{y}.vector.pbf", result.tiles[0]); +} diff --git a/test/test.gypi b/test/test.gypi index d7023874f06..da605b6fd87 100644 --- a/test/test.gypi +++ b/test/test.gypi @@ -73,6 +73,7 @@ 'style/source.cpp', 'style/style.cpp', 'style/style_layer.cpp', + 'style/tile_source.cpp', 'style/filter.cpp', 'style/functions.cpp', 'style/style_parser.cpp', From 41760b5696e71c3d201d94d5c8b2423c1f348446 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Tue, 14 Jun 2016 12:46:29 -0700 Subject: [PATCH 2/3] [core] Use variant in GeoJSON source --- .../default/mbgl/storage/offline_download.cpp | 10 +++-- src/mbgl/style/sources/geojson_source.cpp | 43 ++++++++----------- src/mbgl/style/sources/geojson_source.hpp | 24 ++++++----- src/mbgl/tile/geojson_tile.cpp | 6 +-- src/mbgl/tile/geojson_tile.hpp | 2 +- 5 files changed, 41 insertions(+), 44 deletions(-) diff --git a/platform/default/mbgl/storage/offline_download.cpp b/platform/default/mbgl/storage/offline_download.cpp index 3d0ebaa68f8..3f21b8571ea 100644 --- a/platform/default/mbgl/storage/offline_download.cpp +++ b/platform/default/mbgl/storage/offline_download.cpp @@ -127,7 +127,9 @@ OfflineRegionStatus OfflineDownload::getStatus() const { case SourceType::GeoJSON: { style::GeoJSONSource* geojsonSource = static_cast(source.get()); - if (!geojsonSource->getURL().empty()) { + const variant& urlOrGeoJSON = geojsonSource->getURLOrGeoJSON(); + + if (urlOrGeoJSON.is()) { result.requiredResourceCount += 1; } break; @@ -188,8 +190,10 @@ void OfflineDownload::activateDownload() { case SourceType::GeoJSON: { style::GeoJSONSource* geojsonSource = static_cast(source.get()); - if (!geojsonSource->getURL().empty()) { - ensureResource(Resource::source(geojsonSource->getURL())); + const variant& urlOrGeoJSON = geojsonSource->getURLOrGeoJSON(); + + if (urlOrGeoJSON.is()) { + ensureResource(Resource::source(urlOrGeoJSON.get())); } break; } diff --git a/src/mbgl/style/sources/geojson_source.cpp b/src/mbgl/style/sources/geojson_source.cpp index 00c344cf0ba..e06b00ec99f 100644 --- a/src/mbgl/style/sources/geojson_source.cpp +++ b/src/mbgl/style/sources/geojson_source.cpp @@ -15,7 +15,7 @@ namespace mbgl { namespace style { -std::unique_ptr parseGeoJSON(const JSValue& value) { +std::unique_ptr GeoJSONSource::parseGeoJSON(const JSValue& value) { using namespace mapbox::geojsonvt; Options options; @@ -32,11 +32,7 @@ std::unique_ptr parseGeoJSON(const JSValue& value) } } -std::unique_ptr GeoJSONSource::parse(const std::string& id, - const JSValue& value) { - std::unique_ptr geojsonvt; - std::string url; - +std::unique_ptr GeoJSONSource::parse(const std::string& id, const JSValue& value) { // We should probably split this up to have URLs in the url property, and actual data // in the data property. Until then, we're going to detect the content based on the // object type. @@ -47,36 +43,24 @@ std::unique_ptr GeoJSONSource::parse(const std::string& id, const JSValue& dataVal = value["data"]; if (dataVal.IsString()) { - // We need to load an external GeoJSON file - url = { dataVal.GetString(), dataVal.GetStringLength() }; + return std::make_unique(id, std::string(dataVal.GetString(), dataVal.GetStringLength())); } else if (dataVal.IsObject()) { - // We need to parse dataVal as a GeoJSON object - geojsonvt = parseGeoJSON(dataVal); + return std::make_unique(id, parseGeoJSON(dataVal)); } else { Log::Error(Event::ParseStyle, "GeoJSON data must be a URL or an object"); return nullptr; } - - return std::make_unique(id, url, std::move(geojsonvt)); } -GeoJSONSource::GeoJSONSource(std::string id_, - std::string url_, - std::unique_ptr&& geojsonvt_) +GeoJSONSource::GeoJSONSource(std::string id_, variant urlOrGeoJSON_) : Source(SourceType::GeoJSON, std::move(id_)), - url(std::move(url_)), - geojsonvt(std::move(geojsonvt_)) { + urlOrGeoJSON(std::move(urlOrGeoJSON_)) { } GeoJSONSource::~GeoJSONSource() = default; -Range GeoJSONSource::getZoomRange() { - return { 0, geojsonvt->options.maxZoom }; -} - void GeoJSONSource::load(FileSource& fileSource) { - if (url.empty()) { - // If the URL is empty, the GeoJSON was specified inline in the stylesheet. + if (urlOrGeoJSON.is()) { loaded = true; return; } @@ -85,6 +69,7 @@ void GeoJSONSource::load(FileSource& fileSource) { return; } + const std::string& url = urlOrGeoJSON.get(); req = fileSource.request(Resource::source(url), [this](Response res) { if (res.error) { observer->onSourceError(*this, std::make_exception_ptr(std::runtime_error(res.error->message))); @@ -103,19 +88,25 @@ void GeoJSONSource::load(FileSource& fileSource) { return; } - geojsonvt = style::parseGeoJSON(d); - invalidateTiles(); + urlOrGeoJSON = parseGeoJSON(d); loaded = true; + observer->onSourceLoaded(*this); } }); } +Range GeoJSONSource::getZoomRange() { + assert(loaded); + return { 0, urlOrGeoJSON.get()->options.maxZoom }; +} + std::unique_ptr GeoJSONSource::createTile(const OverscaledTileID& tileID, const UpdateParameters& parameters) { - return std::make_unique(tileID, id, parameters, geojsonvt.get()); + assert(loaded); + return std::make_unique(tileID, id, parameters, *urlOrGeoJSON.get()); } } // namespace style diff --git a/src/mbgl/style/sources/geojson_source.hpp b/src/mbgl/style/sources/geojson_source.hpp index 00d245c8788..490dae48b89 100644 --- a/src/mbgl/style/sources/geojson_source.hpp +++ b/src/mbgl/style/sources/geojson_source.hpp @@ -1,8 +1,8 @@ #pragma once #include - #include +#include namespace mapbox { namespace geojsonvt { @@ -18,25 +18,29 @@ namespace style { class GeoJSONSource : public Source { public: - static std::unique_ptr parse(const std::string& id, - const JSValue&); + using GeoJSON = std::unique_ptr; + + static std::unique_ptr parse(const std::string& id, const JSValue&); + static GeoJSON parseGeoJSON(const JSValue&); - GeoJSONSource(std::string id, - std::string url, - std::unique_ptr&&); + GeoJSONSource(std::string id, variant urlOrGeoJSON); ~GeoJSONSource() final; void load(FileSource&) final; - const std::string& getURL() const { return url; } + uint16_t getTileSize() const final { + return util::tileSize; + } + + const variant& getURLOrGeoJSON() const { + return urlOrGeoJSON; + } private: - uint16_t getTileSize() const final { return util::tileSize; } Range getZoomRange() final; std::unique_ptr createTile(const OverscaledTileID&, const UpdateParameters&) final; - const std::string url; - std::unique_ptr geojsonvt; + variant urlOrGeoJSON; std::unique_ptr req; }; diff --git a/src/mbgl/tile/geojson_tile.cpp b/src/mbgl/tile/geojson_tile.cpp index f7bf969e7cc..9334cf2feca 100644 --- a/src/mbgl/tile/geojson_tile.cpp +++ b/src/mbgl/tile/geojson_tile.cpp @@ -109,11 +109,9 @@ std::unique_ptr convertTile(const mapbox::geojsonvt::Tile& tile GeoJSONTile::GeoJSONTile(const OverscaledTileID& overscaledTileID, std::string sourceID, const style::UpdateParameters& parameters, - mapbox::geojsonvt::GeoJSONVT* geojsonvt) + mapbox::geojsonvt::GeoJSONVT& geojsonvt) : GeometryTile(overscaledTileID, sourceID, parameters.style, parameters.mode) { - if (geojsonvt) { - setData(convertTile(geojsonvt->getTile(id.canonical.z, id.canonical.x, id.canonical.y))); - } + setData(convertTile(geojsonvt.getTile(id.canonical.z, id.canonical.x, id.canonical.y))); } void GeoJSONTile::setNecessity(Necessity) {} diff --git a/src/mbgl/tile/geojson_tile.hpp b/src/mbgl/tile/geojson_tile.hpp index 8cccda77b82..09fdd1ec9b9 100644 --- a/src/mbgl/tile/geojson_tile.hpp +++ b/src/mbgl/tile/geojson_tile.hpp @@ -19,7 +19,7 @@ class GeoJSONTile : public GeometryTile { GeoJSONTile(const OverscaledTileID&, std::string sourceID, const style::UpdateParameters&, - mapbox::geojsonvt::GeoJSONVT*); + mapbox::geojsonvt::GeoJSONVT&); void setNecessity(Necessity) final; }; From d48333f26d9111d8f4f8420b477c36cece0d14a5 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Tue, 14 Jun 2016 12:46:29 -0700 Subject: [PATCH 3/3] [core] Remove unused and mis-parsed properties from Tileset latitude and longitude were reversed. Fortunately they were not used. --- src/mbgl/style/tile_source.cpp | 28 ---------------------------- src/mbgl/util/tileset.hpp | 8 ++------ 2 files changed, 2 insertions(+), 34 deletions(-) diff --git a/src/mbgl/style/tile_source.cpp b/src/mbgl/style/tile_source.cpp index 022693f4990..0824693331a 100644 --- a/src/mbgl/style/tile_source.cpp +++ b/src/mbgl/style/tile_source.cpp @@ -68,27 +68,6 @@ void parseTileJSONMember(const JSValue& value, uint8_t& target, const char* name target = uint; } -void parseTileJSONMember(const JSValue& value, std::array& target, const char* name) { - if (!value.HasMember(name)) { - return; - } - - const JSValue& property = value[name]; - if (!property.IsArray() || property.Size() > 4) { - return; - } - - for (rapidjson::SizeType i = 0; i < property.Size(); i++) { - if (!property[i].IsNumber()) { - return; - } - } - - for (rapidjson::SizeType i = 0; i < property.Size(); i++) { - target[i] = property[i].GetDouble(); - } -} - Tileset parseTileJSON(const JSValue& value) { Tileset result; @@ -97,13 +76,6 @@ Tileset parseTileJSON(const JSValue& value) { parseTileJSONMember(value, result.zoomRange.max, "maxzoom"); parseTileJSONMember(value, result.attribution, "attribution"); - std::array array; - parseTileJSONMember(value, array, "center"); - result.center = { array[0], array[1] }; - result.zoom = array[2]; - parseTileJSONMember(value, array, "bounds"); - result.bounds = LatLngBounds::hull({ array[0], array[1] }, { array[2], array[3] }); - return result; } diff --git a/src/mbgl/util/tileset.hpp b/src/mbgl/util/tileset.hpp index 7e53850dfe9..8a7fbe9b73d 100644 --- a/src/mbgl/util/tileset.hpp +++ b/src/mbgl/util/tileset.hpp @@ -1,10 +1,7 @@ #pragma once -#include #include -#include -#include #include #include #include @@ -16,9 +13,8 @@ class Tileset { std::vector tiles; Range zoomRange { 0, 22 }; std::string attribution; - LatLng center; - double zoom = 0; - LatLngBounds bounds = LatLngBounds::world(); + + // TileJSON also includes center, zoom, and bounds, but they are not used by mbgl. }; } // namespace mbgl