From 0ee9f21c56a69513ba2dce3108f5540afb045c6d Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Thu, 9 Jul 2015 15:57:15 +0300 Subject: [PATCH] Report Sprite parsing errors to Observers Parsing errors will be reported to Observers. In practice this will bubble all the way to the Map object. This will make possible to identify possible corrupted data instead of rendering without sprites like we do in case of semantic errors. --- src/mbgl/annotation/sprite_parser.cpp | 15 +++++++-------- src/mbgl/annotation/sprite_parser.hpp | 8 +++++++- src/mbgl/map/sprite.cpp | 16 ++++++++++++---- src/mbgl/map/sprite.hpp | 3 ++- src/mbgl/style/style.cpp | 5 ++--- src/mbgl/style/style.hpp | 2 +- test/annotations/sprite_atlas.cpp | 13 +++++++++---- test/annotations/sprite_parser.cpp | 27 +++++++++------------------ 8 files changed, 49 insertions(+), 40 deletions(-) diff --git a/src/mbgl/annotation/sprite_parser.cpp b/src/mbgl/annotation/sprite_parser.cpp index 0f9ba97c9..f867b97f4 100644 --- a/src/mbgl/annotation/sprite_parser.cpp +++ b/src/mbgl/annotation/sprite_parser.cpp @@ -9,6 +9,7 @@ #include #include +#include namespace mbgl { @@ -99,7 +100,7 @@ inline bool getBoolean(const rapidjson::Value& value, const char* name, const bo } // namespace -Sprites parseSprite(const std::string& image, const std::string& json) { +SpriteParseResult parseSprite(const std::string& image, const std::string& json) { using namespace rapidjson; Sprites sprites; @@ -107,20 +108,18 @@ Sprites parseSprite(const std::string& image, const std::string& json) { // Parse the sprite image. const util::Image raster(image); if (!raster) { - Log::Warning(Event::Sprite, "Could not parse sprite image"); - return sprites; + return std::string("Could not parse sprite image"); } Document doc; doc.Parse<0>(json.c_str()); if (doc.HasParseError()) { - Log::Warning(Event::Sprite, std::string{ "Failed to parse JSON: " } + doc.GetParseError() + - " at offset " + std::to_string(doc.GetErrorOffset())); - return sprites; + std::stringstream message; + message << "Failed to parse JSON: " << doc.GetParseError() << " at offset " << doc.GetErrorOffset(); + return message.str(); } else if (!doc.IsObject()) { - Log::Warning(Event::Sprite, "Sprite JSON root must be an object"); - return sprites; + return std::string("Sprite JSON root must be an object"); } else { for (Value::ConstMemberIterator itr = doc.MemberBegin(); itr != doc.MemberEnd(); ++itr) { const std::string name = { itr->name.GetString(), itr->name.GetStringLength() }; diff --git a/src/mbgl/annotation/sprite_parser.hpp b/src/mbgl/annotation/sprite_parser.hpp index 71228aacf..d7e995335 100644 --- a/src/mbgl/annotation/sprite_parser.hpp +++ b/src/mbgl/annotation/sprite_parser.hpp @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -31,8 +32,13 @@ SpriteImagePtr createSpriteImage(const util::Image& image, using Sprites = std::map; + +using SpriteParseResult = mapbox::util::variant< + Sprites, // success + std::string>; // error + // Parses an image and an associated JSON file and returns the sprite objects. -Sprites parseSprite(const std::string& image, const std::string& json); +SpriteParseResult parseSprite(const std::string& image, const std::string& json); } // namespace mbgl diff --git a/src/mbgl/map/sprite.cpp b/src/mbgl/map/sprite.cpp index 900b2efa4..5f6190f58 100644 --- a/src/mbgl/map/sprite.cpp +++ b/src/mbgl/map/sprite.cpp @@ -85,11 +85,19 @@ Sprite::~Sprite() { void Sprite::emitSpriteLoadedIfComplete() { assert(loader); - if (loader->loadedImage && loader->loadedJSON && observer) { - std::unique_ptr data(std::move(loader->data)); - loader.reset(); - observer->onSpriteLoaded(std::move(data)); + if (!loader->loadedImage || !loader->loadedJSON || !observer) { + return; + } + + std::unique_ptr data(std::move(loader->data)); + loader.reset(); + + auto result = parseSprite(data->image, data->json); + if (result.is()) { + observer->onSpriteLoaded(result.get()); + } else { + emitSpriteLoadingFailed(result.get()); } } diff --git a/src/mbgl/map/sprite.hpp b/src/mbgl/map/sprite.hpp index d8f91fd9c..b2020e7d6 100644 --- a/src/mbgl/map/sprite.hpp +++ b/src/mbgl/map/sprite.hpp @@ -1,6 +1,7 @@ #ifndef MBGL_STYLE_SPRITE #define MBGL_STYLE_SPRITE +#include #include #include #include @@ -27,7 +28,7 @@ public: public: virtual ~Observer() = default; - virtual void onSpriteLoaded(std::unique_ptr) = 0; + virtual void onSpriteLoaded(const Sprites& sprites) = 0; virtual void onSpriteLoadingFailed(std::exception_ptr error) = 0; }; diff --git a/src/mbgl/style/style.cpp b/src/mbgl/style/style.cpp index 48106d8f3..e091c92f5 100644 --- a/src/mbgl/style/style.cpp +++ b/src/mbgl/style/style.cpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -191,9 +190,9 @@ void Style::onTileLoadingFailed(std::exception_ptr error) { emitResourceLoadingFailed(error); } -void Style::onSpriteLoaded(std::unique_ptr spriteData) { +void Style::onSpriteLoaded(const Sprites& sprites) { // Add all sprite images to the SpriteStore object - spriteStore->setSprites(parseSprite(spriteData->image, spriteData->json)); + spriteStore->setSprites(sprites); shouldReparsePartialTiles = true; emitTileDataChanged(); diff --git a/src/mbgl/style/style.hpp b/src/mbgl/style/style.hpp index 359ab4d83..d6b762a83 100644 --- a/src/mbgl/style/style.hpp +++ b/src/mbgl/style/style.hpp @@ -89,7 +89,7 @@ private: void onTileLoadingFailed(std::exception_ptr error) override; // Sprite::Observer implementation. - void onSpriteLoaded(std::unique_ptr) override; + void onSpriteLoaded(const Sprites& sprites) override; void onSpriteLoadingFailed(std::exception_ptr error) override; void emitTileDataChanged(); diff --git a/test/annotations/sprite_atlas.cpp b/test/annotations/sprite_atlas.cpp index 16140b051..0eece6c47 100644 --- a/test/annotations/sprite_atlas.cpp +++ b/test/annotations/sprite_atlas.cpp @@ -12,9 +12,11 @@ using namespace mbgl; TEST(Annotations, SpriteAtlas) { FixtureLog log; + auto spriteParseResult = parseSprite(util::read_file("test/fixtures/annotations/emerald.png"), + util::read_file("test/fixtures/annotations/emerald.json")); + SpriteStore store; - store.setSprites(parseSprite(util::read_file("test/fixtures/annotations/emerald.png"), - util::read_file("test/fixtures/annotations/emerald.json"))); + store.setSprites(spriteParseResult.get()); SpriteAtlas atlas(63, 112, 1, store); @@ -85,9 +87,12 @@ TEST(Annotations, SpriteAtlas) { } TEST(Annotations, SpriteAtlasSize) { + auto spriteParseResult = parseSprite(util::read_file("test/fixtures/annotations/emerald.png"), + util::read_file("test/fixtures/annotations/emerald.json")); + SpriteStore store; - store.setSprites(parseSprite(util::read_file("test/fixtures/annotations/emerald.png"), - util::read_file("test/fixtures/annotations/emerald.json"))); + store.setSprites(spriteParseResult.get()); + SpriteAtlas atlas(63, 112, 1.4, store); EXPECT_DOUBLE_EQ(1.4f, atlas.getPixelRatio()); diff --git a/test/annotations/sprite_parser.cpp b/test/annotations/sprite_parser.cpp index 280a19c50..703315258 100644 --- a/test/annotations/sprite_parser.cpp +++ b/test/annotations/sprite_parser.cpp @@ -117,7 +117,7 @@ TEST(Annotations, SpriteParsing) { const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png"); const auto json_1x = util::read_file("test/fixtures/annotations/emerald.json"); - const auto images = parseSprite(image_1x, json_1x); + const auto images = parseSprite(image_1x, json_1x).get(); std::set names; std::transform(images.begin(), images.end(), std::inserter(names, names.begin()), @@ -210,22 +210,13 @@ TEST(Annotations, SpriteParsing) { } TEST(Annotations, SpriteParsingInvalidJSON) { - FixtureLog log; - const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png"); const auto json_1x = R"JSON({ "image": " })JSON"; - const auto images = parseSprite(image_1x, json_1x); - EXPECT_EQ(0u, images.size()); + const auto error = parseSprite(image_1x, json_1x).get(); - EXPECT_EQ( - 1u, - log.count({ - EventSeverity::Warning, - Event::Sprite, - int64_t(-1), - "Failed to parse JSON: lacks ending quotation before the end of string at offset 10", - })); + EXPECT_EQ(error, + std::string("Failed to parse JSON: lacks ending quotation before the end of string at offset 10")); } TEST(Annotations, SpriteParsingEmptyImage) { @@ -234,7 +225,7 @@ TEST(Annotations, SpriteParsingEmptyImage) { const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png"); const auto json_1x = R"JSON({ "image": {} })JSON"; - const auto images = parseSprite(image_1x, json_1x); + const auto images = parseSprite(image_1x, json_1x).get(); EXPECT_EQ(0u, images.size()); EXPECT_EQ(1u, log.count({ @@ -251,7 +242,7 @@ TEST(Annotations, SpriteParsingSimpleWidthHeight) { const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png"); const auto json_1x = R"JSON({ "image": { "width": 32, "height": 32 } })JSON"; - const auto images = parseSprite(image_1x, json_1x); + const auto images = parseSprite(image_1x, json_1x).get(); EXPECT_EQ(1u, images.size()); } @@ -261,7 +252,7 @@ TEST(Annotations, SpriteParsingWidthTooBig) { const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png"); const auto json_1x = R"JSON({ "image": { "width": 65536, "height": 32 } })JSON"; - const auto images = parseSprite(image_1x, json_1x); + const auto images = parseSprite(image_1x, json_1x).get(); EXPECT_EQ(0u, images.size()); EXPECT_EQ(1u, log.count({ @@ -284,7 +275,7 @@ TEST(Annotations, SpriteParsingNegativeWidth) { const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png"); const auto json_1x = R"JSON({ "image": { "width": -1, "height": 32 } })JSON"; - const auto images = parseSprite(image_1x, json_1x); + const auto images = parseSprite(image_1x, json_1x).get(); EXPECT_EQ(0u, images.size()); EXPECT_EQ(1u, log.count({ @@ -307,7 +298,7 @@ TEST(Annotations, SpriteParsingNullRatio) { const auto image_1x = util::read_file("test/fixtures/annotations/emerald.png"); const auto json_1x = R"JSON({ "image": { "width": 32, "height": 32, "pixelRatio": 0 } })JSON"; - const auto images = parseSprite(image_1x, json_1x); + const auto images = parseSprite(image_1x, json_1x).get(); EXPECT_EQ(0u, images.size()); EXPECT_EQ(1u, log.count({