From ed9e605b1e3d136399c67768ab7dfdeafdd62ba5 Mon Sep 17 00:00:00 2001 From: Bruno de Oliveira Abinader Date: Thu, 21 Mar 2019 18:36:43 +0200 Subject: [core] Replace shared_ptr with unique_ptr in {Map,Resource}Options --- benchmark/api/render.benchmark.cpp | 43 +++++++++++++++++-------------- include/mbgl/map/map_options.hpp | 21 ++++++++------- include/mbgl/storage/resource_options.hpp | 25 ++++++++++-------- platform/android/src/file_source.cpp | 4 +-- platform/glfw/main.cpp | 8 +++--- platform/ios/vendor/mapbox-events-ios | 2 +- platform/node/src/node_map.cpp | 11 +++----- src/mbgl/map/map_options.cpp | 21 ++++++++------- src/mbgl/storage/resource_options.cpp | 29 +++++++++++---------- 9 files changed, 88 insertions(+), 76 deletions(-) diff --git a/benchmark/api/render.benchmark.cpp b/benchmark/api/render.benchmark.cpp index 0e7a3be322..b66c3f228c 100644 --- a/benchmark/api/render.benchmark.cpp +++ b/benchmark/api/render.benchmark.cpp @@ -18,6 +18,10 @@ using namespace mbgl; namespace { +static std::string cachePath { "benchmark/fixtures/api/cache.db" }; +constexpr double pixelRatio { 1.0 }; +constexpr Size size { 1000, 1000 }; + class RenderBenchmark { public: RenderBenchmark() { @@ -27,22 +31,23 @@ public: util::RunLoop loop; ThreadPool threadPool { 4 }; }; - + static void prepare(Map& map, optional json = {}) { map.getStyle().loadJSON(json ? *json : util::read_file("benchmark/fixtures/api/style.json")); map.jumpTo(CameraOptions().withCenter(LatLng { 40.726989, -73.992857 }).withZoom(15.0)); // Manhattan - map.getStyle().addImage(std::make_unique("test-icon", - decodeImage(util::read_file("benchmark/fixtures/api/default_marker.png")), 1.0)); + + auto image = decodeImage(util::read_file("benchmark/fixtures/api/default_marker.png")); + map.getStyle().addImage(std::make_unique("test-icon", std::move(image), 1.0)); } - + } // end namespace static void API_renderStill_reuse_map(::benchmark::State& state) { RenderBenchmark bench; - HeadlessFrontend frontend { { 1000, 1000 }, 1, bench.threadPool }; - Map map { frontend, MapObserver::nullObserver(), frontend.getSize(), 1, bench.threadPool, + HeadlessFrontend frontend { size, pixelRatio, bench.threadPool }; + Map map { frontend, MapObserver::nullObserver(), size, pixelRatio, bench.threadPool, MapOptions().withMapMode(MapMode::Static), - ResourceOptions().withCachePath("benchmark/fixtures/api/cache.db").withAssetPath(".").withAccessToken("foobar") }; + ResourceOptions().withCachePath(cachePath).withAccessToken("foobar") }; prepare(map); while (state.KeepRunning()) { @@ -52,10 +57,9 @@ static void API_renderStill_reuse_map(::benchmark::State& state) { static void API_renderStill_reuse_map_formatted_labels(::benchmark::State& state) { RenderBenchmark bench; - HeadlessFrontend frontend { { 1000, 1000 }, 1, bench.threadPool }; - Map map { frontend, MapObserver::nullObserver(), frontend.getSize(), 1, bench.threadPool, - MapOptions().withMapMode(MapMode::Static), - ResourceOptions().withCachePath("benchmark/fixtures/api/cache.db").withAssetPath(".").withAccessToken("foobar") }; + HeadlessFrontend frontend { size, pixelRatio, bench.threadPool }; + Map map { frontend, MapObserver::nullObserver(), size, pixelRatio, bench.threadPool, + MapOptions().withMapMode(MapMode::Static), ResourceOptions().withCachePath(cachePath).withAccessToken("foobar") }; prepare(map, util::read_file("benchmark/fixtures/api/style_formatted_labels.json")); while (state.KeepRunning()) { @@ -65,11 +69,10 @@ static void API_renderStill_reuse_map_formatted_labels(::benchmark::State& state static void API_renderStill_reuse_map_switch_styles(::benchmark::State& state) { RenderBenchmark bench; - HeadlessFrontend frontend { { 1000, 1000 }, 1, bench.threadPool }; - Map map { frontend, MapObserver::nullObserver(), frontend.getSize(), 1, bench.threadPool, - MapOptions().withMapMode(MapMode::Static), - ResourceOptions().withCachePath("benchmark/fixtures/api/cache.db").withAssetPath(".").withAccessToken("foobar") }; - + HeadlessFrontend frontend { size, pixelRatio, bench.threadPool }; + Map map { frontend, MapObserver::nullObserver(), size, pixelRatio, bench.threadPool, + MapOptions().withMapMode(MapMode::Static), ResourceOptions().withCachePath(cachePath).withAccessToken("foobar") }; + while (state.KeepRunning()) { prepare(map, { "{}" }); frontend.render(map); @@ -80,11 +83,11 @@ static void API_renderStill_reuse_map_switch_styles(::benchmark::State& state) { static void API_renderStill_recreate_map(::benchmark::State& state) { RenderBenchmark bench; - auto mapOptions = MapOptions().withMapMode(MapMode::Static); - auto resourceOptions = ResourceOptions().withCachePath("benchmark/fixtures/api/cache.db").withAssetPath(".").withAccessToken("foobar"); + while (state.KeepRunning()) { - HeadlessFrontend frontend { { 1000, 1000 }, 1, bench.threadPool }; - Map map { frontend, MapObserver::nullObserver(), frontend.getSize(), 1, bench.threadPool, mapOptions, resourceOptions }; + HeadlessFrontend frontend { size, pixelRatio, bench.threadPool }; + Map map { frontend, MapObserver::nullObserver(), size, pixelRatio, bench.threadPool, + MapOptions().withMapMode(MapMode::Static), ResourceOptions().withCachePath(cachePath).withAccessToken("foobar") }; prepare(map); frontend.render(map); } diff --git a/include/mbgl/map/map_options.hpp b/include/mbgl/map/map_options.hpp index f2297484a3..384b113dba 100644 --- a/include/mbgl/map/map_options.hpp +++ b/include/mbgl/map/map_options.hpp @@ -17,15 +17,18 @@ public: MapOptions(); ~MapOptions(); + MapOptions(MapOptions&&); + explicit MapOptions(const MapOptions&); + /** * @brief Sets the map rendering mode. By default, it is set to Continuous * so the map will render as data arrives from the network and react * immediately to state changes. * * @param mode Map rendering mode. - * @return reference to MapOptions for chaining options together. + * @return MapOptions for chaining options together. */ - MapOptions& withMapMode(MapMode mode); + MapOptions withMapMode(MapMode mode); /** * @brief Gets the previously set (or default) map mode. @@ -40,9 +43,9 @@ public: * HeightOnly. * * @param mode Map constrain mode. - * @return reference to MapOptions for chaining options together. + * @return MapOptions for chaining options together. */ - MapOptions& withConstrainMode(ConstrainMode mode); + MapOptions withConstrainMode(ConstrainMode mode); /** * @brief Gets the previously set (or default) constrain mode. @@ -56,9 +59,9 @@ public: * orientation of the map as some devices may use inverted orientation. * * @param mode Viewport mode. - * @return reference to MapOptions for chaining options together. + * @return MapOptions for chaining options together. */ - MapOptions& withViewportMode(ViewportMode mode); + MapOptions withViewportMode(ViewportMode mode); /** * @brief Gets the previously set (or default) viewport mode. @@ -72,9 +75,9 @@ public: * or not. By default, it is set to true. * * @param enableCollisions true to enable, false to disable - * @return reference to MapOptions for chaining options together. + * @return MapOptions for chaining options together. */ - MapOptions& withCrossSourceCollisions(bool enableCollisions); + MapOptions withCrossSourceCollisions(bool enableCollisions); /** * @brief Gets the previously set (or default) crossSourceCollisions value. @@ -86,7 +89,7 @@ public: private: class Impl; - std::shared_ptr impl_; + std::unique_ptr impl_; }; } // namespace mbgl diff --git a/include/mbgl/storage/resource_options.hpp b/include/mbgl/storage/resource_options.hpp index 0a4669ea15..958392175c 100644 --- a/include/mbgl/storage/resource_options.hpp +++ b/include/mbgl/storage/resource_options.hpp @@ -16,13 +16,16 @@ public: ResourceOptions(); ~ResourceOptions(); + ResourceOptions(ResourceOptions&&); + explicit ResourceOptions(const ResourceOptions&); + /** * @brief Sets the Mapbox access token - see https://docs.mapbox.com/help/how-mapbox-works/access-tokens/ for details. * * @param token Mapbox access token. - * @return reference to ResourceOptions for chaining options together. + * @return ResourceOptions for chaining options together. */ - ResourceOptions& withAccessToken(std::string token); + ResourceOptions withAccessToken(std::string token); /** * @brief Gets the previously set (or default) Mapbox access token. @@ -35,9 +38,9 @@ public: * @brief Sets the API base URL. Default is https://api.mapbox.com for Mapbox. * * @param baseURL API base URL. - * @return reference to ResourceOptions for chaining options together. + * @return ResourceOptions for chaining options together. */ - ResourceOptions& withBaseURL(std::string baseURL); + ResourceOptions withBaseURL(std::string baseURL); /** * @brief Gets the previously set (or default) API base URL. @@ -50,9 +53,9 @@ public: * @brief Sets the cache path. * * @param path Cache path. - * @return reference to ResourceOptions for chaining options together. + * @return ResourceOptions for chaining options together. */ - ResourceOptions& withCachePath(std::string path); + ResourceOptions withCachePath(std::string path); /** * @brief Gets the previously set (or default) cache path. @@ -66,9 +69,9 @@ public: * the asset:// scheme gets resolved in a style. * * @param path Asset path. - * @return reference to ResourceOptions for chaining options together. + * @return ResourceOptions for chaining options together. */ - ResourceOptions& withAssetPath(std::string path); + ResourceOptions withAssetPath(std::string path); /** * @brief Gets the previously set (or default) asset path. @@ -83,7 +86,7 @@ public: * @param size Cache maximum size in bytes. * @return reference to ResourceOptions for chaining options together. */ - ResourceOptions& withMaximumCacheSize(uint64_t size); + ResourceOptions withMaximumCacheSize(uint64_t size); /** * @brief Gets the previously set (or default) maximum allowed cache size. @@ -99,7 +102,7 @@ public: * @param context Platform context. * @return reference to ResourceOptions for chaining options together. */ - ResourceOptions& withPlatformContext(void* context); + ResourceOptions withPlatformContext(void* context); /** * @brief Gets the previously set (or default) platform context. @@ -110,7 +113,7 @@ public: private: class Impl; - std::shared_ptr impl_; + std::unique_ptr impl_; }; } // namespace mbgl diff --git a/platform/android/src/file_source.cpp b/platform/android/src/file_source.cpp index bb1f30981b..94db563efc 100644 --- a/platform/android/src/file_source.cpp +++ b/platform/android/src/file_source.cpp @@ -31,7 +31,7 @@ FileSource::FileSource(jni::JNIEnv& _env, std::string path = jni::Make(_env, _cachePath); mapbox::sqlite::setTempPath(path); - resourceOptions = mbgl::ResourceOptions() + resourceOptions .withAccessToken(accessToken ? jni::Make(_env, accessToken) : "") .withCachePath(path + DATABASE_FILE) .withPlatformContext(reinterpret_cast(new AssetManagerFileSource(_env, assetManager))); @@ -120,7 +120,7 @@ FileSource* FileSource::getNativePeer(jni::JNIEnv& env, const jni::Object& jFileSource) { FileSource* fileSource = FileSource::getNativePeer(env, jFileSource); assert(fileSource != nullptr); - return fileSource->resourceOptions; + return mbgl::ResourceOptions(fileSource->resourceOptions); } void FileSource::registerNative(jni::JNIEnv& env) { diff --git a/platform/glfw/main.cpp b/platform/glfw/main.cpp index 8288a009f2..77585718e1 100644 --- a/platform/glfw/main.cpp +++ b/platform/glfw/main.cpp @@ -94,12 +94,14 @@ int main(int argc, char *argv[]) { view = &backend; // Set access token if present - const char *token = getenv("MAPBOX_ACCESS_TOKEN"); - if (token == nullptr) { + std::string token(getenv("MAPBOX_ACCESS_TOKEN") ?: ""); + if (token.empty()) { mbgl::Log::Warning(mbgl::Event::Setup, "no access token set. mapbox.com tiles won't work."); } - auto resourceOptions = mbgl::ResourceOptions().withCachePath(cacheDB).withAssetPath(".").withAccessToken(std::string(token)); + mbgl::ResourceOptions resourceOptions; + resourceOptions.withCachePath(cacheDB).withAccessToken(token); + auto fileSource = std::static_pointer_cast(mbgl::FileSource::getSharedFileSource(resourceOptions)); if (!settings.online) { fileSource->setOnlineStatus(false); diff --git a/platform/ios/vendor/mapbox-events-ios b/platform/ios/vendor/mapbox-events-ios index a75545ca01..d79e62581d 160000 --- a/platform/ios/vendor/mapbox-events-ios +++ b/platform/ios/vendor/mapbox-events-ios @@ -1 +1 @@ -Subproject commit a75545ca01b3e567a989cd950b74f7865d3518ce +Subproject commit d79e62581df5f51a0064dd2f78972c489c71d418 diff --git a/platform/node/src/node_map.cpp b/platform/node/src/node_map.cpp index 0b27d378a7..7ad9c362bf 100644 --- a/platform/node/src/node_map.cpp +++ b/platform/node/src/node_map.cpp @@ -631,14 +631,9 @@ void NodeMap::cancel() { }); frontend = std::make_unique(mbgl::Size{ 256, 256 }, pixelRatio, threadpool); - auto mapOptions = mbgl::MapOptions() - .withMapMode(mode) - .withCrossSourceCollisions(crossSourceCollisions); - - auto resourceOptions = mbgl::ResourceOptions().withPlatformContext(reinterpret_cast(this)); - - map = std::make_unique(*frontend, mapObserver, frontend->getSize(), pixelRatio, - threadpool, mapOptions, resourceOptions); + map = std::make_unique(*frontend, mapObserver, frontend->getSize(), pixelRatio, threadpool, + mbgl::MapOptions().withMapMode(mode).withCrossSourceCollisions(crossSourceCollisions), + mbgl::ResourceOptions().withPlatformContext(reinterpret_cast(this))); // FIXME: Reload the style after recreating the map. We need to find // a better way of canceling an ongoing rendering on the core level diff --git a/src/mbgl/map/map_options.cpp b/src/mbgl/map/map_options.cpp index ddb3f8e3be..c1146ab0bc 100644 --- a/src/mbgl/map/map_options.cpp +++ b/src/mbgl/map/map_options.cpp @@ -10,39 +10,42 @@ public: bool crossSourceCollisions = true; }; -MapOptions::MapOptions() : impl_(std::make_shared()) {} +// These requires the complete type of Impl. +MapOptions::MapOptions() : impl_(std::make_unique()) {} MapOptions::~MapOptions() = default; +MapOptions::MapOptions(MapOptions&&) = default; +MapOptions::MapOptions(const MapOptions& other) : impl_(std::make_unique(*other.impl_)) {} -MapOptions& MapOptions::withMapMode(MapMode mode) { +MapOptions MapOptions::withMapMode(MapMode mode) { impl_->mapMode = mode; - return *this; + return std::move(*this); } MapMode MapOptions::mapMode() const { return impl_->mapMode; } -MapOptions& MapOptions::withConstrainMode(ConstrainMode mode) { +MapOptions MapOptions::withConstrainMode(ConstrainMode mode) { impl_->constrainMode = mode; - return *this; + return std::move(*this); } ConstrainMode MapOptions::constrainMode() const { return impl_->constrainMode; } -MapOptions& MapOptions::withViewportMode(ViewportMode mode) { +MapOptions MapOptions::withViewportMode(ViewportMode mode) { impl_->viewportMode = mode; - return *this; + return std::move(*this); } ViewportMode MapOptions::viewportMode() const { return impl_->viewportMode; } -MapOptions& MapOptions::withCrossSourceCollisions(bool enableCollisions) { +MapOptions MapOptions::withCrossSourceCollisions(bool enableCollisions) { impl_->crossSourceCollisions = enableCollisions; - return *this; + return std::move(*this); } bool MapOptions::crossSourceCollisions() const { diff --git a/src/mbgl/storage/resource_options.cpp b/src/mbgl/storage/resource_options.cpp index ce82b51a14..dcd7f84f7e 100644 --- a/src/mbgl/storage/resource_options.cpp +++ b/src/mbgl/storage/resource_options.cpp @@ -13,57 +13,60 @@ public: void* platformContext = nullptr; }; -ResourceOptions::ResourceOptions() : impl_(std::make_shared()) {} +// These requires the complete type of Impl. +ResourceOptions::ResourceOptions() : impl_(std::make_unique()) {} ResourceOptions::~ResourceOptions() = default; +ResourceOptions::ResourceOptions(ResourceOptions&&) = default; +ResourceOptions::ResourceOptions(const ResourceOptions& other) : impl_(std::make_unique(*other.impl_)) {} -ResourceOptions& ResourceOptions::withAccessToken(std::string token) { +ResourceOptions ResourceOptions::withAccessToken(std::string token) { impl_->accessToken = std::move(token); - return *this; + return std::move(*this); } const std::string& ResourceOptions::accessToken() const { return impl_->accessToken; } -ResourceOptions& ResourceOptions::withBaseURL(std::string url) { +ResourceOptions ResourceOptions::withBaseURL(std::string url) { impl_->baseURL = std::move(url); - return *this; + return std::move(*this); } const std::string& ResourceOptions::baseURL() const { return impl_->baseURL; } -ResourceOptions& ResourceOptions::withCachePath(std::string path) { +ResourceOptions ResourceOptions::withCachePath(std::string path) { impl_->cachePath = std::move(path); - return *this; + return std::move(*this); } const std::string& ResourceOptions::cachePath() const { return impl_->cachePath; } -ResourceOptions& ResourceOptions::withAssetPath(std::string path) { +ResourceOptions ResourceOptions::withAssetPath(std::string path) { impl_->assetPath = std::move(path); - return *this; + return std::move(*this); } const std::string& ResourceOptions::assetPath() const { return impl_->assetPath; } -ResourceOptions& ResourceOptions::withMaximumCacheSize(uint64_t size) { +ResourceOptions ResourceOptions::withMaximumCacheSize(uint64_t size) { impl_->maximumSize = size; - return *this; + return std::move(*this); } uint64_t ResourceOptions::maximumCacheSize() const { return impl_->maximumSize; } -ResourceOptions& ResourceOptions::withPlatformContext(void* context) { +ResourceOptions ResourceOptions::withPlatformContext(void* context) { impl_->platformContext = context; - return *this; + return std::move(*this); } void* ResourceOptions::platformContext() const { -- cgit v1.2.1