From f27c28b6c4a4796eded7984d7bb3c62f30ba594f Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 30 Nov 2015 12:48:30 -0800 Subject: [PATCH 1/2] Partially revert "[core] Source should receive a ref to MapData just once" This partially reverts commit d55aa7929cb10d40a58b6b7a8ed73bddd4f0a407. --- src/mbgl/annotation/annotation_manager.cpp | 6 ++---- src/mbgl/annotation/annotation_manager.hpp | 4 +--- src/mbgl/map/map_data.hpp | 1 - src/mbgl/map/source.cpp | 16 +++++++++------- src/mbgl/map/source.hpp | 12 ++++++------ src/mbgl/style/style.cpp | 4 ++-- src/mbgl/style/style_parser.cpp | 7 +------ src/mbgl/style/style_parser.hpp | 5 ----- test/miscellaneous/style_parser.cpp | 5 +---- 9 files changed, 22 insertions(+), 38 deletions(-) diff --git a/src/mbgl/annotation/annotation_manager.cpp b/src/mbgl/annotation/annotation_manager.cpp index c6d5413ec08..5b1138a14af 100644 --- a/src/mbgl/annotation/annotation_manager.cpp +++ b/src/mbgl/annotation/annotation_manager.cpp @@ -1,6 +1,5 @@ #include #include -#include #include #include @@ -11,8 +10,7 @@ namespace mbgl { const std::string AnnotationManager::SourceID = "com.mapbox.annotations"; const std::string AnnotationManager::PointLayerID = "com.mapbox.annotations.points"; -AnnotationManager::AnnotationManager(MapData& data_) : data(data_) {} - +AnnotationManager::AnnotationManager() = default; AnnotationManager::~AnnotationManager() = default; AnnotationIDs @@ -110,7 +108,7 @@ std::unique_ptr AnnotationManager::getTile(const TileID& tileID) void AnnotationManager::updateStyle(Style& style) { // Create annotation source, point layer, and point bucket if (!style.getSource(SourceID)) { - std::unique_ptr source = std::make_unique(data); + std::unique_ptr source = std::make_unique(); source->info.type = SourceType::Annotations; source->info.source_id = SourceID; source->enabled = true; diff --git a/src/mbgl/annotation/annotation_manager.hpp b/src/mbgl/annotation/annotation_manager.hpp index 16ebd167165..f1b41c9ccce 100644 --- a/src/mbgl/annotation/annotation_manager.hpp +++ b/src/mbgl/annotation/annotation_manager.hpp @@ -13,7 +13,6 @@ namespace mbgl { -class MapData; class PointAnnotation; class ShapeAnnotation; class AnnotationTile; @@ -22,7 +21,7 @@ class Style; class AnnotationManager : private util::noncopyable { public: - AnnotationManager(MapData&); + AnnotationManager(); ~AnnotationManager(); AnnotationIDs addPointAnnotations(const std::vector&, const uint8_t maxZoom); @@ -43,7 +42,6 @@ class AnnotationManager : private util::noncopyable { private: std::unique_ptr getTile(const TileID&); - MapData& data; AnnotationID nextID = 0; PointAnnotationImpl::Tree pointTree; PointAnnotationImpl::Map pointAnnotations; diff --git a/src/mbgl/map/map_data.hpp b/src/mbgl/map/map_data.hpp index d8c1e5cb0db..43765e25b2d 100644 --- a/src/mbgl/map/map_data.hpp +++ b/src/mbgl/map/map_data.hpp @@ -24,7 +24,6 @@ class MapData { : mode(mode_) , contextMode(contextMode_) , pixelRatio(pixelRatio_) - , annotationManager(*this) , animationTime(Duration::zero()) , defaultFadeDuration(mode_ == MapMode::Continuous ? std::chrono::milliseconds(300) : Duration::zero()) , defaultTransitionDuration(Duration::zero()) diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index 4e8e60de511..c7cfb1f8c92 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -119,7 +119,7 @@ std::string SourceInfo::tileURL(const TileID& id, float pixelRatio) const { return result; } -Source::Source(MapData& data_) : data(data_) {} +Source::Source() {} Source::~Source() = default; @@ -246,7 +246,8 @@ bool Source::handlePartialTile(const TileID& id, Worker&) { }); } -TileData::State Source::addTile(const TransformState& transformState, +TileData::State Source::addTile(MapData& data, + const TransformState& transformState, Style& style, TexturePool& texturePool, const TileID& id) { @@ -280,7 +281,7 @@ TileData::State Source::addTile(const TransformState& transformState, } if (!new_tile.data) { - auto callback = std::bind(&Source::tileLoadingCompleteCallback, this, normalized_id, transformState); + auto callback = std::bind(&Source::tileLoadingCompleteCallback, this, normalized_id, transformState, data.getDebug() & MapDebugOptions::Collision); // If we don't find working tile data, we're just going to load it. if (info.type == SourceType::Raster) { @@ -407,7 +408,8 @@ void Source::findLoadedParent(const TileID& id, int32_t minCoveringZoom, std::fo } } -bool Source::update(const TransformState& transformState, +bool Source::update(MapData& data, + const TransformState& transformState, Style& style, TexturePool& texturePool, bool shouldReparsePartialTiles) { @@ -447,7 +449,7 @@ bool Source::update(const TransformState& transformState, } break; case TileData::State::invalid: - state = addTile(transformState, style, texturePool, id); + state = addTile(data, transformState, style, texturePool, id); break; default: break; @@ -546,7 +548,7 @@ void Source::setObserver(Observer* observer) { observer_ = observer; } -void Source::tileLoadingCompleteCallback(const TileID& normalized_id, const TransformState& transformState) { +void Source::tileLoadingCompleteCallback(const TileID& normalized_id, const TransformState& transformState, bool collisionDebug) { auto it = tileDataMap.find(normalized_id); if (it == tileDataMap.end()) { return; @@ -562,7 +564,7 @@ void Source::tileLoadingCompleteCallback(const TileID& normalized_id, const Tran return; } - tileData->redoPlacement({ transformState.getAngle(), transformState.getPitch(), data.getDebug() & MapDebugOptions::Collision}); + tileData->redoPlacement({ transformState.getAngle(), transformState.getPitch(), collisionDebug }); emitTileLoaded(true); } diff --git a/src/mbgl/map/source.hpp b/src/mbgl/map/source.hpp index 5f4bfe0c61e..6f2e2794288 100644 --- a/src/mbgl/map/source.hpp +++ b/src/mbgl/map/source.hpp @@ -62,7 +62,7 @@ class Source : private util::noncopyable { virtual void onTileLoadingFailed(std::exception_ptr error) = 0; }; - Source(MapData&); + Source(); ~Source(); void load(); @@ -72,7 +72,8 @@ class Source : private util::noncopyable { // will return true if all the tiles were scheduled for updating of false if // they were not. shouldReparsePartialTiles must be set to "true" if there is // new data available that a tile in the "partial" state might be interested at. - bool update(const TransformState&, + bool update(MapData&, + const TransformState&, Style&, TexturePool&, bool shouldReparsePartialTiles); @@ -94,7 +95,7 @@ class Source : private util::noncopyable { bool enabled; private: - void tileLoadingCompleteCallback(const TileID&, const TransformState&); + void tileLoadingCompleteCallback(const TileID&, const TransformState&, bool collisionDebug); void emitSourceLoaded(); void emitSourceLoadingFailed(const std::string& message); @@ -107,7 +108,8 @@ class Source : private util::noncopyable { int32_t coveringZoomLevel(const TransformState&) const; std::forward_list coveringTiles(const TransformState&) const; - TileData::State addTile(const TransformState&, + TileData::State addTile(MapData&, + const TransformState&, Style&, TexturePool&, const TileID&); @@ -117,8 +119,6 @@ class Source : private util::noncopyable { double getZoom(const TransformState &state) const; - MapData& data; - bool loaded = false; // Stores the time when this source was most recently updated. diff --git a/src/mbgl/style/style.cpp b/src/mbgl/style/style.cpp index b7533db4a35..16a9fddd95c 100644 --- a/src/mbgl/style/style.cpp +++ b/src/mbgl/style/style.cpp @@ -43,7 +43,7 @@ void Style::setJSON(const std::string& json, const std::string&) { return; } - StyleParser parser(data); + StyleParser parser; parser.parse(doc); for (auto& source : parser.getSources()) { @@ -105,7 +105,7 @@ void Style::update(const TransformState& transform, TexturePool& texturePool) { bool allTilesUpdated = true; for (const auto& source : sources) { - if (!source->update(transform, *this, texturePool, shouldReparsePartialTiles)) { + if (!source->update(data, transform, *this, texturePool, shouldReparsePartialTiles)) { allTilesUpdated = false; } } diff --git a/src/mbgl/style/style_parser.cpp b/src/mbgl/style/style_parser.cpp index 2eb9562c904..dc14827dc6d 100644 --- a/src/mbgl/style/style_parser.cpp +++ b/src/mbgl/style/style_parser.cpp @@ -1,17 +1,12 @@ #include #include -#include #include #include namespace mbgl { -StyleParser::StyleParser(MapData& data_) - : data(data_) { -} - void StyleParser::parse(const JSVal& document) { if (document.HasMember("version")) { version = document["version"].GetInt(); @@ -54,7 +49,7 @@ void StyleParser::parseSources(const JSVal& value) { const JSVal& nameVal = itr->name; const JSVal& sourceVal = itr->value; - std::unique_ptr source = std::make_unique(data); + std::unique_ptr source = std::make_unique(); source->info.source_id = { nameVal.GetString(), nameVal.GetStringLength() }; diff --git a/src/mbgl/style/style_parser.hpp b/src/mbgl/style/style_parser.hpp index 126a3781dbf..489c9959f25 100644 --- a/src/mbgl/style/style_parser.hpp +++ b/src/mbgl/style/style_parser.hpp @@ -14,7 +14,6 @@ namespace mbgl { -class MapData; class StyleLayer; class Source; @@ -22,8 +21,6 @@ using JSVal = rapidjson::Value; class StyleParser { public: - StyleParser(MapData&); - void parse(const JSVal&); std::vector>&& getSources() { @@ -48,8 +45,6 @@ class StyleParser { void parseLayer(const std::string& id, const JSVal&, util::ptr&); void parseVisibility(StyleLayer&, const JSVal& value); - MapData& data; - std::uint8_t version; std::vector> sources; diff --git a/test/miscellaneous/style_parser.cpp b/test/miscellaneous/style_parser.cpp index 6646ba82db0..7a38ba054d4 100644 --- a/test/miscellaneous/style_parser.cpp +++ b/test/miscellaneous/style_parser.cpp @@ -1,6 +1,5 @@ #include "../fixtures/util.hpp" -#include #include #include @@ -36,9 +35,7 @@ TEST_P(StyleParserTest, ParseStyle) { FixtureLogObserver* observer = new FixtureLogObserver(); Log::setObserver(std::unique_ptr(observer)); - double fakePixelRatio = 1.0; - MapData data(MapMode::Continuous, GLContextMode::Unique, fakePixelRatio); - StyleParser parser(data); + StyleParser parser; parser.parse(styleDoc); for (auto it = infoDoc.MemberBegin(), end = infoDoc.MemberEnd(); it != end; it++) { From 40ebf5d0d08138c353d7c0e4bc61ee53dceae410 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 30 Nov 2015 13:05:51 -0800 Subject: [PATCH 2/2] [core] Introduce StyleUpdateParameters --- src/mbgl/annotation/annotation_tile.hpp | 1 + src/mbgl/map/source.cpp | 49 +++++++++------------ src/mbgl/map/source.hpp | 19 ++------ src/mbgl/style/style.cpp | 13 +++++- src/mbgl/style/style.hpp | 3 ++ src/mbgl/style/style_update_parameters.hpp | 50 ++++++++++++++++++++++ 6 files changed, 91 insertions(+), 44 deletions(-) create mode 100644 src/mbgl/style/style_update_parameters.hpp diff --git a/src/mbgl/annotation/annotation_tile.hpp b/src/mbgl/annotation/annotation_tile.hpp index a8bb2c5677f..e88d6d55cf4 100644 --- a/src/mbgl/annotation/annotation_tile.hpp +++ b/src/mbgl/annotation/annotation_tile.hpp @@ -42,6 +42,7 @@ class MapData; class AnnotationTileMonitor : public GeometryTileMonitor { public: + // TODO: should just take AnnotationManager&, but we need to eliminate util::exclusive from MapData first. AnnotationTileMonitor(const TileID&, MapData&); ~AnnotationTileMonitor(); diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index c7cfb1f8c92..e1c2eb44f28 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -246,11 +247,7 @@ bool Source::handlePartialTile(const TileID& id, Worker&) { }); } -TileData::State Source::addTile(MapData& data, - const TransformState& transformState, - Style& style, - TexturePool& texturePool, - const TileID& id) { +TileData::State Source::addTile(const TileID& id, const StyleUpdateParameters& parameters) { const TileData::State state = hasTile(id); if (state != TileData::State::invalid) { @@ -281,24 +278,24 @@ TileData::State Source::addTile(MapData& data, } if (!new_tile.data) { - auto callback = std::bind(&Source::tileLoadingCompleteCallback, this, normalized_id, transformState, data.getDebug() & MapDebugOptions::Collision); + auto callback = std::bind(&Source::tileLoadingCompleteCallback, this, normalized_id, parameters.transformState, parameters.debugOptions & MapDebugOptions::Collision); // If we don't find working tile data, we're just going to load it. if (info.type == SourceType::Raster) { auto tileData = std::make_shared(normalized_id, - texturePool, + parameters.texturePool, info, - style.workers); + parameters.worker); - tileData->request(data.pixelRatio, callback); + tileData->request(parameters.pixelRatio, callback); new_tile.data = tileData; } else { std::unique_ptr monitor; if (info.type == SourceType::Vector) { - monitor = std::make_unique(info, normalized_id, data.pixelRatio); + monitor = std::make_unique(info, normalized_id, parameters.pixelRatio); } else if (info.type == SourceType::Annotations) { - monitor = std::make_unique(normalized_id, data); + monitor = std::make_unique(normalized_id, parameters.data); } else { throw std::runtime_error("source type not implemented"); } @@ -306,7 +303,7 @@ TileData::State Source::addTile(MapData& data, new_tile.data = std::make_shared(normalized_id, std::move(monitor), info.source_id, - style, + parameters.style, callback); } @@ -408,24 +405,20 @@ void Source::findLoadedParent(const TileID& id, int32_t minCoveringZoom, std::fo } } -bool Source::update(MapData& data, - const TransformState& transformState, - Style& style, - TexturePool& texturePool, - bool shouldReparsePartialTiles) { +bool Source::update(const StyleUpdateParameters& parameters) { bool allTilesUpdated = true; - if (!loaded || data.getAnimationTime() <= updated) { + if (!loaded || parameters.animationTime <= updated) { return allTilesUpdated; } - double zoom = getZoom(transformState); + double zoom = getZoom(parameters.transformState); if (info.type == SourceType::Raster || info.type == SourceType::Video) { zoom = ::round(zoom); } else { zoom = std::floor(zoom); } - std::forward_list required = coveringTiles(transformState); + std::forward_list required = coveringTiles(parameters.transformState); // Determine the overzooming/underzooming amounts. int32_t minCoveringZoom = util::clamp(zoom - 10, info.min_zoom, info.max_zoom); @@ -442,14 +435,14 @@ bool Source::update(MapData& data, switch (state) { case TileData::State::partial: - if (shouldReparsePartialTiles) { - if (!handlePartialTile(id, style.workers)) { + if (parameters.shouldReparsePartialTiles) { + if (!handlePartialTile(id, parameters.worker)) { allTilesUpdated = false; } } break; case TileData::State::invalid: - state = addTile(data, transformState, style, texturePool, id); + state = addTile(id, parameters); break; default: break; @@ -472,9 +465,9 @@ bool Source::update(MapData& data, } if (info.type != SourceType::Raster && cache.getSize() == 0) { - size_t conservativeCacheSize = ((float)transformState.getWidth() / util::tileSize) * - ((float)transformState.getHeight() / util::tileSize) * - (transformState.getMaxZoom() - transformState.getMinZoom() + 1) * + size_t conservativeCacheSize = ((float)parameters.transformState.getWidth() / util::tileSize) * + ((float)parameters.transformState.getHeight() / util::tileSize) * + (parameters.transformState.getMaxZoom() - parameters.transformState.getMinZoom() + 1) * 0.5; cache.setSize(conservativeCacheSize); } @@ -521,10 +514,10 @@ bool Source::update(MapData& data, for (auto& tilePtr : tilePtrs) { tilePtr->data->redoPlacement( - { transformState.getAngle(), transformState.getPitch(), data.getDebug() & MapDebugOptions::Collision }); + { parameters.transformState.getAngle(), parameters.transformState.getPitch(), parameters.debugOptions & MapDebugOptions::Collision }); } - updated = data.getAnimationTime(); + updated = parameters.animationTime; return allTilesUpdated; } diff --git a/src/mbgl/map/source.hpp b/src/mbgl/map/source.hpp index 6f2e2794288..d7009f6615a 100644 --- a/src/mbgl/map/source.hpp +++ b/src/mbgl/map/source.hpp @@ -22,9 +22,7 @@ namespace mbgl { -class MapData; -class TexturePool; -class Style; +class StyleUpdateParameters; class Painter; class FileRequest; class TransformState; @@ -72,11 +70,7 @@ class Source : private util::noncopyable { // will return true if all the tiles were scheduled for updating of false if // they were not. shouldReparsePartialTiles must be set to "true" if there is // new data available that a tile in the "partial" state might be interested at. - bool update(MapData&, - const TransformState&, - Style&, - TexturePool&, - bool shouldReparsePartialTiles); + bool update(const StyleUpdateParameters&); void updateMatrices(const mat4 &projMatrix, const TransformState &transform); void drawClippingMasks(Painter &painter); @@ -108,13 +102,8 @@ class Source : private util::noncopyable { int32_t coveringZoomLevel(const TransformState&) const; std::forward_list coveringTiles(const TransformState&) const; - TileData::State addTile(MapData&, - const TransformState&, - Style&, - TexturePool&, - const TileID&); - - TileData::State hasTile(const TileID& id); + TileData::State addTile(const TileID&, const StyleUpdateParameters&); + TileData::State hasTile(const TileID&); void updateTilePtrs(); double getZoom(const TransformState &state) const; diff --git a/src/mbgl/style/style.cpp b/src/mbgl/style/style.cpp index 16a9fddd95c..69f28e86bb2 100644 --- a/src/mbgl/style/style.cpp +++ b/src/mbgl/style/style.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -104,8 +105,18 @@ void Style::removeLayer(const std::string& id) { void Style::update(const TransformState& transform, TexturePool& texturePool) { bool allTilesUpdated = true; + StyleUpdateParameters parameters(data.pixelRatio, + data.getDebug(), + data.getAnimationTime(), + transform, + workers, + texturePool, + shouldReparsePartialTiles, + data, + *this); + for (const auto& source : sources) { - if (!source->update(data, transform, *this, texturePool, shouldReparsePartialTiles)) { + if (!source->update(parameters)) { allTilesUpdated = false; } } diff --git a/src/mbgl/style/style.hpp b/src/mbgl/style/style.hpp index e1113b3cd6b..c2cb9e397ca 100644 --- a/src/mbgl/style/style.hpp +++ b/src/mbgl/style/style.hpp @@ -19,12 +19,15 @@ namespace mbgl { +class MapData; class GlyphAtlas; class GlyphStore; class SpriteStore; class SpriteAtlas; class LineAtlas; class StyleLayer; +class TransformState; +class TexturePool; class Style : public GlyphStore::Observer, public SpriteStore::Observer, diff --git a/src/mbgl/style/style_update_parameters.hpp b/src/mbgl/style/style_update_parameters.hpp new file mode 100644 index 00000000000..7a0f34bf56f --- /dev/null +++ b/src/mbgl/style/style_update_parameters.hpp @@ -0,0 +1,50 @@ +#ifndef STYLE_UPDATE_PARAMETERS +#define STYLE_UPDATE_PARAMETERS + +#include + +namespace mbgl { + +class TransformState; +class Worker; +class TexturePool; +class MapData; +class Style; + +class StyleUpdateParameters { +public: + StyleUpdateParameters(float pixelRatio_, + MapDebugOptions debugOptions_, + TimePoint animationTime_, + const TransformState& transformState_, + Worker& worker_, + TexturePool& texturePool_, + bool shouldReparsePartialTiles_, + MapData& data_, + Style& style_) + : pixelRatio(pixelRatio_), + debugOptions(debugOptions_), + animationTime(animationTime_), + transformState(transformState_), + worker(worker_), + texturePool(texturePool_), + shouldReparsePartialTiles(shouldReparsePartialTiles_), + data(data_), + style(style_) {} + + float pixelRatio; + MapDebugOptions debugOptions; + TimePoint animationTime; + const TransformState& transformState; + Worker& worker; + TexturePool& texturePool; + bool shouldReparsePartialTiles; + + // TODO: remove + MapData& data; + Style& style; +}; + +} + +#endif