From ddecdd65ca1ac6a311a8118365dec0e08a567228 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Fri, 26 May 2017 10:23:12 -0700 Subject: [PATCH 1/5] [tests] Add test for calling addAnnotationImage with an existing ID --- test/api/annotations.test.cpp | 20 ++++++++++++++++-- .../annotations/readd_image/expected.png | Bin 0 -> 1031 bytes 2 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 test/fixtures/annotations/readd_image/expected.png diff --git a/test/api/annotations.test.cpp b/test/api/annotations.test.cpp index 9e85a74c5bb..4c27c871ae8 100644 --- a/test/api/annotations.test.cpp +++ b/test/api/annotations.test.cpp @@ -16,9 +16,12 @@ using namespace mbgl; namespace { +PremultipliedImage namedImage(const std::string& name) { + return decodeImage(util::read_file("test/fixtures/sprites/" + name + ".png")); +} + std::unique_ptr namedMarker(const std::string& name) { - PremultipliedImage image = decodeImage(util::read_file("test/fixtures/sprites/" + name + ".png")); - return std::make_unique(name, std::move(image), 1.0); + return std::make_unique(name, namedImage(name), 1.0); } class AnnotationTest { @@ -319,6 +322,19 @@ TEST(Annotations, SwitchStyle) { test.checkRendering("switch_style"); } +TEST(Annotations, ReaddImage) { + AnnotationTest test; + + test.map.setStyleJSON(util::read_file("test/fixtures/api/empty.json")); + test.map.addAnnotationImage(namedMarker("default_marker")); + test.map.addAnnotation(SymbolAnnotation { Point { 0, 0 }, "default_marker" }); + + test::render(test.map, test.view); + + test.map.addAnnotationImage(std::make_unique("default_marker", namedImage("flipped_marker"), 1.0)); + test.checkRendering("readd_image"); +} + TEST(Annotations, QueryRenderedFeatures) { AnnotationTest test; diff --git a/test/fixtures/annotations/readd_image/expected.png b/test/fixtures/annotations/readd_image/expected.png new file mode 100644 index 0000000000000000000000000000000000000000..3c4847f3a70408ee864435e2a4f5c72181f0c6bc GIT binary patch literal 1031 zcmeAS@N?(olHy`uVBq!ia0y~yU<5K5893O0R7}x|GzJFdbDl1aAr*7p-r3(Rk}kuZ zpg7@BR(|>Bqhdw(oE+8#Dsgx|RJ_5_v^KWu597+UK})?R2)xf>@l-Byy|jEI*AmW5 zrb#AIk(%sbFU>w%mZjW0^Tx88J-_U;&4<+Xncr(G??0OR{rH(TJBxu<4ixwxzj62O z-w#uaB;USyQxo&I?vlvn?8QrTW8)v>)NF{&eElVNqRq+ZU7rqg9ee!o=PLR5cej2% z&tsZ?mMy_oclNve`Ad2(y=4e12zvf6&7fkMg=Xx_G6wC4Yb*E5@y)l{TaY;G#71d` zoEh6x5A418Vzs+`@3Qsz_iXxPp3hNb$eEDyKsS%6&QR`t?S=RIYb*c$`tttv_WsA~ z{q1Kzi&+PhOH0cZ#B=3?R)orznk#k`TFp6dtU!3eG;=ywfA_#hD0OjA7vIb zUmhRr{{QMvX^OoQr-p_vvc7(70eLIigYn{WTY3uY0+}?e!+WWWe|Iz&L_1nt+ zSxr*QoV>hbZ=-R+h4=U4&A!I>Eb*Op=_{Mx%f$8DV|!nCFaPK3-T1`w-lebXeh(9} zUq@bE_ums79RCh!-_g Date: Thu, 18 May 2017 16:31:16 -0700 Subject: [PATCH 2/5] [core] Auto-growable SpriteAtlas using shelf-pack --- CMakeLists.txt | 1 + cmake/core.cmake | 1 + cmake/test.cmake | 1 + src/mbgl/annotation/annotation_manager.cpp | 3 +- src/mbgl/annotation/annotation_manager.hpp | 2 +- src/mbgl/map/map.cpp | 2 +- src/mbgl/sprite/sprite_atlas.cpp | 80 ++++++++++-------- src/mbgl/sprite/sprite_atlas.hpp | 21 ++--- src/mbgl/style/style.cpp | 2 +- test/fixtures/sprite_atlas/basic/expected.png | Bin 694 -> 673 bytes test/fixtures/sprite_atlas/size/expected.png | Bin 1118 -> 609 bytes .../sprite_atlas/updates_after/expected.png | Bin 135 -> 118 bytes .../sprite_atlas/updates_before/expected.png | Bin 110 -> 96 bytes test/sprite/sprite_atlas.test.cpp | 27 ++---- test/style/source.test.cpp | 2 +- test/text/quads.test.cpp | 4 +- test/tile/annotation_tile.test.cpp | 2 +- test/tile/geojson_tile.test.cpp | 2 +- test/tile/raster_tile.test.cpp | 2 +- test/tile/vector_tile.test.cpp | 2 +- 20 files changed, 75 insertions(+), 79 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index f0c7a2ac572..50e82553a4f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -56,6 +56,7 @@ mason_use(pixelmatch VERSION 0.10.0 HEADER_ONLY) mason_use(geojson VERSION 0.4.0 HEADER_ONLY) mason_use(polylabel VERSION 1.0.2 HEADER_ONLY) mason_use(wagyu VERSION 0.4.1 HEADER_ONLY) +mason_use(shelf-pack VERSION 2.0.1 HEADER_ONLY) add_definitions(-DRAPIDJSON_HAS_STDSTRING=1) diff --git a/cmake/core.cmake b/cmake/core.cmake index 531edc092da..5671ab53b5d 100644 --- a/cmake/core.cmake +++ b/cmake/core.cmake @@ -26,6 +26,7 @@ target_add_mason_package(mbgl-core PRIVATE earcut) target_add_mason_package(mbgl-core PRIVATE protozero) target_add_mason_package(mbgl-core PRIVATE polylabel) target_add_mason_package(mbgl-core PRIVATE wagyu) +target_add_mason_package(mbgl-core PRIVATE shelf-pack) mbgl_platform_core() diff --git a/cmake/test.cmake b/cmake/test.cmake index 3c63f7bcf89..8a5233f5a52 100644 --- a/cmake/test.cmake +++ b/cmake/test.cmake @@ -35,6 +35,7 @@ target_add_mason_package(mbgl-test PRIVATE pixelmatch) target_add_mason_package(mbgl-test PRIVATE boost) target_add_mason_package(mbgl-test PRIVATE geojson) target_add_mason_package(mbgl-test PRIVATE geojsonvt) +target_add_mason_package(mbgl-test PRIVATE shelf-pack) mbgl_platform_test() diff --git a/src/mbgl/annotation/annotation_manager.cpp b/src/mbgl/annotation/annotation_manager.cpp index a7f1c69f3bb..f04a0bb3dc5 100644 --- a/src/mbgl/annotation/annotation_manager.cpp +++ b/src/mbgl/annotation/annotation_manager.cpp @@ -20,8 +20,7 @@ using namespace style; const std::string AnnotationManager::SourceID = "com.mapbox.annotations"; const std::string AnnotationManager::PointLayerID = "com.mapbox.annotations.points"; -AnnotationManager::AnnotationManager(float pixelRatio) - : spriteAtlas({ 1024, 1024 }, pixelRatio){ +AnnotationManager::AnnotationManager() { // This is a special atlas, holding only images added via addIcon, so we always treat it as // loaded. spriteAtlas.markAsLoaded(); diff --git a/src/mbgl/annotation/annotation_manager.hpp b/src/mbgl/annotation/annotation_manager.hpp index 837827b75c4..4d22ac81f6c 100644 --- a/src/mbgl/annotation/annotation_manager.hpp +++ b/src/mbgl/annotation/annotation_manager.hpp @@ -26,7 +26,7 @@ class Image; class AnnotationManager : private util::noncopyable { public: - AnnotationManager(float pixelRatio); + AnnotationManager(); ~AnnotationManager(); AnnotationID addAnnotation(const Annotation&, const uint8_t maxZoom); diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp index e994428bf1d..14a2401ed54 100644 --- a/src/mbgl/map/map.cpp +++ b/src/mbgl/map/map.cpp @@ -153,7 +153,7 @@ Map::Impl::Impl(Map& map_, contextMode(contextMode_), pixelRatio(pixelRatio_), programCacheDir(std::move(programCacheDir_)), - annotationManager(std::make_unique(pixelRatio)), + annotationManager(std::make_unique()), asyncInvalidate([this] { if (mode == MapMode::Continuous) { backend.invalidate(); diff --git a/src/mbgl/sprite/sprite_atlas.cpp b/src/mbgl/sprite/sprite_atlas.cpp index 57a5488105c..d3f0072b20c 100644 --- a/src/mbgl/sprite/sprite_atlas.cpp +++ b/src/mbgl/sprite/sprite_atlas.cpp @@ -20,22 +20,25 @@ namespace mbgl { // correct behavior of GL_LINEAR texture sampling mode. static constexpr uint16_t padding = 1; -SpriteAtlasElement::SpriteAtlasElement(Rect rect, const style::Image::Impl& image) +SpriteAtlasElement::SpriteAtlasElement(const mapbox::Bin& bin, const style::Image::Impl& image) : sdf(image.sdf), pixelRatio(image.pixelRatio), textureRect( - rect.x + padding, - rect.y + padding, - rect.w - padding * 2, - rect.h - padding * 2 + bin.x + padding, + bin.y + padding, + bin.w - padding * 2, + bin.h - padding * 2 ) { } -SpriteAtlas::SpriteAtlas(Size size, float pixelRatio) - : pixelSize(std::ceil(size.width * pixelRatio), - std::ceil(size.height * pixelRatio)), - bin(pixelSize.width, pixelSize.height), - dirty(true) { +static mapbox::ShelfPack::ShelfPackOptions shelfPackOptions() { + mapbox::ShelfPack::ShelfPackOptions options; + options.autoResize = true; + return options; +} + +SpriteAtlas::SpriteAtlas() + : shelfPack(64, 64, shelfPackOptions()) { } SpriteAtlas::~SpriteAtlas() = default; @@ -64,12 +67,12 @@ void SpriteAtlas::addImage(Immutable image_) { entry.image = std::move(image_); - if (entry.iconRect) { - copy(entry, &Entry::iconRect); + if (entry.iconBin) { + copy(entry, &Entry::iconBin); } - if (entry.patternRect) { - copy(entry, &Entry::patternRect); + if (entry.patternBin) { + copy(entry, &Entry::patternBin); } } @@ -81,12 +84,12 @@ void SpriteAtlas::removeImage(const std::string& id) { Entry& entry = it->second; - if (entry.iconRect) { - bin.release(*entry.iconRect); + if (entry.iconBin) { + shelfPack.unref(*entry.iconBin); } - if (entry.patternRect) { - bin.release(*entry.patternRect); + if (entry.patternBin) { + shelfPack.unref(*entry.patternBin); } entries.erase(it); @@ -116,16 +119,14 @@ void SpriteAtlas::removeRequestor(IconRequestor& requestor) { } optional SpriteAtlas::getIcon(const std::string& id) { - return getImage(id, &Entry::iconRect); + return getImage(id, &Entry::iconBin); } optional SpriteAtlas::getPattern(const std::string& id) { - return getImage(id, &Entry::patternRect); + return getImage(id, &Entry::patternBin); } -optional SpriteAtlas::getImage(const std::string& id, - optional> Entry::*entryRect) { - +optional SpriteAtlas::getImage(const std::string& id, mapbox::Bin* Entry::*entryBin) { auto it = entries.find(id); if (it == entries.end()) { if (!entries.empty()) { @@ -136,10 +137,10 @@ optional SpriteAtlas::getImage(const std::string& id, Entry& entry = it->second; - if (entry.*entryRect) { + if (entry.*entryBin) { assert(entry.image.get()); return SpriteAtlasElement { - *(entry.*entryRect), + *(entry.*entryBin), *entry.image }; } @@ -147,44 +148,51 @@ optional SpriteAtlas::getImage(const std::string& id, const uint16_t width = entry.image->image.size.width + padding * 2; const uint16_t height = entry.image->image.size.height + padding * 2; - Rect rect = bin.allocate(width, height); - if (rect.w == 0) { + mapbox::Bin* bin = shelfPack.packOne(-1, width, height); + if (!bin) { if (debug::spriteWarnings) { Log::Warning(Event::Sprite, "sprite atlas bitmap overflow"); } return {}; } - entry.*entryRect = rect; - copy(entry, entryRect); + entry.*entryBin = bin; + copy(entry, entryBin); return SpriteAtlasElement { - rect, + *bin, *entry.image }; } Size SpriteAtlas::getPixelSize() const { - return pixelSize; + return Size { + static_cast(shelfPack.width()), + static_cast(shelfPack.height()) + }; } -void SpriteAtlas::copy(const Entry& entry, optional> Entry::*entryRect) { +void SpriteAtlas::copy(const Entry& entry, mapbox::Bin* Entry::*entryBin) { if (!image.valid()) { image = PremultipliedImage(getPixelSize()); image.fill(0); + } else if (image.size != getPixelSize()) { + PremultipliedImage newImage(getPixelSize()); + PremultipliedImage::copy(image, newImage, { 0, 0 }, { 0, 0 }, image.size); + image = std::move(newImage); } const PremultipliedImage& src = entry.image->image; - const Rect& rect = *(entry.*entryRect); + const mapbox::Bin& bin = *(entry.*entryBin); - const uint32_t x = rect.x + padding; - const uint32_t y = rect.y + padding; + const uint32_t x = bin.x + padding; + const uint32_t y = bin.y + padding; const uint32_t w = src.size.width; const uint32_t h = src.size.height; PremultipliedImage::copy(src, image, { 0, 0 }, { x, y }, { w, h }); - if (entryRect == &Entry::patternRect) { + if (entryBin == &Entry::patternBin) { // Add 1 pixel wrapped padding on each side of the image. PremultipliedImage::copy(src, image, { 0, h - 1 }, { x, y - 1 }, { w, 1 }); // T PremultipliedImage::copy(src, image, { 0, 0 }, { x, y + h }, { w, 1 }); // B diff --git a/src/mbgl/sprite/sprite_atlas.hpp b/src/mbgl/sprite/sprite_atlas.hpp index 3629ed6eb10..f5c7fd114a2 100644 --- a/src/mbgl/sprite/sprite_atlas.hpp +++ b/src/mbgl/sprite/sprite_atlas.hpp @@ -1,11 +1,13 @@ #pragma once -#include #include #include #include +#include #include +#include + #include #include #include @@ -20,7 +22,7 @@ class Context; class SpriteAtlasElement { public: - SpriteAtlasElement(Rect, const style::Image::Impl&); + SpriteAtlasElement(const mapbox::Bin&, const style::Image::Impl&); bool sdf; float pixelRatio; @@ -59,7 +61,7 @@ class IconRequestor { class SpriteAtlas : public util::noncopyable { public: - SpriteAtlas(Size, float pixelRatio); + SpriteAtlas(); ~SpriteAtlas(); void onSpriteLoaded(); @@ -106,7 +108,6 @@ class SpriteAtlas : public util::noncopyable { } private: - const Size pixelSize; bool loaded = false; struct Entry { @@ -116,20 +117,20 @@ class SpriteAtlas : public util::noncopyable { // it must have two distinct entries in the texture. The one for the icon image has // a single pixel transparent border, and the one for the pattern image has a single // pixel border wrapped from the opposite side. - optional> iconRect; - optional> patternRect; + mapbox::Bin* iconBin = nullptr; + mapbox::Bin* patternBin = nullptr; }; - optional getImage(const std::string& name, optional> Entry::*rect); - void copy(const Entry&, optional> Entry::*rect); + optional getImage(const std::string& name, mapbox::Bin* Entry::*bin); + void copy(const Entry&, mapbox::Bin* Entry::*bin); IconMap buildIconMap(); std::unordered_map entries; - BinPack bin; + mapbox::ShelfPack shelfPack; PremultipliedImage image; mbgl::optional texture; - bool dirty; + bool dirty = true; std::set requestors; IconMap icons; diff --git a/src/mbgl/style/style.cpp b/src/mbgl/style/style.cpp index 84ee841c064..1a4cf0ca106 100644 --- a/src/mbgl/style/style.cpp +++ b/src/mbgl/style/style.cpp @@ -74,7 +74,7 @@ Style::Style(Scheduler& scheduler_, FileSource& fileSource_, float pixelRatio) fileSource(fileSource_), glyphAtlas(std::make_unique(Size{ 2048, 2048 }, fileSource)), spriteLoader(std::make_unique(pixelRatio)), - spriteAtlas(std::make_unique(Size{ 1024, 1024 }, pixelRatio)), + spriteAtlas(std::make_unique()), lineAtlas(std::make_unique(Size{ 256, 512 })), light(std::make_unique()), renderLight(light->impl), diff --git a/test/fixtures/sprite_atlas/basic/expected.png b/test/fixtures/sprite_atlas/basic/expected.png index cd13d16df6ca9819e2bacdf36184a3c53e5a050f..2960891c0498c063fab809bd5f679feb5d8e7b56 100644 GIT binary patch literal 673 zcmV;S0$%-zP)&Lq-stA|{?9qDJNKz*+&N4=bU@)nb*-_fH>Eg?zyLpq_&w| z5?pRSsZXV;yK3R(%%q9C*VwClgu%e@LL+gHv8apGYa0iNyPla^JtkA-%>8k9to7PT zR;E3gp&y%D^5De>>AGK$&fCxO>&o3x>3#l2J}-YSj3cvFdTphdsoZRJ_G@NzTt?q5 z%I_T~N-as{?fjZt>whVOlQS)*@&Bq5Q_l=7Q<%N+=#{Mh+?JDo@qzK_Jfm8^*vn0w zg_wFKVDD*}B2#L%Z{)p9%q$f~zkK^8x1LmsEMj5Piz`LunhTd$dS*QV#}FQfGDMlG zxqP4Yj^!BwFpGQs-*i8mCt9&}6spyx9hlx9l}tHJQQf(7mF?z99TmL7Dh0+2rt zX8iIjx^-qsvlSY~{jdUJ>8a+02EaEOmh8U-;5>Sk0dQKf+K|*R%)u10_GxLOWEe-- zF~!3HE(2ChhBU|_2Cba4!+nDh3={w$F|k>ek2 zobEZqJA7mmS>t--Qi8GuM^lDs($XY_rVSfABRX_6xWw8Nnp)KqJQO;FbXg9r=v6s; zS5NIQj^d<8Evv!!f)+rLscx_ zE|=5!p}+dv?6_&af1khqYwz;BU)iss=K(Z2{?GgD^vWwY@?!kA-L$RteI3cV z%}QQl-XjkM7Ri)z{{&o9A0*7`UkZTH5}_)q5V@I1lvY#=@-=ZA(7-ZPmW5w$wCI z{_Os>r!Rb5Bwe=V*O|3>8ilsI*7eo#ye$tp-mUk=T;KZL>^T?8j+k2llNf$*KSo|T<^Oie4czLKC^ic2%nU(r9HWOG@@iZUlzAdrMhEE`exR@n1J8A)=(50dx)>= znU%Ba2Pp)M=$c#$6QbpS-r!opuR$$q>~ZoczIAk2rq>T6;rr*e(m0TbQURmZ_Ku?@ zxYqG&UcKBr1R&Oky^0O%J~bYmIDmLOv%gc@J)L|5g3aGR02-d2z8au{i&U9J^i==c>W&}P|yHdc)SGulL1CcKpUJI0I?8>6l@$`elq}9V)9Gi zW8@95HSPMg0r6DmGxQ6XmiPpObAKQJ6Q14mAvA6Pu15P-7|CoU-vFRD0j@&a?<%qo z7=-LA5M|_ALs953q@{q`#@HK2harg9+RuBrH2z1p)=)HB3R(^g{c}Eakt z!T9z@v`2`e#DS0RLza|XysPm-@!~wClOJ>ijOCkN#QhKuyt~4qX@!DPmmF7DliZu7 z)+LQ59a*Nk1VY>dIbD|qxlGCSG;iKKbE&As-26M6<;w52%2mCx-%!18=Ff9_|H|gf zG*+5YulHcd@3wb(&^OHgWUNskEch3 zZ?`Oey>jNRZxe2oExDJmZeK~(#oxyD%zGat+J0l>t4-=+~9Rxdly)ajWY2nk>iTvSytl4-Qrv?4T^{%KQ-itQmMmNxk$mbt z_m30bPU_ajtlRT9=kO8rsiNW$clJ!Y8MHMaYSCSOlldeD z6s5lB>b#aC+{!oY*|q*8AN|X&0;v;r^CitD3 z$424OUa0ReU_W}Noqul#=j!(x!2r>mdKI;Vst05cRJ Aq5uE@ literal 135 zcmeAS@N?(olHy`uVBq!ia0vp^3LwnE1|*BCs=fdz#^NA%Cx&(BWL^R}j-D=#ArYK! zuWaN6s$y{{_&--P+(Bse?|+Rkjz^w9xw~ImzdU8>?fAQoLe4X?a0nYgr+Ar*7p9%SSN@(wNd?w`wN fz5vKX1`XxR4h{^A{hv+-f`mO?{an^LB{Ts5qcarg literal 110 zcmeAS@N?(olHy`uVBq!ia0vp^3LwnE1|*BCs=fdz#^NA%Cx&(BWL^R}8lEnWArYK! zFEj!<42Ku|43>Y=8jyIPfsvVwN5UWh!q~&lBcQ;*cus5UagZ`kS3j3^P6impl); } - EXPECT_EQ(63u, atlas.getPixelSize().width); - EXPECT_EQ(112u, atlas.getPixelSize().height); - auto metro = *atlas.getIcon("metro"); EXPECT_EQ(1, metro.tl()[0]); EXPECT_EQ(1, metro.tl()[1]); @@ -38,8 +35,7 @@ TEST(SpriteAtlas, Basic) { EXPECT_EQ(18, metro.displaySize()[1]); EXPECT_EQ(1.0f, metro.pixelRatio); - EXPECT_EQ(63u, atlas.getAtlasImage().size.width); - EXPECT_EQ(112u, atlas.getAtlasImage().size.height); + EXPECT_EQ(atlas.getPixelSize(), atlas.getAtlasImage().size); auto missing = atlas.getIcon("doesnotexist"); EXPECT_FALSE(missing); @@ -62,7 +58,7 @@ TEST(SpriteAtlas, Basic) { } TEST(SpriteAtlas, Size) { - SpriteAtlas atlas({ 63, 112 }, 1.4); + SpriteAtlas atlas; auto images = parseSprite(util::read_file("test/fixtures/annotations/emerald.png"), util::read_file("test/fixtures/annotations/emerald.json")); @@ -70,9 +66,6 @@ TEST(SpriteAtlas, Size) { atlas.addImage(image->impl); } - EXPECT_EQ(89u, atlas.getPixelSize().width); - EXPECT_EQ(157u, atlas.getPixelSize().height); - auto metro = *atlas.getIcon("metro"); EXPECT_EQ(1, metro.tl()[0]); EXPECT_EQ(1, metro.tl()[1]); @@ -86,10 +79,7 @@ TEST(SpriteAtlas, Size) { } TEST(SpriteAtlas, Updates) { - SpriteAtlas atlas({ 32, 32 }, 1); - - EXPECT_EQ(32u, atlas.getPixelSize().width); - EXPECT_EQ(32u, atlas.getPixelSize().height); + SpriteAtlas atlas; atlas.addImage(makeMutable("one", PremultipliedImage({ 16, 12 }), 1)); auto one = *atlas.getIcon("one"); @@ -101,10 +91,6 @@ TEST(SpriteAtlas, Updates) { EXPECT_EQ(12, one.displaySize()[1]); EXPECT_EQ(1.0f, one.pixelRatio); - // Now the image was created lazily. - EXPECT_EQ(32u, atlas.getAtlasImage().size.width); - EXPECT_EQ(32u, atlas.getAtlasImage().size.height); - test::checkImage("test/fixtures/sprite_atlas/updates_before", atlas.getAtlasImage()); // Update image @@ -119,7 +105,7 @@ TEST(SpriteAtlas, Updates) { TEST(SpriteAtlas, AddRemove) { FixtureLog log; - SpriteAtlas atlas({ 32, 32 }, 1); + SpriteAtlas atlas; atlas.addImage(makeMutable("one", PremultipliedImage({ 16, 16 }), 2)); atlas.addImage(makeMutable("two", PremultipliedImage({ 16, 16 }), 2)); @@ -148,8 +134,7 @@ TEST(SpriteAtlas, AddRemove) { TEST(SpriteAtlas, RemoveReleasesBinPackRect) { FixtureLog log; - - SpriteAtlas atlas({ 36, 36 }, 1); + SpriteAtlas atlas; atlas.addImage(makeMutable("big", PremultipliedImage({ 32, 32 }), 1)); EXPECT_TRUE(atlas.getIcon("big")); diff --git a/test/style/source.test.cpp b/test/style/source.test.cpp index 0473286c8b6..880bcd986c3 100644 --- a/test/style/source.test.cpp +++ b/test/style/source.test.cpp @@ -43,7 +43,7 @@ class SourceTest { Transform transform; TransformState transformState; ThreadPool threadPool { 1 }; - AnnotationManager annotationManager { 1.0 }; + AnnotationManager annotationManager; style::Style style { threadPool, fileSource, 1.0 }; TileParameters tileParameters { diff --git a/test/text/quads.test.cpp b/test/text/quads.test.cpp index 4c40b41a971..0a1dbbd1c95 100644 --- a/test/text/quads.test.cpp +++ b/test/text/quads.test.cpp @@ -13,7 +13,7 @@ TEST(getIconQuads, normal) { SymbolLayoutProperties::Evaluated layout; Anchor anchor(2.0, 3.0, 0.0, 0.5f, 0); SpriteAtlasElement image = { - Rect( 0, 0, 15, 11 ), + mapbox::Bin(-1, 15, 11, 0, 0), style::Image::Impl("test", PremultipliedImage({1,1}), 1.0) }; @@ -43,7 +43,7 @@ TEST(getIconQuads, normal) { TEST(getIconQuads, style) { Anchor anchor(0.0, 0.0, 0.0, 0.5f, 0); SpriteAtlasElement image = { - Rect( 0, 0, 20, 20 ), + mapbox::Bin(-1, 20, 20, 0, 0), style::Image::Impl("test", PremultipliedImage({1,1}), 1.0) }; diff --git a/test/tile/annotation_tile.test.cpp b/test/tile/annotation_tile.test.cpp index 607a8cca175..05ce1097668 100644 --- a/test/tile/annotation_tile.test.cpp +++ b/test/tile/annotation_tile.test.cpp @@ -23,7 +23,7 @@ class AnnotationTileTest { TransformState transformState; util::RunLoop loop; ThreadPool threadPool { 1 }; - AnnotationManager annotationManager { 1.0 }; + AnnotationManager annotationManager; style::Style style { threadPool, fileSource, 1.0 }; TileParameters tileParameters { diff --git a/test/tile/geojson_tile.test.cpp b/test/tile/geojson_tile.test.cpp index 8669c02dfd9..dad4aef2ee4 100644 --- a/test/tile/geojson_tile.test.cpp +++ b/test/tile/geojson_tile.test.cpp @@ -23,7 +23,7 @@ class GeoJSONTileTest { TransformState transformState; util::RunLoop loop; ThreadPool threadPool { 1 }; - AnnotationManager annotationManager { 1.0 }; + AnnotationManager annotationManager; style::Style style { threadPool, fileSource, 1.0 }; Tileset tileset { { "https://example.com" }, { 0, 22 }, "none" }; diff --git a/test/tile/raster_tile.test.cpp b/test/tile/raster_tile.test.cpp index e363b736099..ee6e31d845e 100644 --- a/test/tile/raster_tile.test.cpp +++ b/test/tile/raster_tile.test.cpp @@ -19,7 +19,7 @@ class RasterTileTest { TransformState transformState; util::RunLoop loop; ThreadPool threadPool { 1 }; - AnnotationManager annotationManager { 1.0 }; + AnnotationManager annotationManager; style::Style style { threadPool, fileSource, 1.0 }; Tileset tileset { { "https://example.com" }, { 0, 22 }, "none" }; diff --git a/test/tile/vector_tile.test.cpp b/test/tile/vector_tile.test.cpp index 03896199f40..205d001f72c 100644 --- a/test/tile/vector_tile.test.cpp +++ b/test/tile/vector_tile.test.cpp @@ -25,7 +25,7 @@ class VectorTileTest { TransformState transformState; util::RunLoop loop; ThreadPool threadPool { 1 }; - AnnotationManager annotationManager { 1.0 }; + AnnotationManager annotationManager; style::Style style { threadPool, fileSource, 1.0 }; Tileset tileset { { "https://example.com" }, { 0, 22 }, "none" }; From bef533cc9f803c8854824cbeb26fd4c2679c9464 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 22 May 2017 13:33:18 -0700 Subject: [PATCH 3/5] [test] Lower memory ceiling We gained a lot of overhead by reducing the initial SpriteAtlas size. --- test/util/memory.test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/util/memory.test.cpp b/test/util/memory.test.cpp index f824ba30ca1..97eabf9cb32 100644 --- a/test/util/memory.test.cpp +++ b/test/util/memory.test.cpp @@ -159,7 +159,7 @@ TEST(Memory, Footprint) { RecordProperty("vectorFootprint", vectorFootprint); RecordProperty("rasterFootprint", rasterFootprint); - ASSERT_LT(vectorFootprint, 65.2 * 1024 * 1024) << "\ + ASSERT_LT(vectorFootprint, 40 * 1024 * 1024) << "\ mbgl::Map footprint over 65.2MB for vector styles."; ASSERT_LT(rasterFootprint, 25 * 1024 * 1024) << "\ From e04c90b98765ed32047962fe56cf5b8f928358be Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 22 May 2017 13:28:09 -0700 Subject: [PATCH 4/5] [core] Don't use a separate SpriteAtlas for annotation images Instead, just add them to the Style as needed. Includes changes from #8905 and takes care to avoid regressing #3817. --- cmake/core-files.cmake | 2 - cmake/test-files.cmake | 1 - include/mbgl/style/image.hpp | 1 + src/mbgl/annotation/annotation_manager.cpp | 47 ++++++++++++------- src/mbgl/annotation/annotation_manager.hpp | 10 ++-- src/mbgl/annotation/annotation_tile.cpp | 2 +- src/mbgl/map/map.cpp | 8 ++-- src/mbgl/renderer/buckets/symbol_bucket.hpp | 2 - src/mbgl/renderer/painter.cpp | 3 +- src/mbgl/renderer/painter.hpp | 3 +- src/mbgl/renderer/painters/painter_symbol.cpp | 5 +- src/mbgl/sprite/sprite_atlas.cpp | 18 +++++-- src/mbgl/sprite/sprite_atlas.hpp | 8 +--- src/mbgl/sprite/sprite_image_collection.cpp | 40 ---------------- src/mbgl/sprite/sprite_image_collection.hpp | 24 ---------- src/mbgl/style/image.cpp | 2 + src/mbgl/style/style.cpp | 28 ++++++----- src/mbgl/style/style.hpp | 2 +- src/mbgl/tile/geometry_tile.cpp | 7 +-- test/sprite/sprite_atlas.test.cpp | 40 ++++++++++++++++ test/sprite/sprite_image_collection.test.cpp | 45 ------------------ 21 files changed, 121 insertions(+), 177 deletions(-) delete mode 100644 src/mbgl/sprite/sprite_image_collection.cpp delete mode 100644 src/mbgl/sprite/sprite_image_collection.hpp delete mode 100644 test/sprite/sprite_image_collection.test.cpp diff --git a/cmake/core-files.cmake b/cmake/core-files.cmake index dfe11b82b84..e56f2fdc37d 100644 --- a/cmake/core-files.cmake +++ b/cmake/core-files.cmake @@ -288,8 +288,6 @@ set(MBGL_CORE_FILES # sprite src/mbgl/sprite/sprite_atlas.cpp src/mbgl/sprite/sprite_atlas.hpp - src/mbgl/sprite/sprite_image_collection.cpp - src/mbgl/sprite/sprite_image_collection.hpp src/mbgl/sprite/sprite_loader.cpp src/mbgl/sprite/sprite_loader.hpp src/mbgl/sprite/sprite_loader_observer.hpp diff --git a/cmake/test-files.cmake b/cmake/test-files.cmake index 83b935d63ff..c0673bedefd 100644 --- a/cmake/test-files.cmake +++ b/cmake/test-files.cmake @@ -46,7 +46,6 @@ set(MBGL_TEST_FILES # sprite test/sprite/sprite_atlas.test.cpp - test/sprite/sprite_image_collection.test.cpp test/sprite/sprite_loader.test.cpp test/sprite/sprite_parser.test.cpp diff --git a/include/mbgl/style/image.hpp b/include/mbgl/style/image.hpp index 53cab23c2f8..55760ca5034 100644 --- a/include/mbgl/style/image.hpp +++ b/include/mbgl/style/image.hpp @@ -11,6 +11,7 @@ namespace style { class Image { public: Image(std::string id, PremultipliedImage&&, float pixelRatio, bool sdf = false); + Image(const Image&); std::string getID() const; diff --git a/src/mbgl/annotation/annotation_manager.cpp b/src/mbgl/annotation/annotation_manager.cpp index f04a0bb3dc5..9ea47acadb4 100644 --- a/src/mbgl/annotation/annotation_manager.cpp +++ b/src/mbgl/annotation/annotation_manager.cpp @@ -4,9 +4,7 @@ #include #include #include -#include #include -#include #include #include #include @@ -20,12 +18,7 @@ using namespace style; const std::string AnnotationManager::SourceID = "com.mapbox.annotations"; const std::string AnnotationManager::PointLayerID = "com.mapbox.annotations.points"; -AnnotationManager::AnnotationManager() { - // This is a special atlas, holding only images added via addIcon, so we always treat it as - // loaded. - spriteAtlas.markAsLoaded(); -} - +AnnotationManager::AnnotationManager() = default; AnnotationManager::~AnnotationManager() = default; AnnotationID AnnotationManager::addAnnotation(const Annotation& annotation, const uint8_t maxZoom) { @@ -155,7 +148,7 @@ void AnnotationManager::updateStyle(Style& style) { std::unique_ptr layer = std::make_unique(PointLayerID, SourceID); layer->setSourceLayer(PointLayerID); - layer->setIconImage({"{sprite}"}); + layer->setIconImage({SourceID + ".{sprite}"}); layer->setIconAllowOverlap(true); layer->setIconIgnorePlacement(true); @@ -166,13 +159,30 @@ void AnnotationManager::updateStyle(Style& style) { shape.second->updateStyle(style); } + for (const auto& image : images) { + // Call addImage even for images we may have previously added, because we must support + // addAnnotationImage being used to update an existing image. Creating a new image is + // relatively cheap, as it copies only the Immutable reference. (We can't keep track + // of which images need to be added because we don't know if the style is the same + // instance as in the last updateStyle call. If it's a new style, we need to add all + // images.) + style.addImage(std::make_unique(image.second)); + } + for (const auto& layer : obsoleteShapeAnnotationLayers) { if (style.getLayer(layer)) { style.removeLayer(layer); } } + for (const auto& image : obsoleteImages) { + if (style.getImage(image)) { + style.removeImage(image); + } + } + obsoleteShapeAnnotationLayers.clear(); + obsoleteImages.clear(); } void AnnotationManager::updateData() { @@ -191,20 +201,23 @@ void AnnotationManager::removeTile(AnnotationTile& tile) { } void AnnotationManager::addImage(std::unique_ptr image) { - addSpriteImage(spriteImages, std::move(image), [&](style::Image& added) { - spriteAtlas.addImage(added.impl); - }); + // To ensure that annotation images do not collide with images from the style, + // create a new image with the input ID prefixed by "com.mapbox.annotations". + std::string id = SourceID + "." + image->getID(); + images.erase(id); + images.emplace(id, + style::Image(id, image->getImage().clone(), image->getPixelRatio(), image->isSdf())); + obsoleteImages.erase(id); } void AnnotationManager::removeImage(const std::string& id) { - removeSpriteImage(spriteImages, id, [&] () { - spriteAtlas.removeImage(id); - }); + images.erase(id); + obsoleteImages.insert(id); } double AnnotationManager::getTopOffsetPixelsForImage(const std::string& id) { - const style::Image::Impl* impl = spriteAtlas.getImage(id); - return impl ? -(impl->image.size.height / impl->pixelRatio) / 2 : 0; + auto it = images.find(id); + return it == images.end() ? -(it->second.getImage().size.height / it->second.getPixelRatio()) / 2 : 0; } } // namespace mbgl diff --git a/src/mbgl/annotation/annotation_manager.hpp b/src/mbgl/annotation/annotation_manager.hpp index 4d22ac81f6c..36f18e1aaea 100644 --- a/src/mbgl/annotation/annotation_manager.hpp +++ b/src/mbgl/annotation/annotation_manager.hpp @@ -2,7 +2,7 @@ #include #include -#include +#include #include #include @@ -21,7 +21,6 @@ class ShapeAnnotationImpl; namespace style { class Style; -class Image; } // namespace style class AnnotationManager : private util::noncopyable { @@ -36,7 +35,6 @@ class AnnotationManager : private util::noncopyable { void addImage(std::unique_ptr); void removeImage(const std::string&); double getTopOffsetPixelsForImage(const std::string&); - SpriteAtlas& getSpriteAtlas() { return spriteAtlas; } void updateStyle(style::Style&); void updateData(); @@ -67,16 +65,16 @@ class AnnotationManager : private util::noncopyable { // using SymbolAnnotationMap = std::map>; using ShapeAnnotationMap = std::map>; + using ImageMap = std::unordered_map; SymbolAnnotationTree symbolTree; SymbolAnnotationMap symbolAnnotations; ShapeAnnotationMap shapeAnnotations; + ImageMap images; std::unordered_set obsoleteShapeAnnotationLayers; + std::unordered_set obsoleteImages; std::unordered_set tiles; - std::unordered_map> spriteImages; - SpriteAtlas spriteAtlas; - friend class AnnotationTile; }; diff --git a/src/mbgl/annotation/annotation_tile.cpp b/src/mbgl/annotation/annotation_tile.cpp index 12536814140..1b34026f741 100644 --- a/src/mbgl/annotation/annotation_tile.cpp +++ b/src/mbgl/annotation/annotation_tile.cpp @@ -13,7 +13,7 @@ AnnotationTile::AnnotationTile(const OverscaledTileID& overscaledTileID, const TileParameters& parameters) : GeometryTile(overscaledTileID, AnnotationManager::SourceID, parameters, *parameters.style.glyphAtlas, - parameters.annotationManager.spriteAtlas), + *parameters.style.spriteAtlas), annotationManager(parameters.annotationManager) { annotationManager.addTile(*this); } diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp index 14a2401ed54..a24e798fb9f 100644 --- a/src/mbgl/map/map.cpp +++ b/src/mbgl/map/map.cpp @@ -278,8 +278,7 @@ void Map::Impl::render(View& view) { painter->render(*style, frameData, - view, - annotationManager->getSpriteAtlas()); + view); painter->cleanup(); @@ -312,8 +311,7 @@ void Map::Impl::render(View& view) { painter->render(*style, frameData, - view, - annotationManager->getSpriteAtlas()); + view); auto request = std::move(stillImageRequest); request->callback(nullptr); @@ -790,10 +788,12 @@ LatLng Map::latLngForPixel(const ScreenCoordinate& pixel) const { void Map::addAnnotationImage(std::unique_ptr image) { impl->annotationManager->addImage(std::move(image)); + impl->onUpdate(Update::AnnotationStyle); } void Map::removeAnnotationImage(const std::string& id) { impl->annotationManager->removeImage(id); + impl->onUpdate(Update::AnnotationStyle); } double Map::getTopOffsetPixelsForAnnotationImage(const std::string& id) { diff --git a/src/mbgl/renderer/buckets/symbol_bucket.hpp b/src/mbgl/renderer/buckets/symbol_bucket.hpp index f30c9cf10ec..652f2ea8e37 100644 --- a/src/mbgl/renderer/buckets/symbol_bucket.hpp +++ b/src/mbgl/renderer/buckets/symbol_bucket.hpp @@ -70,8 +70,6 @@ class SymbolBucket : public Bucket { optional> vertexBuffer; optional> indexBuffer; } collisionBox; - - SpriteAtlas* spriteAtlas = nullptr; }; } // namespace mbgl diff --git a/src/mbgl/renderer/painter.cpp b/src/mbgl/renderer/painter.cpp index 7649f86ca6f..3832403cce8 100644 --- a/src/mbgl/renderer/painter.cpp +++ b/src/mbgl/renderer/painter.cpp @@ -125,7 +125,7 @@ void Painter::cleanup() { context.performCleanup(); } -void Painter::render(const Style& style, const FrameData& frame_, View& view, SpriteAtlas& annotationSpriteAtlas) { +void Painter::render(const Style& style, const FrameData& frame_, View& view) { frame = frame_; if (frame.contextMode == GLContextMode::Shared) { context.setDirtyState(); @@ -176,7 +176,6 @@ void Painter::render(const Style& style, const FrameData& frame_, View& view, Sp lineAtlas->upload(context, 0); glyphAtlas->upload(context, 0); frameHistory.upload(context, 0); - annotationSpriteAtlas.upload(context, 0); for (const auto& item : order) { for (const auto& tileRef : item.tiles) { diff --git a/src/mbgl/renderer/painter.hpp b/src/mbgl/renderer/painter.hpp index 47b469d9713..e2777134d04 100644 --- a/src/mbgl/renderer/painter.hpp +++ b/src/mbgl/renderer/painter.hpp @@ -79,8 +79,7 @@ class Painter : private util::noncopyable { void render(const style::Style&, const FrameData&, - View&, - SpriteAtlas& annotationSpriteAtlas); + View&); void cleanup(); diff --git a/src/mbgl/renderer/painters/painter_symbol.cpp b/src/mbgl/renderer/painters/painter_symbol.cpp index a89e7db28d6..563489987c2 100644 --- a/src/mbgl/renderer/painters/painter_symbol.cpp +++ b/src/mbgl/renderer/painters/painter_symbol.cpp @@ -67,12 +67,11 @@ void Painter::renderSymbol(PaintParameters& parameters, auto values = layer.iconPropertyValues(layout); auto paintPropertyValues = layer.iconPaintProperties(); - SpriteAtlas& atlas = *bucket.spriteAtlas; const bool iconScaled = layout.get().constantOr(1.0) != 1.0 || bucket.iconsNeedLinear; const bool iconTransformed = values.rotationAlignment == AlignmentType::Map || state.getPitch() != 0; - atlas.bind(bucket.sdfIcons || state.isChanging() || iconScaled || iconTransformed, context, 0); + spriteAtlas->bind(bucket.sdfIcons || state.isChanging() || iconScaled || iconTransformed, context, 0); - const Size texsize = atlas.getPixelSize(); + const Size texsize = spriteAtlas->getPixelSize(); if (bucket.sdfIcons) { if (values.hasHalo) { diff --git a/src/mbgl/sprite/sprite_atlas.cpp b/src/mbgl/sprite/sprite_atlas.cpp index d3f0072b20c..97bf9edcf90 100644 --- a/src/mbgl/sprite/sprite_atlas.cpp +++ b/src/mbgl/sprite/sprite_atlas.cpp @@ -44,7 +44,7 @@ SpriteAtlas::SpriteAtlas() SpriteAtlas::~SpriteAtlas() = default; void SpriteAtlas::onSpriteLoaded() { - markAsLoaded(); + loaded = true; for (auto requestor : requestors) { requestor->onIconsAvailable(buildIconMap()); } @@ -106,8 +106,20 @@ const style::Image::Impl* SpriteAtlas::getImage(const std::string& id) const { return nullptr; } -void SpriteAtlas::getIcons(IconRequestor& requestor) { - if (isLoaded()) { +void SpriteAtlas::getIcons(IconRequestor& requestor, IconDependencies dependencies) { + // If the sprite has been loaded, or if all the icon dependencies are already present + // (i.e. if they've been addeded via runtime styling), then notify the requestor immediately. + // Otherwise, delay notification until the sprite is loaded. At that point, if any of the + // dependencies are still unavailable, we'll just assume they are permanently missing. + bool hasAllDependencies = true; + if (!isLoaded()) { + for (const auto& dependency : dependencies) { + if (entries.find(dependency) == entries.end()) { + hasAllDependencies = false; + } + } + } + if (isLoaded() || hasAllDependencies) { requestor.onIconsAvailable(buildIconMap()); } else { requestors.insert(&requestor); diff --git a/src/mbgl/sprite/sprite_atlas.hpp b/src/mbgl/sprite/sprite_atlas.hpp index f5c7fd114a2..807c871731c 100644 --- a/src/mbgl/sprite/sprite_atlas.hpp +++ b/src/mbgl/sprite/sprite_atlas.hpp @@ -66,10 +66,6 @@ class SpriteAtlas : public util::noncopyable { void onSpriteLoaded(); - void markAsLoaded() { - loaded = true; - } - bool isLoaded() const { return loaded; } @@ -80,8 +76,8 @@ class SpriteAtlas : public util::noncopyable { void addImage(Immutable); void removeImage(const std::string&); - void getIcons(IconRequestor& requestor); - void removeRequestor(IconRequestor& requestor); + void getIcons(IconRequestor&, IconDependencies); + void removeRequestor(IconRequestor&); // Ensure that the atlas contains the named image suitable for rendering as an icon, and // return its metrics. The image will be padded on each side with a one pixel wide transparent diff --git a/src/mbgl/sprite/sprite_image_collection.cpp b/src/mbgl/sprite/sprite_image_collection.cpp deleted file mode 100644 index ae00a6b146c..00000000000 --- a/src/mbgl/sprite/sprite_image_collection.cpp +++ /dev/null @@ -1,40 +0,0 @@ -#include -#include - -namespace mbgl { - -void addSpriteImage(Images& images, - std::unique_ptr image_, - std::function onAdded) { - std::string id = image_->getID(); - auto it = images.find(id); - if (it == images.end()) { - // Add new - it = images.emplace(id, std::move(image_)).first; - onAdded(*it->second.get()); - return; - } - - std::unique_ptr& image = it->second; - - // There is already a sprite with that name in our store. - if (image->getImage().size != image_->getImage().size) { - Log::Warning(Event::Sprite, "Can't change sprite dimensions for '%s'", id.c_str()); - } - - // Update existing - image = std::move(image_); - onAdded(*it->second.get()); -} - -void removeSpriteImage(Images& images, - const std::string& id, - std::function onRemoved) { - if (images.erase(id) > 0) { - onRemoved(); - } -} - - - -} // namespace mbgl diff --git a/src/mbgl/sprite/sprite_image_collection.hpp b/src/mbgl/sprite/sprite_image_collection.hpp deleted file mode 100644 index 44c7bcd4119..00000000000 --- a/src/mbgl/sprite/sprite_image_collection.hpp +++ /dev/null @@ -1,24 +0,0 @@ -#pragma once - -#include - -#include -#include -#include -#include - -namespace mbgl { - -using Images = std::unordered_map>; - -void addSpriteImage(Images&, - std::unique_ptr, - std::function onAdded = [] (style::Image&){}); - -void removeSpriteImage(Images&, - const std::string&, - std::function onRemoved = [] (){}); - - - -} // namespace mbgl diff --git a/src/mbgl/style/image.cpp b/src/mbgl/style/image.cpp index 0cce32ab986..6039c0458c6 100644 --- a/src/mbgl/style/image.cpp +++ b/src/mbgl/style/image.cpp @@ -16,6 +16,8 @@ std::string Image::getID() const { return impl->id; } +Image::Image(const Image&) = default; + const PremultipliedImage& Image::getImage() const { return impl->image; } diff --git a/src/mbgl/style/style.cpp b/src/mbgl/style/style.cpp index 1a4cf0ca106..9445772a66e 100644 --- a/src/mbgl/style/style.cpp +++ b/src/mbgl/style/style.cpp @@ -16,7 +16,6 @@ #include #include #include -#include #include #include #include @@ -36,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -506,22 +506,24 @@ bool Style::isLoaded() const { } void Style::addImage(std::unique_ptr image) { - addSpriteImage(spriteImages, std::move(image), [&](style::Image& added) { - spriteAtlas->addImage(added.impl); - observer->onUpdate(Update::Repaint); - }); + std::string id = image->getID(); + auto it = images.find(id); + if (it != images.end() && it->second->getImage().size != image->getImage().size) { + Log::Warning(Event::Sprite, "Can't change sprite dimensions for '%s'", id.c_str()); + return; + } + spriteAtlas->addImage(image->impl); + images[id] = std::move(image); } void Style::removeImage(const std::string& id) { - removeSpriteImage(spriteImages, id, [&] () { - spriteAtlas->removeImage(id); - observer->onUpdate(Update::Repaint); - }); + images.erase(id); + spriteAtlas->removeImage(id); } const style::Image* Style::getImage(const std::string& id) const { - auto it = spriteImages.find(id); - return it == spriteImages.end() ? nullptr : it->second.get(); + auto it = images.find(id); + return it == images.end() ? nullptr : it->second.get(); } RenderData Style::getRenderData(MapDebugOptions debugOptions, float angle) const { @@ -735,8 +737,8 @@ void Style::onTileError(RenderSource& source, const OverscaledTileID& tileID, st observer->onResourceError(error); } -void Style::onSpriteLoaded(std::vector>&& images) { - for (auto& image : images) { +void Style::onSpriteLoaded(std::vector>&& images_) { + for (auto& image : images_) { addImage(std::move(image)); } spriteAtlas->onSpriteLoaded(); diff --git a/src/mbgl/style/style.hpp b/src/mbgl/style/style.hpp index bc1d52eed84..5d3c2899fd8 100644 --- a/src/mbgl/style/style.hpp +++ b/src/mbgl/style/style.hpp @@ -125,6 +125,7 @@ class Style : public GlyphAtlasObserver, RenderSource* getRenderSource(const std::string& id) const; private: + std::unordered_map> images; std::vector> sources; std::vector> layers; TransitionOptions transitionOptions; @@ -150,7 +151,6 @@ class Style : public GlyphAtlasObserver, void onGlyphsError(const FontStack&, const GlyphRange&, std::exception_ptr) override; // SpriteLoaderObserver implementation. - std::unordered_map> spriteImages; void onSpriteLoaded(std::vector>&&) override; void onSpriteError(std::exception_ptr) override; diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index e3c4e7bf3b4..d25d93d6b86 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -139,9 +139,6 @@ void GeometryTile::onPlacement(PlacementResult result) { pending = false; } symbolBuckets = std::move(result.symbolBuckets); - for (auto& entry : symbolBuckets) { - dynamic_cast(entry.second.get())->spriteAtlas = &spriteAtlas; - } collisionTile = std::move(result.collisionTile); observer->onTileChanged(*this); } @@ -165,8 +162,8 @@ void GeometryTile::onIconsAvailable(IconMap icons) { worker.invoke(&GeometryTileWorker::onIconsAvailable, std::move(icons)); } -void GeometryTile::getIcons(IconDependencies) { - spriteAtlas.getIcons(*this); +void GeometryTile::getIcons(IconDependencies iconDependencies) { + spriteAtlas.getIcons(*this, std::move(iconDependencies)); } Bucket* GeometryTile::getBucket(const Layer::Impl& layer) const { diff --git a/test/sprite/sprite_atlas.test.cpp b/test/sprite/sprite_atlas.test.cpp index 7a638a9ec5b..4291fe99022 100644 --- a/test/sprite/sprite_atlas.test.cpp +++ b/test/sprite/sprite_atlas.test.cpp @@ -145,3 +145,43 @@ TEST(SpriteAtlas, RemoveReleasesBinPackRect) { EXPECT_TRUE(atlas.getIcon("big")); EXPECT_TRUE(log.empty()); } + +class StubIconRequestor : public IconRequestor { +public: + void onIconsAvailable(IconMap icons) final { + if (iconsAvailable) iconsAvailable(icons); + } + + std::function iconsAvailable; +}; + +TEST(SpriteAtlas, NotifiesRequestorWhenSpriteIsLoaded) { + SpriteAtlas atlas; + StubIconRequestor requestor; + bool notified = false; + + requestor.iconsAvailable = [&] (IconMap) { + notified = true; + }; + + atlas.getIcons(requestor, {"one"}); + ASSERT_FALSE(notified); + + atlas.onSpriteLoaded(); + ASSERT_TRUE(notified); +} + +TEST(SpriteAtlas, NotifiesRequestorImmediatelyIfDependenciesAreSatisfied) { + SpriteAtlas atlas; + StubIconRequestor requestor; + bool notified = false; + + requestor.iconsAvailable = [&] (IconMap) { + notified = true; + }; + + atlas.addImage(makeMutable("one", PremultipliedImage({ 16, 16 }), 2)); + atlas.getIcons(requestor, {"one"}); + + ASSERT_TRUE(notified); +} diff --git a/test/sprite/sprite_image_collection.test.cpp b/test/sprite/sprite_image_collection.test.cpp deleted file mode 100644 index 788d7a59d53..00000000000 --- a/test/sprite/sprite_image_collection.test.cpp +++ /dev/null @@ -1,45 +0,0 @@ -#include -#include - -#include -#include -#include - -#include - -using namespace mbgl; - -TEST(SpriteImageCollection, OtherPixelRatio) { - FixtureLog log; - Images images; - - // Adding mismatched sprite image - addSpriteImage(images, std::make_unique("one", PremultipliedImage({ 8, 8 }), 2)); -} - -TEST(SpriteImageCollection, Replace) { - FixtureLog log; - Images images; - - addSpriteImage(images, std::make_unique("sprite", PremultipliedImage({ 16, 16 }), 2)); - auto image = images.find("sprite")->second.get(); - addSpriteImage(images, std::make_unique("sprite", PremultipliedImage({ 16, 16 }), 2)); - EXPECT_NE(image, images.find("sprite")->second.get()); -} - -TEST(SpriteImageCollection, ReplaceWithDifferentDimensions) { - FixtureLog log; - Images images; - - addSpriteImage(images, std::make_unique("sprite", PremultipliedImage({ 16, 16 }), 2)); - addSpriteImage(images, std::make_unique("sprite", PremultipliedImage({ 18, 18 }), 2)); - - EXPECT_EQ(1u, log.count({ - EventSeverity::Warning, - Event::Sprite, - int64_t(-1), - "Can't change sprite dimensions for 'sprite'", - })); -} - - From ac2f1f390414dca1e57424d463243f35957e20f8 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Fri, 26 May 2017 10:39:47 -0700 Subject: [PATCH 5/5] [core] Don't need unique_ptr for AnnotationManager --- src/mbgl/map/map.cpp | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp index a24e798fb9f..75447609271 100644 --- a/src/mbgl/map/map.cpp +++ b/src/mbgl/map/map.cpp @@ -90,7 +90,7 @@ class Map::Impl : public style::Observer { Update updateFlags = Update::Nothing; - std::unique_ptr annotationManager; + AnnotationManager annotationManager; std::unique_ptr painter; std::unique_ptr