From 8c743d2467c67eecdd0a74d6bad3e0f0b980907b Mon Sep 17 00:00:00 2001 From: Bart Louwers Date: Tue, 11 Jun 2024 16:45:09 +0200 Subject: [PATCH] Use `std::variant` with `OfflineRegionDefinition` (#2482) --- include/mbgl/storage/offline.hpp | 4 +- include/mbgl/util/variant.hpp | 9 +++ .../src/cpp/offline/offline_region.cpp | 17 ++-- platform/darwin/src/MLNOfflinePack.mm | 18 ++--- platform/default/src/mbgl/storage/offline.cpp | 52 ++++++------ .../src/mbgl/storage/offline_download.cpp | 34 ++++---- test/storage/offline.test.cpp | 4 +- test/storage/offline_database.test.cpp | 81 +++++++++++-------- 8 files changed, 127 insertions(+), 92 deletions(-) diff --git a/include/mbgl/storage/offline.hpp b/include/mbgl/storage/offline.hpp index 5f0d9fccd..b136ee7fe 100644 --- a/include/mbgl/storage/offline.hpp +++ b/include/mbgl/storage/offline.hpp @@ -3,13 +3,13 @@ #include #include #include -#include #include #include #include #include #include +#include namespace mbgl { @@ -71,7 +71,7 @@ public: /* * The offline region definition types supported */ -using OfflineRegionDefinition = variant; +using OfflineRegionDefinition = std::variant; /* * The encoded format is private. diff --git a/include/mbgl/util/variant.hpp b/include/mbgl/util/variant.hpp index 82990b71d..daa5d9d48 100644 --- a/include/mbgl/util/variant.hpp +++ b/include/mbgl/util/variant.hpp @@ -16,4 +16,13 @@ namespace mbgl { template using variant = mapbox::util::variant; +// https://en.cppreference.com/w/cpp/utility/variant/visit +template +struct overloaded : Ts... { + using Ts::operator()...; +}; +// explicit deduction guide (not needed as of C++20) +template +overloaded(Ts...) -> overloaded; + } // namespace mbgl diff --git a/platform/android/MapLibreAndroid/src/cpp/offline/offline_region.cpp b/platform/android/MapLibreAndroid/src/cpp/offline/offline_region.cpp index c9f631289..20fa12ee8 100644 --- a/platform/android/MapLibreAndroid/src/cpp/offline/offline_region.cpp +++ b/platform/android/MapLibreAndroid/src/cpp/offline/offline_region.cpp @@ -1,8 +1,11 @@ #include "offline_region.hpp" +#include + #include #include #include +#include #include "offline_region_definition.hpp" #include "offline_region_error.hpp" @@ -180,13 +183,13 @@ jni::Local> OfflineRegion::New(jni::JNIEnv& env, const jni::Object& jFileSource, mbgl::OfflineRegion region) { // Definition - auto definition = region.getDefinition().match( - [&](const mbgl::OfflineTilePyramidRegionDefinition def) { - return OfflineTilePyramidRegionDefinition::New(env, def); - }, - [&](const mbgl::OfflineGeometryRegionDefinition def) { - return OfflineGeometryRegionDefinition::New(env, def); - }); + auto definition = std::visit(overloaded{[&](const mbgl::OfflineTilePyramidRegionDefinition def) { + return OfflineTilePyramidRegionDefinition::New(env, def); + }, + [&](const mbgl::OfflineGeometryRegionDefinition def) { + return OfflineGeometryRegionDefinition::New(env, def); + }}, + region.getDefinition()); // Create region java object static auto& javaClass = jni::Class::Singleton(env); diff --git a/platform/darwin/src/MLNOfflinePack.mm b/platform/darwin/src/MLNOfflinePack.mm index b68f831f4..0e94d60c4 100644 --- a/platform/darwin/src/MLNOfflinePack.mm +++ b/platform/darwin/src/MLNOfflinePack.mm @@ -12,6 +12,7 @@ #include #include +#include const MLNExceptionName MLNInvalidOfflinePackException = @"MLNInvalidOfflinePackException"; @@ -93,15 +94,14 @@ private: MLNAssert([MLNTilePyramidOfflineRegion conformsToProtocol:@protocol(MLNOfflineRegion_Private)], @"MLNTilePyramidOfflineRegion should conform to MLNOfflineRegion_Private."); MLNAssert([MLNShapeOfflineRegion conformsToProtocol:@protocol(MLNOfflineRegion_Private)], @"MLNShapeOfflineRegion should conform to MLNOfflineRegion_Private."); - - - return regionDefinition.match( - [&] (const mbgl::OfflineTilePyramidRegionDefinition def){ - return (id )[[MLNTilePyramidOfflineRegion alloc] initWithOfflineRegionDefinition:def]; - }, - [&] (const mbgl::OfflineGeometryRegionDefinition& def){ - return (id )[[MLNShapeOfflineRegion alloc] initWithOfflineRegionDefinition:def]; - }); + return std::visit(mbgl::overloaded{ + [&] (const mbgl::OfflineTilePyramidRegionDefinition def){ + return (id )[[MLNTilePyramidOfflineRegion alloc] initWithOfflineRegionDefinition:def]; + }, + [&] (const mbgl::OfflineGeometryRegionDefinition& def){ + return (id )[[MLNShapeOfflineRegion alloc] initWithOfflineRegionDefinition:def]; + } + }, regionDefinition); } - (NSData *)context { diff --git a/platform/default/src/mbgl/storage/offline.cpp b/platform/default/src/mbgl/storage/offline.cpp index c700f47ed..f590c7db6 100644 --- a/platform/default/src/mbgl/storage/offline.cpp +++ b/platform/default/src/mbgl/storage/offline.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include @@ -110,32 +111,37 @@ std::string encodeOfflineRegionDefinition(const OfflineRegionDefinition& region) doc.SetObject(); // Encode common properties - region.match([&](auto& _region) { - doc.AddMember( - "style_url", rapidjson::StringRef(_region.styleURL.data(), _region.styleURL.length()), doc.GetAllocator()); - doc.AddMember("min_zoom", _region.minZoom, doc.GetAllocator()); - if (std::isfinite(_region.maxZoom)) { - doc.AddMember("max_zoom", _region.maxZoom, doc.GetAllocator()); - } + std::visit( + [&](auto& _region) { + doc.AddMember("style_url", + rapidjson::StringRef(_region.styleURL.data(), _region.styleURL.length()), + doc.GetAllocator()); + doc.AddMember("min_zoom", _region.minZoom, doc.GetAllocator()); + if (std::isfinite(_region.maxZoom)) { + doc.AddMember("max_zoom", _region.maxZoom, doc.GetAllocator()); + } - doc.AddMember("pixel_ratio", _region.pixelRatio, doc.GetAllocator()); - doc.AddMember("include_ideographs", _region.includeIdeographs, doc.GetAllocator()); - }); + doc.AddMember("pixel_ratio", _region.pixelRatio, doc.GetAllocator()); + doc.AddMember("include_ideographs", _region.includeIdeographs, doc.GetAllocator()); + }, + region); // Encode specific properties - region.match( - [&](const OfflineTilePyramidRegionDefinition& _region) { - rapidjson::GenericValue, rapidjson::CrtAllocator> bounds(rapidjson::kArrayType); - bounds.PushBack(_region.bounds.south(), doc.GetAllocator()); - bounds.PushBack(_region.bounds.west(), doc.GetAllocator()); - bounds.PushBack(_region.bounds.north(), doc.GetAllocator()); - bounds.PushBack(_region.bounds.east(), doc.GetAllocator()); - doc.AddMember("bounds", bounds, doc.GetAllocator()); - }, - [&](const OfflineGeometryRegionDefinition& _region) { - doc.AddMember( - "geometry", mapbox::geojson::convert(_region.geometry, doc.GetAllocator()), doc.GetAllocator()); - }); + std::visit(overloaded{[&](const OfflineTilePyramidRegionDefinition& _region) { + rapidjson::GenericValue, rapidjson::CrtAllocator> bounds( + rapidjson::kArrayType); + bounds.PushBack(_region.bounds.south(), doc.GetAllocator()); + bounds.PushBack(_region.bounds.west(), doc.GetAllocator()); + bounds.PushBack(_region.bounds.north(), doc.GetAllocator()); + bounds.PushBack(_region.bounds.east(), doc.GetAllocator()); + doc.AddMember("bounds", bounds, doc.GetAllocator()); + }, + [&](const OfflineGeometryRegionDefinition& _region) { + doc.AddMember("geometry", + mapbox::geojson::convert(_region.geometry, doc.GetAllocator()), + doc.GetAllocator()); + }}, + region); rapidjson::StringBuffer buffer; rapidjson::Writer writer(buffer); diff --git a/platform/default/src/mbgl/storage/offline_download.cpp b/platform/default/src/mbgl/storage/offline_download.cpp index 105535cef..dcd4deef3 100644 --- a/platform/default/src/mbgl/storage/offline_download.cpp +++ b/platform/default/src/mbgl/storage/offline_download.cpp @@ -63,12 +63,15 @@ void tileCover(const OfflineRegionDefinition& definition, uint16_t tileSize, const Range& zoomRange, Fn&& fn) { - const Range clampedZoomRange = definition.match( - [&](auto& reg) { return coveringZoomRange(reg, type, tileSize, zoomRange); }); + const Range clampedZoomRange = std::visit( + [&](auto& reg) { return coveringZoomRange(reg, type, tileSize, zoomRange); }, definition); for (uint8_t z = clampedZoomRange.min; z <= clampedZoomRange.max; z++) { - definition.match([&](const OfflineTilePyramidRegionDefinition& reg) { tileCover(reg.bounds, z, fn); }, - [&](const OfflineGeometryRegionDefinition& reg) { tileCover(reg.geometry, z, fn); }); + std::visit(overloaded{[&](const OfflineTilePyramidRegionDefinition& reg) { tileCover(reg.bounds, z, fn); }, + [&](const OfflineGeometryRegionDefinition& reg) { + tileCover(reg.geometry, z, fn); + }}, + definition); } } @@ -76,14 +79,17 @@ uint64_t tileCount(const OfflineRegionDefinition& definition, style::SourceType type, uint16_t tileSize, const Range& zoomRange) { - const Range clampedZoomRange = definition.match( - [&](auto& reg) { return coveringZoomRange(reg, type, tileSize, zoomRange); }); + const Range clampedZoomRange = std::visit( + [&](auto& reg) { return coveringZoomRange(reg, type, tileSize, zoomRange); }, definition); uint64_t result{}; for (uint8_t z = clampedZoomRange.min; z <= clampedZoomRange.max; z++) { - result += definition.match( - [&](const OfflineTilePyramidRegionDefinition& reg) { return util::tileCount(reg.bounds, z); }, - [&](const OfflineGeometryRegionDefinition& reg) { return util::tileCount(reg.geometry, z); }); + result += std::visit( + overloaded{[&](const OfflineTilePyramidRegionDefinition& reg) { return util::tileCount(reg.bounds, z); }, + [&](const OfflineGeometryRegionDefinition& reg) { + return util::tileCount(reg.geometry, z); + }}, + definition); } return result; @@ -138,7 +144,7 @@ OfflineRegionStatus OfflineDownload::getStatus() const { result->requiredResourceCount++; std::optional styleResponse = offlineDatabase.get( - Resource::style(definition.match([](auto& reg) { return reg.styleURL; }))); + Resource::style(std::visit([](auto& reg) { return reg.styleURL; }, definition))); if (!styleResponse) { return *result; } @@ -219,7 +225,7 @@ OfflineRegionStatus OfflineDownload::getStatus() const { if (!parser.glyphURL.empty()) { result->requiredResourceCount += parser.fontStacks().size() * - (definition.match([](auto& reg) { return reg.includeIdeographs; }) + (std::visit([](auto& reg) { return reg.includeIdeographs; }, definition) ? GLYPH_RANGES_PER_FONT_STACK : NON_IDEOGRAPH_GLYPH_RANGES_PER_FONT_STACK); } @@ -236,7 +242,7 @@ void OfflineDownload::activateDownload() { status.downloadState = OfflineRegionDownloadState::Active; status.requiredResourceCount++; - auto styleResource = Resource::style(definition.match([](auto& reg) { return reg.styleURL; })); + auto styleResource = Resource::style(std::visit([](auto& reg) { return reg.styleURL; }, definition)); styleResource.setPriority(Resource::Priority::Low); styleResource.setUsage(Resource::Usage::Offline); @@ -330,7 +336,7 @@ void OfflineDownload::activateDownload() { } if (!parser.glyphURL.empty()) { - const bool includeIdeographs = definition.match([](auto& reg) { return reg.includeIdeographs; }); + const bool includeIdeographs = std::visit([](auto& reg) { return reg.includeIdeographs; }, definition); for (const auto& fontStack : parser.fontStacks()) { for (char16_t i = 0; i < GLYPH_RANGES_PER_FONT_STACK; i++) { // Assumes that if a glyph range starts with fixed width/ideographic @@ -438,7 +444,7 @@ void OfflineDownload::queueTiles(SourceType type, uint16_t tileSize, const Tiles status.requiredTileCount++; auto tileResource = Resource::tile(tileset.tiles[0], - definition.match([](auto& def) { return def.pixelRatio; }), + std::visit([](auto& def) { return def.pixelRatio; }, definition), tile.x, tile.y, tile.z, diff --git a/test/storage/offline.test.cpp b/test/storage/offline.test.cpp index 99ed8ff70..2e9d0547a 100644 --- a/test/storage/offline.test.cpp +++ b/test/storage/offline.test.cpp @@ -11,7 +11,7 @@ TEST(OfflineTilePyramidRegionDefinition, EncodeDecode) { "maptiler://style", LatLngBounds::hull({37.6609, -122.5744}, {37.8271, -122.3204}), 0, 20, 1.0, true); auto encoded = encodeOfflineRegionDefinition(region); - auto decoded = decodeOfflineRegionDefinition(encoded).get(); + auto decoded = std::get(decodeOfflineRegionDefinition(encoded)); EXPECT_EQ(decoded.styleURL, region.styleURL); EXPECT_EQ(decoded.minZoom, region.minZoom); @@ -26,7 +26,7 @@ TEST(OfflineGeometryRegionDefinition, EncodeDecode) { OfflineGeometryRegionDefinition region("maptiler://style", Point(-122.5744, 37.6609), 0, 2, 1.0, false); auto encoded = encodeOfflineRegionDefinition(region); - auto decoded = decodeOfflineRegionDefinition(encoded).get(); + auto decoded = std::get(decodeOfflineRegionDefinition(encoded)); EXPECT_EQ(decoded.styleURL, region.styleURL); EXPECT_EQ(decoded.minZoom, region.minZoom); diff --git a/test/storage/offline_database.test.cpp b/test/storage/offline_database.test.cpp index b3ae5f6d3..8a3bec098 100644 --- a/test/storage/offline_database.test.cpp +++ b/test/storage/offline_database.test.cpp @@ -9,8 +9,10 @@ #include #include +#include #include #include +#include using namespace std::literals::string_literals; using namespace mbgl; @@ -437,16 +439,18 @@ TEST(OfflineDatabase, CreateRegion) { EXPECT_EQ(0u, log.uncheckedCount()); - region->getDefinition().match( - [&](OfflineTilePyramidRegionDefinition& def) { - EXPECT_EQ(definition.styleURL, def.styleURL); - EXPECT_EQ(definition.bounds, def.bounds); - EXPECT_EQ(definition.minZoom, def.minZoom); - EXPECT_EQ(definition.maxZoom, def.maxZoom); - EXPECT_EQ(definition.pixelRatio, def.pixelRatio); - EXPECT_EQ(definition.includeIdeographs, def.includeIdeographs); - }, - [](auto&) { EXPECT_FALSE(false); }); + std::visit(overloaded{[&](OfflineTilePyramidRegionDefinition& def) { + EXPECT_EQ(definition.styleURL, def.styleURL); + EXPECT_EQ(definition.bounds, def.bounds); + EXPECT_EQ(definition.minZoom, def.minZoom); + EXPECT_EQ(definition.maxZoom, def.maxZoom); + EXPECT_EQ(definition.pixelRatio, def.pixelRatio); + EXPECT_EQ(definition.includeIdeographs, def.includeIdeographs); + }, + [](auto&) { + EXPECT_FALSE(false); + }}, + region->getDefinition()); EXPECT_EQ(metadata, region->getMetadata()); } @@ -481,16 +485,18 @@ TEST(OfflineDatabase, ListRegions) { ASSERT_EQ(1u, regions.size()); EXPECT_EQ(region->getID(), regions.at(0).getID()); - regions.at(0).getDefinition().match( - [&](OfflineTilePyramidRegionDefinition& def) { - EXPECT_EQ(definition.styleURL, def.styleURL); - EXPECT_EQ(definition.bounds, def.bounds); - EXPECT_EQ(definition.minZoom, def.minZoom); - EXPECT_EQ(definition.maxZoom, def.maxZoom); - EXPECT_EQ(definition.pixelRatio, def.pixelRatio); - EXPECT_EQ(definition.includeIdeographs, def.includeIdeographs); - }, - [&](auto&) { EXPECT_FALSE(false); }); + std::visit(overloaded{[&](OfflineTilePyramidRegionDefinition& def) { + EXPECT_EQ(definition.styleURL, def.styleURL); + EXPECT_EQ(definition.bounds, def.bounds); + EXPECT_EQ(definition.minZoom, def.minZoom); + EXPECT_EQ(definition.maxZoom, def.maxZoom); + EXPECT_EQ(definition.pixelRatio, def.pixelRatio); + EXPECT_EQ(definition.includeIdeographs, def.includeIdeographs); + }, + [&](auto&) { + EXPECT_FALSE(false); + }}, + regions.at(0).getDefinition()); EXPECT_EQ(metadata, regions.at(0).getMetadata()); EXPECT_EQ(0u, log.uncheckedCount()); @@ -506,17 +512,20 @@ TEST(OfflineDatabase, GetRegionDefinition) { EXPECT_EQ(0u, log.uncheckedCount()); auto region = db.createRegion(definition, metadata); - db.getRegionDefinition(region->getID()) - ->match( - [&](OfflineTilePyramidRegionDefinition& result) { - EXPECT_EQ(definition.styleURL, result.styleURL); - EXPECT_EQ(definition.bounds, result.bounds); - EXPECT_EQ(definition.minZoom, result.minZoom); - EXPECT_EQ(definition.maxZoom, result.maxZoom); - EXPECT_EQ(definition.pixelRatio, result.pixelRatio); - EXPECT_EQ(definition.includeIdeographs, result.includeIdeographs); - }, - [&](auto&) { EXPECT_FALSE(false); }); + auto regionDefinition = db.getRegionDefinition(region->getID()).value(); + + std::visit(overloaded{[&](OfflineTilePyramidRegionDefinition& result) { + EXPECT_EQ(definition.styleURL, result.styleURL); + EXPECT_EQ(definition.bounds, result.bounds); + EXPECT_EQ(definition.minZoom, result.minZoom); + EXPECT_EQ(definition.maxZoom, result.maxZoom); + EXPECT_EQ(definition.pixelRatio, result.pixelRatio); + EXPECT_EQ(definition.includeIdeographs, result.includeIdeographs); + }, + [&](auto&) { + EXPECT_FALSE(false); + }}, + regionDefinition); } // Disabled due to flakiness: https://github.com/mapbox/mapbox-gl-native/issues/14966 @@ -1016,10 +1025,12 @@ TEST(OfflineDatabase, CreateRegionInfiniteMaxZoom) { EXPECT_EQ(0u, log.uncheckedCount()); - region->getDefinition().match([&](auto& def) { - EXPECT_EQ(0, def.minZoom); - EXPECT_EQ(INFINITY, def.maxZoom); - }); + std::visit( + [&](auto& def) { + EXPECT_EQ(0, def.minZoom); + EXPECT_EQ(INFINITY, def.maxZoom); + }, + region->getDefinition()); } #ifndef __QT__ // Qt doesn't support concurrent access to the same database.