From d47159b1f8cdc10a5ca4248963b38f18c5b8fd47 Mon Sep 17 00:00:00 2001 From: mwilsnd Date: Thu, 7 Mar 2024 13:03:09 -0500 Subject: [PATCH 1/5] [Android] Couple a sequenced scheduler to the lifetime of a GeoJSON source --- include/mbgl/style/sources/geojson_source.hpp | 7 +++---- .../src/cpp/style/sources/geojson_source.cpp | 4 ++-- .../src/cpp/style/sources/geojson_source.hpp | 3 ++- src/mbgl/style/sources/geojson_source.cpp | 21 +++++-------------- .../style/sources/geojson_source_impl.cpp | 7 ++----- 5 files changed, 14 insertions(+), 28 deletions(-) diff --git a/include/mbgl/style/sources/geojson_source.hpp b/include/mbgl/style/sources/geojson_source.hpp index 1759c4fb242..25e0e068b24 100644 --- a/include/mbgl/style/sources/geojson_source.hpp +++ b/include/mbgl/style/sources/geojson_source.hpp @@ -41,8 +41,8 @@ class GeoJSONData { using TileFeatures = mapbox::feature::feature_collection; using Features = mapbox::feature::feature_collection; static std::shared_ptr create(const GeoJSON&, - const Immutable& = GeoJSONOptions::defaultOptions(), - std::shared_ptr scheduler = nullptr); + std::shared_ptr scheduler, + const Immutable& = GeoJSONOptions::defaultOptions()); virtual ~GeoJSONData() = default; virtual void getTile(const CanonicalTileID&, const std::function&) = 0; @@ -51,8 +51,6 @@ class GeoJSONData { virtual Features getChildren(std::uint32_t) = 0; virtual Features getLeaves(std::uint32_t, std::uint32_t limit, std::uint32_t offset) = 0; virtual std::uint8_t getClusterExpansionZoom(std::uint32_t) = 0; - - virtual std::shared_ptr getScheduler() { return nullptr; } }; class GeoJSONSource final : public Source { @@ -83,6 +81,7 @@ class GeoJSONSource final : public Source { std::optional url; std::unique_ptr req; std::shared_ptr threadPool; + std::shared_ptr sequencedScheduler; mapbox::base::WeakPtrFactory weakFactory{this}; }; diff --git a/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.cpp b/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.cpp index 0a27b3b8535..1ce0d8ad0e1 100644 --- a/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.cpp +++ b/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.cpp @@ -245,7 +245,7 @@ void FeatureConverter::convertJson(std::shared_ptr json, ActorRef @@ -257,7 +257,7 @@ void FeatureConverter::convertObject( android::UniqueEnv _env = android::AttachEnv(); // Convert the jni object auto geometry = JNIType::convert(*_env, *jObject); - callback.invoke(&GeoJSONDataCallback::operator(), style::GeoJSONData::create(geometry, options)); + callback.invoke(&GeoJSONDataCallback::operator(), style::GeoJSONData::create(geometry, sequencedScheduler, options)); } Update::Update(Converter _converterFn, std::unique_ptr> _callback) diff --git a/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.hpp b/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.hpp index e0453ecdc68..1003dbedf17 100644 --- a/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.hpp +++ b/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.hpp @@ -16,7 +16,7 @@ using GeoJSONDataCallback = std::function options_) - : options(std::move(options_)) {} + : options(std::move(options_)), sequencedScheduler(Scheduler::GetSequenced()) {} void convertJson(std::shared_ptr, ActorRef); template @@ -25,6 +25,7 @@ class FeatureConverter { private: Immutable options; + std::shared_ptr sequencedScheduler; }; struct Update { diff --git a/src/mbgl/style/sources/geojson_source.cpp b/src/mbgl/style/sources/geojson_source.cpp index 11d323a0be2..1c47afae863 100644 --- a/src/mbgl/style/sources/geojson_source.cpp +++ b/src/mbgl/style/sources/geojson_source.cpp @@ -21,7 +21,8 @@ Immutable GeoJSONOptions::defaultOptions() { GeoJSONSource::GeoJSONSource(std::string id, Immutable options) : Source(makeMutable(std::move(id), std::move(options))), - threadPool(Scheduler::GetBackground()) {} + threadPool(Scheduler::GetBackground()), + sequencedScheduler(Scheduler::GetSequenced()) {} GeoJSONSource::~GeoJSONSource() = default; @@ -40,20 +41,8 @@ void GeoJSONSource::setURL(const std::string& url_) { } } -namespace { - -inline std::shared_ptr createGeoJSONData(const mapbox::geojson::geojson& geoJSON, - const GeoJSONSource::Impl& impl) { - if (auto data = impl.getData().lock()) { - return GeoJSONData::create(geoJSON, impl.getOptions(), data->getScheduler()); - } - return GeoJSONData::create(geoJSON, impl.getOptions()); -} - -} // namespace - void GeoJSONSource::setGeoJSON(const mapbox::geojson::geojson& geoJSON) { - setGeoJSONData(createGeoJSONData(geoJSON, impl())); + setGeoJSONData(GeoJSONData::create(geoJSON, sequencedScheduler, impl().getOptions())); } void GeoJSONSource::setGeoJSONData(std::shared_ptr geoJSONData) { @@ -88,13 +77,13 @@ void GeoJSONSource::loadDescription(FileSource& fileSource) { } else if (res.noContent) { observer->onSourceError(*this, std::make_exception_ptr(std::runtime_error("unexpectedly empty GeoJSON"))); } else { - auto makeImplInBackground = [currentImpl = baseImpl, data = res.data]() -> Immutable { + auto makeImplInBackground = [currentImpl = baseImpl, data = res.data, seqScheduler{sequencedScheduler}]() -> Immutable { assert(data); auto& current = static_cast(*currentImpl); conversion::Error error; std::shared_ptr geoJSONData; if (std::optional geoJSON = conversion::convertJSON(*data, error)) { - geoJSONData = createGeoJSONData(*geoJSON, current); + geoJSONData = GeoJSONData::create(*geoJSON, std::move(seqScheduler), current.getOptions()); } else { // Create an empty GeoJSON VT object to make sure we're not // infinitely waiting for tiles to load. diff --git a/src/mbgl/style/sources/geojson_source_impl.cpp b/src/mbgl/style/sources/geojson_source_impl.cpp index 2d733d5f6ba..84872cbfe0d 100644 --- a/src/mbgl/style/sources/geojson_source_impl.cpp +++ b/src/mbgl/style/sources/geojson_source_impl.cpp @@ -38,8 +38,6 @@ class GeoJSONVTData final : public GeoJSONData { std::uint8_t getClusterExpansionZoom(std::uint32_t) final { return 0; } - std::shared_ptr getScheduler() final { return scheduler; } - friend GeoJSONData; GeoJSONVTData(const GeoJSON& geoJSON, const mapbox::geojsonvt::Options& options, @@ -91,8 +89,8 @@ T evaluateFeature(const mapbox::feature::feature& f, // static std::shared_ptr GeoJSONData::create(const GeoJSON& geoJSON, - const Immutable& options, - std::shared_ptr scheduler) { + std::shared_ptr scheduler, + const Immutable& options) { constexpr double scale = util::EXTENT / util::tileSize_D; if (options->cluster && geoJSON.is() && !geoJSON.get().empty()) { mapbox::supercluster::Options clusterOptions; @@ -128,7 +126,6 @@ std::shared_ptr GeoJSONData::create(const GeoJSON& geoJSON, vtOptions.buffer = static_cast(::round(scale * options->buffer)); vtOptions.tolerance = scale * options->tolerance; vtOptions.lineMetrics = options->lineMetrics; - if (!scheduler) scheduler = Scheduler::GetSequenced(); return std::shared_ptr(new GeoJSONVTData(geoJSON, vtOptions, std::move(scheduler))); } From 08affec55725edddb39c27eecf5df9f44d0f6460 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 7 Mar 2024 18:45:55 +0000 Subject: [PATCH 2/5] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- .../src/cpp/style/sources/geojson_source.cpp | 6 ++++-- .../src/cpp/style/sources/geojson_source.hpp | 3 ++- src/mbgl/style/sources/geojson_source.cpp | 4 +++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.cpp b/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.cpp index 1ce0d8ad0e1..bc37a5f5c5f 100644 --- a/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.cpp +++ b/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.cpp @@ -245,7 +245,8 @@ void FeatureConverter::convertJson(std::shared_ptr json, ActorRef @@ -257,7 +258,8 @@ void FeatureConverter::convertObject( android::UniqueEnv _env = android::AttachEnv(); // Convert the jni object auto geometry = JNIType::convert(*_env, *jObject); - callback.invoke(&GeoJSONDataCallback::operator(), style::GeoJSONData::create(geometry, sequencedScheduler, options)); + callback.invoke(&GeoJSONDataCallback::operator(), + style::GeoJSONData::create(geometry, sequencedScheduler, options)); } Update::Update(Converter _converterFn, std::unique_ptr> _callback) diff --git a/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.hpp b/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.hpp index 1003dbedf17..6a360484b53 100644 --- a/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.hpp +++ b/platform/android/MapboxGLAndroidSDK/src/cpp/style/sources/geojson_source.hpp @@ -16,7 +16,8 @@ using GeoJSONDataCallback = std::function options_) - : options(std::move(options_)), sequencedScheduler(Scheduler::GetSequenced()) {} + : options(std::move(options_)), + sequencedScheduler(Scheduler::GetSequenced()) {} void convertJson(std::shared_ptr, ActorRef); template diff --git a/src/mbgl/style/sources/geojson_source.cpp b/src/mbgl/style/sources/geojson_source.cpp index 1c47afae863..a00477d3acf 100644 --- a/src/mbgl/style/sources/geojson_source.cpp +++ b/src/mbgl/style/sources/geojson_source.cpp @@ -77,7 +77,9 @@ void GeoJSONSource::loadDescription(FileSource& fileSource) { } else if (res.noContent) { observer->onSourceError(*this, std::make_exception_ptr(std::runtime_error("unexpectedly empty GeoJSON"))); } else { - auto makeImplInBackground = [currentImpl = baseImpl, data = res.data, seqScheduler{sequencedScheduler}]() -> Immutable { + auto makeImplInBackground = [currentImpl = baseImpl, + data = res.data, + seqScheduler{sequencedScheduler}]() -> Immutable { assert(data); auto& current = static_cast(*currentImpl); conversion::Error error; From 085c207a73eaa6572f6f2ae4ceb5f2aaf882d364 Mon Sep 17 00:00:00 2001 From: mwilsnd Date: Thu, 7 Mar 2024 13:57:32 -0500 Subject: [PATCH 3/5] Fix test --- include/mbgl/style/sources/geojson_source.hpp | 2 ++ test/style/source.test.cpp | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/include/mbgl/style/sources/geojson_source.hpp b/include/mbgl/style/sources/geojson_source.hpp index 25e0e068b24..5ba372dc6d7 100644 --- a/include/mbgl/style/sources/geojson_source.hpp +++ b/include/mbgl/style/sources/geojson_source.hpp @@ -74,6 +74,8 @@ class GeoJSONSource final : public Source { mapbox::base::WeakPtr makeWeakPtr() override { return weakFactory.makeWeakPtr(); } + const std::shared_ptr& getSequencedScheduler() noexcept { return sequencedScheduler; } + protected: Mutable createMutable() const noexcept final; diff --git a/test/style/source.test.cpp b/test/style/source.test.cpp index 0a2a52b5afd..46208d9e010 100644 --- a/test/style/source.test.cpp +++ b/test/style/source.test.cpp @@ -970,7 +970,7 @@ TEST(Source, GeoJSONSourceTilesAfterDataReset) { SourceTest test; GeoJSONSource source("source"); auto geoJSONData = GeoJSONData::create(mapbox::geojson::parse( - R"({"geometry": {"type": "Point", "coordinates": [1.1, 1.1]}, "type": "Feature", "properties": {}})")); + R"({"geometry": {"type": "Point", "coordinates": [1.1, 1.1]}, "type": "Feature", "properties": {}})"), source.getSequencedScheduler()); source.setGeoJSONData(geoJSONData); RenderGeoJSONSource renderSource{staticImmutableCast(source.baseImpl), test.threadPool}; From 4bca27e58b4bac054d9c5a71a71cb0c97720b5d3 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 7 Mar 2024 18:58:01 +0000 Subject: [PATCH 4/5] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- test/style/source.test.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/style/source.test.cpp b/test/style/source.test.cpp index 46208d9e010..fdec2212c73 100644 --- a/test/style/source.test.cpp +++ b/test/style/source.test.cpp @@ -969,8 +969,10 @@ TEST(Source, VectorSourceUrlSetTiles) { TEST(Source, GeoJSONSourceTilesAfterDataReset) { SourceTest test; GeoJSONSource source("source"); - auto geoJSONData = GeoJSONData::create(mapbox::geojson::parse( - R"({"geometry": {"type": "Point", "coordinates": [1.1, 1.1]}, "type": "Feature", "properties": {}})"), source.getSequencedScheduler()); + auto geoJSONData = GeoJSONData::create( + mapbox::geojson::parse( + R"({"geometry": {"type": "Point", "coordinates": [1.1, 1.1]}, "type": "Feature", "properties": {}})"), + source.getSequencedScheduler()); source.setGeoJSONData(geoJSONData); RenderGeoJSONSource renderSource{staticImmutableCast(source.baseImpl), test.threadPool}; From e10a0b8e3f8c1f068c7f0441e1afdb5bcec040c3 Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Mon, 11 Mar 2024 16:29:21 +0100 Subject: [PATCH 5/5] Remove getSewuencedScheduler() method --- include/mbgl/style/sources/geojson_source.hpp | 2 -- test/style/source.test.cpp | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/include/mbgl/style/sources/geojson_source.hpp b/include/mbgl/style/sources/geojson_source.hpp index 5ba372dc6d7..25e0e068b24 100644 --- a/include/mbgl/style/sources/geojson_source.hpp +++ b/include/mbgl/style/sources/geojson_source.hpp @@ -74,8 +74,6 @@ class GeoJSONSource final : public Source { mapbox::base::WeakPtr makeWeakPtr() override { return weakFactory.makeWeakPtr(); } - const std::shared_ptr& getSequencedScheduler() noexcept { return sequencedScheduler; } - protected: Mutable createMutable() const noexcept final; diff --git a/test/style/source.test.cpp b/test/style/source.test.cpp index fdec2212c73..55655d0fda3 100644 --- a/test/style/source.test.cpp +++ b/test/style/source.test.cpp @@ -972,7 +972,7 @@ TEST(Source, GeoJSONSourceTilesAfterDataReset) { auto geoJSONData = GeoJSONData::create( mapbox::geojson::parse( R"({"geometry": {"type": "Point", "coordinates": [1.1, 1.1]}, "type": "Feature", "properties": {}})"), - source.getSequencedScheduler()); + Scheduler::GetSequenced()); source.setGeoJSONData(geoJSONData); RenderGeoJSONSource renderSource{staticImmutableCast(source.baseImpl), test.threadPool};