From 2921127bdbe6cb7583c097675fe67416c8dff38e Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Thu, 16 Jun 2016 17:31:52 -0700 Subject: [PATCH] [core] Avoid unnecessary work when a symbol annotation is updated In particular, if only the geometry changes, don't cascade and recalculate the style. --- include/mbgl/map/update.hpp | 3 +- src/mbgl/annotation/annotation_manager.cpp | 56 ++++++++++++++++-- src/mbgl/annotation/annotation_manager.hpp | 13 +++- src/mbgl/map/map.cpp | 15 +++-- src/mbgl/tile/geojson_tile.cpp | 4 +- src/mbgl/tile/vector_tile.cpp | 4 +- test/api/annotations.cpp | 20 +++---- .../annotations/update_icon/expected.png | Bin 2949 -> 2545 bytes .../annotations/update_point/expected.png | Bin 2504 -> 2554 bytes 9 files changed, 87 insertions(+), 28 deletions(-) diff --git a/include/mbgl/map/update.hpp b/include/mbgl/map/update.hpp index 3915545bb0c..1c1270ac700 100644 --- a/include/mbgl/map/update.hpp +++ b/include/mbgl/map/update.hpp @@ -13,7 +13,8 @@ enum class Update : uint8_t { RecalculateStyle = 1 << 3, RenderStill = 1 << 4, Repaint = 1 << 5, - Annotations = 1 << 6, + AnnotationStyle = 1 << 6, + AnnotationData = 1 << 7, }; inline Update operator| (const Update& lhs, const Update& rhs) { diff --git a/src/mbgl/annotation/annotation_manager.cpp b/src/mbgl/annotation/annotation_manager.cpp index e3c6cc56d46..dd2467e34f4 100644 --- a/src/mbgl/annotation/annotation_manager.cpp +++ b/src/mbgl/annotation/annotation_manager.cpp @@ -31,10 +31,9 @@ AnnotationID AnnotationManager::addAnnotation(const Annotation& annotation, cons return id; } -void AnnotationManager::updateAnnotation(const AnnotationID& id, const Annotation& annotation, const uint8_t maxZoom) { - removeAnnotation(id); - Annotation::visit(annotation, [&] (const auto& annotation_) { - this->add(id, annotation_, maxZoom); +Update AnnotationManager::updateAnnotation(const AnnotationID& id, const Annotation& annotation, const uint8_t maxZoom) { + return Annotation::visit(annotation, [&] (const auto& annotation_) { + return this->update(id, annotation_, maxZoom); }); } @@ -69,6 +68,53 @@ void AnnotationManager::add(const AnnotationID& id, const StyleSourcedAnnotation std::make_unique(id, annotation, maxZoom)); } +Update AnnotationManager::update(const AnnotationID& id, const SymbolAnnotation& annotation, const uint8_t maxZoom) { + auto it = symbolAnnotations.find(id); + if (it == symbolAnnotations.end()) { + removeAndAdd(id, annotation, maxZoom); + return Update::AnnotationData | Update::AnnotationStyle; + } + + Update result = Update::Nothing; + const SymbolAnnotation& existing = it->second->annotation; + + if (existing.geometry != annotation.geometry) { + result |= Update::AnnotationData; + } + + if (existing.icon != annotation.icon) { + result |= Update::AnnotationData | Update::AnnotationStyle; + } + + if (result != Update::Nothing) { + removeAndAdd(id, annotation, maxZoom); + } + + return result; +} + +Update AnnotationManager::update(const AnnotationID& id, const LineAnnotation& annotation, const uint8_t maxZoom) { + removeAndAdd(id, annotation, maxZoom); + return Update::AnnotationData | Update::AnnotationStyle; +} + +Update AnnotationManager::update(const AnnotationID& id, const FillAnnotation& annotation, const uint8_t maxZoom) { + removeAndAdd(id, annotation, maxZoom); + return Update::AnnotationData | Update::AnnotationStyle; +} + +Update AnnotationManager::update(const AnnotationID& id, const StyleSourcedAnnotation& annotation, const uint8_t maxZoom) { + removeAndAdd(id, annotation, maxZoom); + return Update::AnnotationData | Update::AnnotationStyle; +} + +void AnnotationManager::removeAndAdd(const AnnotationID& id, const Annotation& annotation, const uint8_t maxZoom) { + removeAnnotation(id); + Annotation::visit(annotation, [&] (const auto& annotation_) { + this->add(id, annotation_, maxZoom); + }); +} + AnnotationIDs AnnotationManager::getPointAnnotationsInBounds(const LatLngBounds& bounds) const { AnnotationIDs result; @@ -133,7 +179,9 @@ void AnnotationManager::updateStyle(Style& style) { } obsoleteShapeAnnotationLayers.clear(); +} +void AnnotationManager::updateData() { for (auto& tile : tiles) { tile->setData(getTileData(tile->id.canonical)); } diff --git a/src/mbgl/annotation/annotation_manager.hpp b/src/mbgl/annotation/annotation_manager.hpp index d2e0813be24..bdce1a8c3ab 100644 --- a/src/mbgl/annotation/annotation_manager.hpp +++ b/src/mbgl/annotation/annotation_manager.hpp @@ -4,7 +4,7 @@ #include #include #include -#include +#include #include #include @@ -14,6 +14,7 @@ namespace mbgl { +class LatLngBounds; class AnnotationTile; class AnnotationTileData; class SymbolAnnotationImpl; @@ -29,7 +30,7 @@ class AnnotationManager : private util::noncopyable { ~AnnotationManager(); AnnotationID addAnnotation(const Annotation&, const uint8_t maxZoom); - void updateAnnotation(const AnnotationID&, const Annotation&, const uint8_t maxZoom); + Update updateAnnotation(const AnnotationID&, const Annotation&, const uint8_t maxZoom); void removeAnnotation(const AnnotationID&); AnnotationIDs getPointAnnotationsInBounds(const LatLngBounds&) const; @@ -40,6 +41,7 @@ class AnnotationManager : private util::noncopyable { SpriteAtlas& getSpriteAtlas() { return spriteAtlas; } void updateStyle(style::Style&); + void updateData(); void addTile(AnnotationTile&); void removeTile(AnnotationTile&); @@ -53,6 +55,13 @@ class AnnotationManager : private util::noncopyable { void add(const AnnotationID&, const FillAnnotation&, const uint8_t); void add(const AnnotationID&, const StyleSourcedAnnotation&, const uint8_t); + Update update(const AnnotationID&, const SymbolAnnotation&, const uint8_t); + Update update(const AnnotationID&, const LineAnnotation&, const uint8_t); + Update update(const AnnotationID&, const FillAnnotation&, const uint8_t); + Update update(const AnnotationID&, const StyleSourcedAnnotation&, const uint8_t); + + void removeAndAdd(const AnnotationID&, const Annotation&, const uint8_t); + std::unique_ptr getTileData(const CanonicalTileID&); AnnotationID nextID = 0; diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp index b599268bf3f..11bc89a1a04 100644 --- a/src/mbgl/map/map.cpp +++ b/src/mbgl/map/map.cpp @@ -211,11 +211,15 @@ void Map::Impl::update() { // - Hint style sources to notify when all its tiles are loaded; timePoint = Clock::now(); - if (style->loaded && updateFlags & Update::Annotations) { + if (style->loaded && updateFlags & Update::AnnotationStyle) { annotationManager->updateStyle(*style); updateFlags |= Update::Classes; } + if (updateFlags & Update::AnnotationData) { + annotationManager->updateData(); + } + if (updateFlags & Update::Classes) { style->cascade(timePoint, mode); } @@ -338,7 +342,7 @@ void Map::Impl::loadStyleJSON(const std::string& json) { // force style cascade, causing all pending transitions to complete. style->cascade(Clock::now(), mode); - updateFlags |= Update::Classes | Update::RecalculateStyle | Update::Annotations; + updateFlags |= Update::Classes | Update::RecalculateStyle | Update::AnnotationStyle; asyncUpdate.send(); } @@ -689,18 +693,17 @@ double Map::getTopOffsetPixelsForAnnotationIcon(const std::string& name) { AnnotationID Map::addAnnotation(const Annotation& annotation) { auto result = impl->annotationManager->addAnnotation(annotation, getMaxZoom()); - update(Update::Annotations); + update(Update::AnnotationStyle | Update::AnnotationData); return result; } void Map::updateAnnotation(AnnotationID id, const Annotation& annotation) { - impl->annotationManager->updateAnnotation(id, annotation, getMaxZoom()); - update(Update::Annotations); + update(impl->annotationManager->updateAnnotation(id, annotation, getMaxZoom())); } void Map::removeAnnotation(AnnotationID annotation) { impl->annotationManager->removeAnnotation(annotation); - update(Update::Annotations); + update(Update::AnnotationStyle | Update::AnnotationData); } AnnotationIDs Map::getPointAnnotationsInBounds(const LatLngBounds& bounds) { diff --git a/src/mbgl/tile/geojson_tile.cpp b/src/mbgl/tile/geojson_tile.cpp index 9334cf2feca..e2f8b69e4d4 100644 --- a/src/mbgl/tile/geojson_tile.cpp +++ b/src/mbgl/tile/geojson_tile.cpp @@ -107,10 +107,10 @@ std::unique_ptr convertTile(const mapbox::geojsonvt::Tile& tile } GeoJSONTile::GeoJSONTile(const OverscaledTileID& overscaledTileID, - std::string sourceID, + std::string sourceID_, const style::UpdateParameters& parameters, mapbox::geojsonvt::GeoJSONVT& geojsonvt) - : GeometryTile(overscaledTileID, sourceID, parameters.style, parameters.mode) { + : GeometryTile(overscaledTileID, sourceID_, parameters.style, parameters.mode) { setData(convertTile(geojsonvt.getTile(id.canonical.z, id.canonical.x, id.canonical.y))); } diff --git a/src/mbgl/tile/vector_tile.cpp b/src/mbgl/tile/vector_tile.cpp index 2184fb24ddd..1f924a45e14 100644 --- a/src/mbgl/tile/vector_tile.cpp +++ b/src/mbgl/tile/vector_tile.cpp @@ -69,10 +69,10 @@ class VectorTileData : public GeometryTileData { }; VectorTile::VectorTile(const OverscaledTileID& id_, - std::string sourceID, + std::string sourceID_, const style::UpdateParameters& parameters, const Tileset& tileset) - : GeometryTile(id_, sourceID, parameters.style, parameters.mode), + : GeometryTile(id_, sourceID_, parameters.style, parameters.mode), loader(*this, id_, parameters, tileset) { } diff --git a/test/api/annotations.cpp b/test/api/annotations.cpp index abe3862f4e7..5b588a9e5ce 100644 --- a/test/api/annotations.cpp +++ b/test/api/annotations.cpp @@ -106,23 +106,21 @@ TEST(Annotations, NonImmediateAdd) { test.checkRendering("non_immediate_add"); } -TEST(Annotations, UpdateIcon) { +TEST(Annotations, UpdateSymbolAnnotationGeometry) { AnnotationTest test; test.map.setStyleJSON(util::read_file("test/fixtures/api/empty.json")); - test.map.addAnnotationIcon("flipped_marker", namedMarker("default_marker.png")); - test.map.addAnnotation(SymbolAnnotation { Point { 0, 0 }, "flipped_marker" }); + test.map.addAnnotationIcon("default_marker", namedMarker("default_marker.png")); + test.map.addAnnotationIcon("flipped_marker", namedMarker("flipped_marker.png")); + AnnotationID point = test.map.addAnnotation(SymbolAnnotation { Point { 0, 0 }, "default_marker" }); test::render(test.map); - test.map.removeAnnotationIcon("flipped_marker"); - test.map.addAnnotationIcon("flipped_marker", namedMarker("flipped_marker.png")); - test.map.update(Update::Annotations); - - test.checkRendering("update_icon"); + test.map.updateAnnotation(point, SymbolAnnotation { Point { -10, 0 }, "default_marker" }); + test.checkRendering("update_point"); } -TEST(Annotations, UpdatePoint) { +TEST(Annotations, UpdateSymbolAnnotationIcon) { AnnotationTest test; test.map.setStyleJSON(util::read_file("test/fixtures/api/empty.json")); @@ -132,8 +130,8 @@ TEST(Annotations, UpdatePoint) { test::render(test.map); - test.map.updateAnnotation(point, SymbolAnnotation { Point { -10, 0 }, "flipped_marker" }); - test.checkRendering("update_point"); + test.map.updateAnnotation(point, SymbolAnnotation { Point { 0, 0 }, "flipped_marker" }); + test.checkRendering("update_icon"); } TEST(Annotations, RemovePoint) { diff --git a/test/fixtures/annotations/update_icon/expected.png b/test/fixtures/annotations/update_icon/expected.png index 3b6ca22747cf1cf05a0579410785f1604e61dd42..408d681ede7839402a08cce381bdb36e535c9eb7 100644 GIT binary patch delta 22 ecmZn_|0p~`g^RH`$lZxy-8q?;n-4J_<^%v*RtRwb delta 428 zcmY*VyH3L}6un3=5JrZs#vqttCk2LDNu-JjF*E`yhIAus(-J`*N?awdqyx+_GVl{! z8R2IT8!KYKPD%hz*T?5^k3ZJ$8V_5=@}zCI0p|B>U+xM3py5%^2DsS^29lOhhQ9#^LH1J3s#6kgJ6PFAo zW(iY_B&CS?rHLK;1RX?)hxW-H(Xgc;gp6b~X1&hgLN9J6zTiA&lxA5*vRx7-6RNjb zEvhwXv#AP(nqG(8&DAj7tW177tmZ zNaYLFiAHO4AvGk*juYP#(K5i&jgt8flZ_;q%Je_mY*bxKSd_ICgVsN1DczTy{+zt7 Fegp4?fS3RP diff --git a/test/fixtures/annotations/update_point/expected.png b/test/fixtures/annotations/update_point/expected.png index 02cf77df8defa6ca7bbd9008507b3d6c07f0da0a..30aa39e38069168202758573d4b67adccff87e78 100644 GIT binary patch delta 1542 zcmV+h2Ko8O6Z#X7F@Lm4L_t(|0qxygh+J140Ps6AoBho04m15QLG7kmQkql(z62j6 zD2PZQKKb0dsl|sv>ANo?h0^-a>Pv7zMDQsAeJCh|3Mxpkt=*6`F>KZjMUR_jbwES`R*R-0a(=Z(OY0s_F6G`*SeYPI_PR;%?= zb?@G(!~6DSm9eq*D|)+d>sI^q&ef|~?dHwdLZR^Nk88D4y*U!S>IDG-pcj_H4*T#* zrSj{c(!kT-eC)C8%YW6%wzaO*7P8kbT*yAYvoih6!orDd3(M?Vfl}X&JZA%cwNi<_ z{tv%*_)zxX#Q0r%KRG&*J^H`{+53&gk#Chsaa zHvYZC!#nQr;J`rk#d0~jaP7mR-yIo#D+PgbJFcu?UkDWYVt(VgdS0s=sfuLH67PaE;nS08$4Lr>qhb}jqu`SaO7(uKfy z#dOT;PEj}7h_<@({N3LI1N}Yx-XDG1b00svfB%M##)kf0+OU5xJ)Kn^f4rT)aPD08 z=jWf#vXuAru77|LZT$VxrQ@lfy8;_D`*(&TfZkvBN2+P>-kIW1|Lytt?AtHA@c)fJ z@*@z$yvXZ59awGSNM}2y2?zkazYZMv!oo>dh=GAm%zjpR)7U%rkDSzfgUXuzy#N4gJY#HLInEfFe~79^6nS=Eog@)J7I<#orQvH36ppYZ5*B{-b(*Ha*67 z>TLRk-)1Qe_4lRQgz@S=>|ZP2uAACux3xCYJAou10OVjbXqHO9cxUdt4fpnU()HN; zN8M;+C*_stF987{2g~~>^YdqxS8ku#>L_5lA%8@js2go;S597k3J3r>S?=iy{)sm( zU(UL332xW@C>wR6?snzm^{;>ckdtK}=kMv)TV8(s;>8{JILbyH=l(k;>$`#iK#msq z$Ia$XuU@}CJF~?F!Oo#XnJ61|I_H}v&?z7QbZYeY^t1JPy;&@t_~+#-+nx%30F5OqAWtXQ+`28AOL*2zW1c{Lx&Cxj*X2SE`OCu?H>lXQ7kTxwp#yc)@t9`UoH>r9U5wX zPW%$^U;cfu@%HfWb8ig}wx1j-7K@`P2s}7CI(nngXxzRh9n#3>MnC|_!J-@=AOA|) zkk@^^Z+6d~+9N9~3yUAlAB$iAU0zwqUQ6EvxKb+q{10Pef9o7ps#_cw7#Ln$Tz{-} z&Nod!0LZ~QP^na=HtXih(9pXFmzKsCZ`^qDQmvNF-Cq9PZzd+DH!GX|NytzN0GmHg zyjcbF`a-~$fpWf>Pfkw0nC|oMi(#BBmtV~JaCHAZ3kU%B8GJV%mwp(uk@nzQ{x^_c zmdnrGt=(wjZe`^6pMcwdIauaPOIk~FcT(DISEKI8$jIz&>#O(q6-ZBtN;wgT+kjt6 zcLJ)Pq`zs~ug%WR{wLMhbpZJ!pQmiT1O$M5Ew3zY38o{0ayq0hZ*^L*ddg5A9v-gG s%*-t3Y>2u`vjG9q0g?e%7}g2=53nrhx0^u(TmS$707*qoM6N<$f=n(kM*si- delta 1493 zcmV;`1uFXb6UY;gF@J(dL_t(|0qxyQj2qP*0PwLl+3ZKinso@>C3KCH1)9)`3JIY~ zKoC^wfhsxm zsZ?$s85t?Br;5#Hv%0vr_}`SLxt@H|4u^maU;vKu?Ck8s=;-L3>Ea*RNOe;u(43r{ zJYh7j5xuRQr?|D7w>4UqhVp-VH+u1hK;&IsUao8n4XwD50y=;jsri2@Ypr|z&f7g* zum3LA&VH}g>y^$9oFJpFeijU0JNL9bvuFu2n{_w8Dl*KnE}Y$9jBxd@&~Sx~qTs$nx@?vtNGs ztH<_#C)+tT)?Oc)pU)yJ;=0pD-t_43gYGcf1VUg#v&!f;S72#rsadbrm!_tswik=V ziQAjan;!o3)8F4eH#fWQmRl}YW@=(QOYu9tsMWrEdi(a*&liiIr)xhR11>Zg4c`a2 zEM=o3pabYg^#5xsbpn6&^hmJ!vyVUi&D^eCKYVcCzN>2etzEma4{EjY%*Bh%cS@xr z3kwU?l&E!8LIDJlfLnk8I}M)t@WUTW6^c(j)Z^msG$rN(B2A?2gn1>lte1GoS?R);Usu^);iKe z+Q@U=#1agxfDT|l4*j1?rAoSy|HZY2{`sav+Q<`m^KteGgn$lUK+c0!p>X8Bxw&f& z{qs$UTY<AQdLZpYV7p3L5S{`o9RardRT z?j_1Nb@uF`6!ubgxb5KQPvd&*%^8P9YFt|0;PDI*S%C%RYy zW46CD;TOaH{3m~;Ih8t!e=IC4K3l8Z5&ISb9Rarh9f>}EO`qJqeCyxuXBToa`%&e% z;Aheofnx1-@kgG>8)fuy6IJaS67Z8?-w^zI-uBhy<&!@e+fh2xXgsht{l3TOwy$^R z78ig28|mAD7d~5g>6u#X59@hPX@^5V2QVPVIduZBJu)&r(} zTJ~C+6KH(4ax@M7pT1UFal