diff --git a/include/mbgl/style/sources/geojson_source.hpp b/include/mbgl/style/sources/geojson_source.hpp index 1cc104b24..9569b61df 100644 --- a/include/mbgl/style/sources/geojson_source.hpp +++ b/include/mbgl/style/sources/geojson_source.hpp @@ -39,17 +39,19 @@ struct GeoJSONOptions { }; class GeoJSONData { public: + using TileFeatures = mapbox::feature::feature_collection; + using Features = mapbox::feature::feature_collection; static std::shared_ptr create(const GeoJSON&, Immutable = GeoJSONOptions::defaultOptions()); virtual ~GeoJSONData() = default; - virtual mapbox::feature::feature_collection getTile(const CanonicalTileID&) = 0; + virtual void getTile(const CanonicalTileID&, const std::function&) = 0; // SuperclusterData - virtual mapbox::feature::feature_collection getChildren(const std::uint32_t) = 0; - virtual mapbox::feature::feature_collection getLeaves(const std::uint32_t, - const std::uint32_t limit = 10u, - const std::uint32_t offset = 0u) = 0; + virtual Features getChildren(const std::uint32_t) = 0; + virtual Features getLeaves(const std::uint32_t, + const std::uint32_t limit = 10u, + const std::uint32_t offset = 0u) = 0; virtual std::uint8_t getClusterExpansionZoom(std::uint32_t) = 0; }; diff --git a/src/mbgl/renderer/sources/render_geojson_source.cpp b/src/mbgl/renderer/sources/render_geojson_source.cpp index 4ed9dd933..94d171cf1 100644 --- a/src/mbgl/renderer/sources/render_geojson_source.cpp +++ b/src/mbgl/renderer/sources/render_geojson_source.cpp @@ -95,8 +95,7 @@ void RenderGeoJSONSource::update(Immutable baseImpl_, const uint8_t maxZ = impl().getZoomRange().max; for (const auto& pair : tilePyramid.getTiles()) { if (pair.first.canonical.z <= maxZ) { - static_cast(pair.second.get()) - ->updateData(data_->getTile(pair.first.canonical), needsRelayout); + static_cast(pair.second.get())->updateData(data_, needsRelayout); } } } @@ -112,8 +111,8 @@ void RenderGeoJSONSource::update(Immutable baseImpl_, util::tileSize, impl().getZoomRange(), optional{}, - [&, data_] (const OverscaledTileID& tileID) { - return std::make_unique(tileID, impl().id, parameters, data_->getTile(tileID.canonical)); + [&, data_](const OverscaledTileID& tileID) { + return std::make_unique(tileID, impl().id, parameters, data_); }); } diff --git a/src/mbgl/style/sources/geojson_source_impl.cpp b/src/mbgl/style/sources/geojson_source_impl.cpp index 962d6bd06..e4b48223f 100644 --- a/src/mbgl/style/sources/geojson_source_impl.cpp +++ b/src/mbgl/style/sources/geojson_source_impl.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -18,18 +19,16 @@ public: : impl(geoJSON, options) { } - mapbox::feature::feature_collection getTile(const CanonicalTileID& tileID) final { - return impl.getTile(tileID.z, tileID.x, tileID.y).features; + void getTile(const CanonicalTileID& id, const std::function& fn) final { + assert(fn); + // It's safe to pass `this` as scheduler will die earlier. + scheduler.scheduleAndReplyValue( + [id, this]() -> TileFeatures { return impl.getTile(id.z, id.x, id.y).features; }, fn); } - mapbox::feature::feature_collection getChildren(const std::uint32_t) final { - return {}; - } + Features getChildren(const std::uint32_t) final { return {}; } - mapbox::feature::feature_collection - getLeaves(const std::uint32_t, const std::uint32_t, const std::uint32_t) final { - return {}; - } + Features getLeaves(const std::uint32_t, const std::uint32_t, const std::uint32_t) final { return {}; } std::uint8_t getClusterExpansionZoom(std::uint32_t) final { return 0; @@ -37,26 +36,22 @@ public: private: mapbox::geojsonvt::GeoJSONVT impl; + SequencedScheduler scheduler; }; class SuperclusterData : public GeoJSONData { public: - SuperclusterData(const mapbox::feature::feature_collection& features, - const mapbox::supercluster::Options& options) - : impl(features, options) { + SuperclusterData(const Features& features, const mapbox::supercluster::Options& options) + : impl(features, options) {} + + void getTile(const CanonicalTileID& id, const std::function& fn) final { + assert(fn); + fn(impl.getTile(id.z, id.x, id.y)); } - mapbox::feature::feature_collection getTile(const CanonicalTileID& tileID) final { - return impl.getTile(tileID.z, tileID.x, tileID.y); - } + Features getChildren(const std::uint32_t cluster_id) final { return impl.getChildren(cluster_id); } - mapbox::feature::feature_collection getChildren(const std::uint32_t cluster_id) final { - return impl.getChildren(cluster_id); - } - - mapbox::feature::feature_collection getLeaves(const std::uint32_t cluster_id, - const std::uint32_t limit, - const std::uint32_t offset) final { + Features getLeaves(const std::uint32_t cluster_id, const std::uint32_t limit, const std::uint32_t offset) final { return impl.getLeaves(cluster_id, limit, offset); } @@ -85,8 +80,7 @@ T evaluateFeature(const mapbox::feature::feature& f, // static std::shared_ptr GeoJSONData::create(const GeoJSON& geoJSON, Immutable options) { constexpr double scale = util::EXTENT / util::tileSize; - if (options->cluster && geoJSON.is>() && - !geoJSON.get>().empty()) { + if (options->cluster && geoJSON.is() && !geoJSON.get().empty()) { mapbox::supercluster::Options clusterOptions; clusterOptions.maxZoom = options->clusterMaxZoom; clusterOptions.extent = util::EXTENT; @@ -111,8 +105,7 @@ std::shared_ptr GeoJSONData::create(const GeoJSON& geoJSON, Immutab toReturn[p.first] = evaluateFeature(*feature, p.second.second, accumulated); } }; - return std::make_shared(geoJSON.get>(), - clusterOptions); + return std::make_shared(geoJSON.get(), clusterOptions); } mapbox::geojsonvt::Options vtOptions; diff --git a/src/mbgl/tile/geojson_tile.cpp b/src/mbgl/tile/geojson_tile.cpp index 0782f74d5..a4c48fb80 100644 --- a/src/mbgl/tile/geojson_tile.cpp +++ b/src/mbgl/tile/geojson_tile.cpp @@ -1,20 +1,29 @@ -#include -#include #include #include - +#include +#include +#include namespace mbgl { GeoJSONTile::GeoJSONTile(const OverscaledTileID& overscaledTileID, std::string sourceID_, const TileParameters& parameters, - mapbox::feature::feature_collection features) + std::shared_ptr data_) : GeometryTile(overscaledTileID, sourceID_, parameters) { - updateData(std::move(features)); + updateData(std::move(data_)); } -void GeoJSONTile::updateData(mapbox::feature::feature_collection features, bool resetLayers) { - setData(std::make_unique(std::move(features)), resetLayers); +void GeoJSONTile::updateData(std::shared_ptr data_, bool resetLayers) { + assert(data_); + data = std::move(data_); + data->getTile(id.canonical, + [this, self = weakFactory.makeWeakPtr(), capturedData = data.get(), resetLayers]( + style::GeoJSONData::TileFeatures features) { + if (!self) return; + if (data.get() != capturedData) return; + auto tileData = std::make_unique(std::move(features)); + setData(std::move(tileData), resetLayers); + }); } void GeoJSONTile::querySourceFeatures( diff --git a/src/mbgl/tile/geojson_tile.hpp b/src/mbgl/tile/geojson_tile.hpp index 9161e33f0..26dc300bd 100644 --- a/src/mbgl/tile/geojson_tile.hpp +++ b/src/mbgl/tile/geojson_tile.hpp @@ -4,6 +4,9 @@ #include namespace mbgl { +namespace style { +class GeoJSONData; +} // namespace style class TileParameters; @@ -12,13 +15,17 @@ public: GeoJSONTile(const OverscaledTileID&, std::string sourceID, const TileParameters&, - mapbox::feature::feature_collection); + std::shared_ptr); - void updateData(mapbox::feature::feature_collection, bool resetLayers = false); + void updateData(std::shared_ptr data, bool resetLayers = false); void querySourceFeatures( std::vector& result, const SourceQueryOptions&) override; + +private: + std::shared_ptr data; + mapbox::base::WeakPtrFactory weakFactory{this}; }; } // namespace mbgl diff --git a/test/tile/geojson_tile.test.cpp b/test/tile/geojson_tile.test.cpp index cc2dbfced..d4bf1e075 100644 --- a/test/tile/geojson_tile.test.cpp +++ b/test/tile/geojson_tile.test.cpp @@ -4,15 +4,16 @@ #include #include -#include +#include #include +#include #include -#include #include #include -#include -#include +#include +#include #include +#include #include @@ -43,6 +44,29 @@ public: }; }; +namespace { + +class FakeGeoJSONData : public GeoJSONData { +public: + FakeGeoJSONData(TileFeatures features_) : features(std::move(features_)) {} + + void getTile(const CanonicalTileID&, const std::function& fn) final { + assert(fn); + fn(features); + } + + Features getChildren(const std::uint32_t) final { return {}; } + + Features getLeaves(const std::uint32_t, const std::uint32_t, const std::uint32_t) final { return {}; } + + std::uint8_t getClusterExpansionZoom(std::uint32_t) final { return 0; } + +private: + TileFeatures features; +}; + +} // namespace + TEST(GeoJSONTile, Issue7648) { GeoJSONTileTest test; @@ -50,8 +74,8 @@ TEST(GeoJSONTile, Issue7648) { mapbox::feature::feature_collection features; features.push_back(mapbox::feature::feature { mapbox::geometry::point(0, 0) }); - - GeoJSONTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, features); + auto data = std::make_shared(std::move(features)); + GeoJSONTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, data); Immutable layerProperties = makeMutable(staticImmutableCast(layer.baseImpl)); StubTileObserver observer; observer.tileChanged = [&] (const Tile&) { @@ -68,7 +92,7 @@ TEST(GeoJSONTile, Issue7648) { test.loop.runOnce(); } - tile.updateData(features); + tile.updateData(data); while (!tile.isComplete()) { test.loop.runOnce(); } @@ -83,8 +107,8 @@ TEST(GeoJSONTile, Issue9927) { mapbox::feature::feature_collection features; features.push_back(mapbox::feature::feature { mapbox::geometry::point(0, 0) }); - - GeoJSONTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, features); + auto data = std::make_shared(std::move(features)); + GeoJSONTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, data); Immutable layerProperties = makeMutable(staticImmutableCast(layer.baseImpl)); std::vector> layers { layerProperties };