From 34c33737e3e2b67ec0e64582bc2374da1705e05c Mon Sep 17 00:00:00 2001 From: Alexander Shalamov Date: Thu, 18 Oct 2018 10:54:24 +0300 Subject: [core] Share GeoJSONData pointer as weak_ptr instead of raw ptr GeoJSONSource::Impl shares its private data as a raw pointer, this may lead to unwanted side-effects. Moreover GeoJSONSource, used to access its Impl data without checking whether data is valid, this PR fixes both issues. --- src/mbgl/renderer/sources/render_geojson_source.cpp | 12 ++++++------ src/mbgl/renderer/sources/render_geojson_source.hpp | 2 +- src/mbgl/style/sources/geojson_source.cpp | 18 +++++++++++++++--- src/mbgl/style/sources/geojson_source_impl.cpp | 10 +++++----- src/mbgl/style/sources/geojson_source_impl.hpp | 4 ++-- 5 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/mbgl/renderer/sources/render_geojson_source.cpp b/src/mbgl/renderer/sources/render_geojson_source.cpp index 0e265efff4..cd5cead6dc 100644 --- a/src/mbgl/renderer/sources/render_geojson_source.cpp +++ b/src/mbgl/renderer/sources/render_geojson_source.cpp @@ -33,23 +33,23 @@ void RenderGeoJSONSource::update(Immutable baseImpl_, enabled = needsRendering; - GeoJSONData* data_ = impl().getData(); + auto data_ = impl().getData().lock(); - if (data_ != data) { + if (data.lock() != data_) { data = data_; tilePyramid.cache.clear(); - if (data) { + if (data_) { const uint8_t maxZ = impl().getZoomRange().max; for (const auto& pair : tilePyramid.tiles) { if (pair.first.canonical.z <= maxZ) { - static_cast(pair.second.get())->updateData(data->getTile(pair.first.canonical)); + static_cast(pair.second.get())->updateData(data_->getTile(pair.first.canonical)); } } } } - if (!data) { + if (!data_) { tilePyramid.tiles.clear(); tilePyramid.renderTiles.clear(); return; @@ -63,7 +63,7 @@ void RenderGeoJSONSource::update(Immutable baseImpl_, util::tileSize, impl().getZoomRange(), optional{}, - [&] (const OverscaledTileID& tileID) { + [&, data = data_] (const OverscaledTileID& tileID) { return std::make_unique(tileID, impl().id, parameters, data->getTile(tileID.canonical)); }); } diff --git a/src/mbgl/renderer/sources/render_geojson_source.hpp b/src/mbgl/renderer/sources/render_geojson_source.hpp index 297fa09a29..6351c5f6f8 100644 --- a/src/mbgl/renderer/sources/render_geojson_source.hpp +++ b/src/mbgl/renderer/sources/render_geojson_source.hpp @@ -44,7 +44,7 @@ private: const style::GeoJSONSource::Impl& impl() const; TilePyramid tilePyramid; - style::GeoJSONData* data = nullptr; + std::weak_ptr data; }; template <> diff --git a/src/mbgl/style/sources/geojson_source.cpp b/src/mbgl/style/sources/geojson_source.cpp index 79ab6b33e5..53734b91a7 100644 --- a/src/mbgl/style/sources/geojson_source.cpp +++ b/src/mbgl/style/sources/geojson_source.cpp @@ -41,17 +41,29 @@ optional GeoJSONSource::getURL() const { } mapbox::geometry::feature_collection GeoJSONSource::getClusterChildren(const std::uint32_t cluster_id) const { - return impl().getData()->getClusterChildren(cluster_id); + if (auto data = impl().getData().lock()) { + return data->getClusterChildren(cluster_id); + } + + return {}; } mapbox::geometry::feature_collection GeoJSONSource::getClusterLeaves(const std::uint32_t cluster_id, const std::uint32_t limit, const std::uint32_t offset) const { - return impl().getData()->getClusterLeaves(cluster_id, limit, offset); + if (auto data = impl().getData().lock()) { + return data->getClusterLeaves(cluster_id, limit, offset); + } + + return {}; } std::uint8_t GeoJSONSource::getClusterExpansionZoom(std::uint32_t cluster_id) const { - return impl().getData()->getClusterExpansionZoom(cluster_id); + if (auto data = impl().getData().lock()) { + return data->getClusterExpansionZoom(cluster_id); + } + + return 0; } void GeoJSONSource::loadDescription(FileSource& fileSource) { diff --git a/src/mbgl/style/sources/geojson_source_impl.cpp b/src/mbgl/style/sources/geojson_source_impl.cpp index 84f462e410..a89b093fc0 100644 --- a/src/mbgl/style/sources/geojson_source_impl.cpp +++ b/src/mbgl/style/sources/geojson_source_impl.cpp @@ -76,7 +76,7 @@ GeoJSONSource::Impl::Impl(std::string id_, GeoJSONOptions options_) GeoJSONSource::Impl::Impl(const Impl& other, const GeoJSON& geoJSON) : Source::Impl(other), options(other.options) { - double scale = util::EXTENT / util::tileSize; + constexpr double scale = util::EXTENT / util::tileSize; if (options.cluster && geoJSON.is>() @@ -85,7 +85,7 @@ GeoJSONSource::Impl::Impl(const Impl& other, const GeoJSON& geoJSON) clusterOptions.maxZoom = options.clusterMaxZoom; clusterOptions.extent = util::EXTENT; clusterOptions.radius = ::round(scale * options.clusterRadius); - data = std::make_unique( + data = std::make_shared( geoJSON.get>(), clusterOptions); } else { mapbox::geojsonvt::Options vtOptions; @@ -94,7 +94,7 @@ GeoJSONSource::Impl::Impl(const Impl& other, const GeoJSON& geoJSON) vtOptions.buffer = ::round(scale * options.buffer); vtOptions.tolerance = scale * options.tolerance; vtOptions.lineMetrics = options.lineMetrics; - data = std::make_unique(geoJSON, vtOptions); + data = std::make_shared(geoJSON, vtOptions); } } @@ -104,8 +104,8 @@ Range GeoJSONSource::Impl::getZoomRange() const { return { options.minzoom, options.maxzoom }; } -GeoJSONData* GeoJSONSource::Impl::getData() const { - return data.get(); +std::weak_ptr GeoJSONSource::Impl::getData() const { + return data; } optional GeoJSONSource::Impl::getAttribution() const { diff --git a/src/mbgl/style/sources/geojson_source_impl.hpp b/src/mbgl/style/sources/geojson_source_impl.hpp index 8fa40c78e9..0e1dcab108 100644 --- a/src/mbgl/style/sources/geojson_source_impl.hpp +++ b/src/mbgl/style/sources/geojson_source_impl.hpp @@ -30,13 +30,13 @@ public: ~Impl() final; Range getZoomRange() const; - GeoJSONData* getData() const; + std::weak_ptr getData() const; optional getAttribution() const final; private: GeoJSONOptions options; - std::unique_ptr data; + std::shared_ptr data; }; } // namespace style -- cgit v1.2.1