tei/MapLibreParticleLab
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[core][android][darwin] Fix GeoJSONOptions handling

Browse Source 
- share the `GeoJSONOptions` instances using `Immutable<GeoJSONOptions>` - avoid extra copying
- fix wrapping of the `GeoJSONOptions` instances in supercluster map/reduce lambdas.
  Previously, local variables were wrapped by reference.
This commit is contained in:
Mikhail Pozdnyakov
2019-11-28 18:19:17 +02:00
parent e3d1daaf66
commit cd4136ed4b
12 changed files with 104 additions and 97 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
include/mbgl/style/sources/geojson_source.hpp
View File

@@ -34,10 +34,13 @@ struct GeoJSONOptions {
std::shared_ptr<mbgl::style::expression::Expression>>;
using ClusterProperties = std::unordered_map<std::string, ClusterExpression>;
ClusterProperties clusterProperties;
static Immutable<GeoJSONOptions> defaultOptions();
};
class GeoJSONData {
public:
static std::shared_ptr<GeoJSONData> create(const GeoJSON&, const GeoJSONOptions&);
static std::shared_ptr<GeoJSONData> create(const GeoJSON&,
Immutable<GeoJSONOptions> = GeoJSONOptions::defaultOptions());
virtual ~GeoJSONData() = default;
virtual mapbox::feature::feature_collection<int16_t> getTile(const CanonicalTileID&) = 0;
@@ -52,7 +55,7 @@ public:
class GeoJSONSource final : public Source {
public:
GeoJSONSource(const std::string& id, optional<GeoJSONOptions> = nullopt);
GeoJSONSource(std::string id, Immutable<GeoJSONOptions> = GeoJSONOptions::defaultOptions());
~GeoJSONSource() final;
void setURL(const std::string& url);

72
platform/android/src/style/sources/geojson_source.cpp
View File

@@ -1,4 +1,5 @@
#include "geojson_source.hpp"
#include <mbgl/style/sources/geojson_source_impl.hpp>
#include "../../attach_env.hpp"
#include <mbgl/renderer/query.hpp>
@@ -29,44 +30,42 @@ namespace android {
// This conversion is expected not to fail because it's used only in contexts where
// the value was originally a GeoJsonOptions object on the Java side. If it fails
// to convert, it's a bug in our serialization or Java-side static typing.
static style::GeoJSONOptions convertGeoJSONOptions(jni::JNIEnv& env, const jni::Object<>& options) {
using namespace mbgl::style::conversion;
if (!options) {
return style::GeoJSONOptions();
}
Error error;
optional<style::GeoJSONOptions> result = convert<style::GeoJSONOptions>(
mbgl::android::Value(env, options), error);
if (!result) {
throw std::logic_error(error.message);
}
return *result;
static Immutable<style::GeoJSONOptions> convertGeoJSONOptions(jni::JNIEnv& env, const jni::Object<>& options) {
using namespace mbgl::style::conversion;
if (!options) {
return style::GeoJSONOptions::defaultOptions();
}
GeoJSONSource::GeoJSONSource(jni::JNIEnv& env, const jni::String& sourceId, const jni::Object<>& options)
: Source(env,
std::make_unique<mbgl::style::GeoJSONSource>(jni::Make<std::string>(env, sourceId),
convertGeoJSONOptions(env, options))),
converter(std::make_unique<Actor<FeatureConverter>>(Scheduler::GetBackground(),
source.as<style::GeoJSONSource>()->getOptions())) {}
GeoJSONSource::GeoJSONSource(jni::JNIEnv& env, mbgl::style::Source& coreSource, AndroidRendererFrontend& frontend)
: Source(env, coreSource, createJavaPeer(env), frontend),
converter(std::make_unique<Actor<FeatureConverter>>(Scheduler::GetBackground(),
source.as<style::GeoJSONSource>()->getOptions())) {}
GeoJSONSource::~GeoJSONSource() = default;
void GeoJSONSource::setGeoJSONString(jni::JNIEnv& env, const jni::String& jString) {
std::shared_ptr<std::string> json = std::make_shared<std::string>(jni::Make<std::string>(env, jString));
Update::Converter converterFn = [this, json](ActorRef<GeoJSONDataCallback> _callback) {
converter->self().invoke(&FeatureConverter::convertJson, json, _callback);
};
setAsync(converterFn);
Error error;
optional<style::GeoJSONOptions> result = convert<style::GeoJSONOptions>(mbgl::android::Value(env, options), error);
if (!result) {
throw std::logic_error(error.message);
}
return makeMutable<style::GeoJSONOptions>(std::move(*result));
}
GeoJSONSource::GeoJSONSource(jni::JNIEnv& env, const jni::String& sourceId, const jni::Object<>& options)
: Source(env,
std::make_unique<mbgl::style::GeoJSONSource>(jni::Make<std::string>(env, sourceId),
convertGeoJSONOptions(env, options))),
converter(std::make_unique<Actor<FeatureConverter>>(Scheduler::GetBackground(),
source.as<style::GeoJSONSource>()->impl().getOptions())) {}
GeoJSONSource::GeoJSONSource(jni::JNIEnv& env, mbgl::style::Source& coreSource, AndroidRendererFrontend& frontend)
: Source(env, coreSource, createJavaPeer(env), frontend),
converter(std::make_unique<Actor<FeatureConverter>>(Scheduler::GetBackground(),
source.as<style::GeoJSONSource>()->impl().getOptions())) {}
GeoJSONSource::~GeoJSONSource() = default;
void GeoJSONSource::setGeoJSONString(jni::JNIEnv& env, const jni::String& jString) {
std::shared_ptr<std::string> json = std::make_shared<std::string>(jni::Make<std::string>(env, jString));
Update::Converter converterFn = [this, json](ActorRef<GeoJSONDataCallback> _callback) {
converter->self().invoke(&FeatureConverter::convertJson, json, _callback);
};
setAsync(converterFn);
}
void GeoJSONSource::setFeatureCollection(jni::JNIEnv& env, const jni::Object<geojson::FeatureCollection>& jFeatures) {
setCollectionAsync(env, jFeatures);
@@ -237,6 +236,7 @@ namespace android {
mbgl::Log::Error(mbgl::Event::JNI, "Error setting geo json: " + error.message);
return;
}
callback.invoke(&GeoJSONDataCallback::operator(), style::GeoJSONData::create(*converted, options));
}

4
platform/android/src/style/sources/geojson_source.hpp
View File

@@ -15,7 +15,7 @@ using GeoJSONDataCallback = std::function<void(std::shared_ptr<style::GeoJSONDat
class FeatureConverter {
public:
explicit FeatureConverter(style::GeoJSONOptions options_) : options(std::move(options_)) {}
explicit FeatureConverter(Immutable<style::GeoJSONOptions> options_) : options(std::move(options_)) {}
void convertJson(std::shared_ptr<std::string>, ActorRef<GeoJSONDataCallback>);
template <class JNIType>
@@ -23,7 +23,7 @@ public:
ActorRef<GeoJSONDataCallback>);
private:
style::GeoJSONOptions options;
Immutable<style::GeoJSONOptions> options;
};
struct Update {

26
platform/darwin/src/MGLShapeSource.mm
View File

@@ -27,15 +27,15 @@ const MGLShapeSourceOption MGLShapeSourceOptionMinimumZoomLevel = @"MGLShapeSour
const MGLShapeSourceOption MGLShapeSourceOptionSimplificationTolerance = @"MGLShapeSourceOptionSimplificationTolerance";
const MGLShapeSourceOption MGLShapeSourceOptionLineDistanceMetrics = @"MGLShapeSourceOptionLineDistanceMetrics";
mbgl::style::GeoJSONOptions MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShapeSourceOption, id> *options) {
auto geoJSONOptions = mbgl::style::GeoJSONOptions();
mbgl::Immutable<mbgl::style::GeoJSONOptions> MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShapeSourceOption, id> *options) {
auto geoJSONOptions = mbgl::makeMutable<mbgl::style::GeoJSONOptions>();
if (NSNumber *value = options[MGLShapeSourceOptionMinimumZoomLevel]) {
if (![value isKindOfClass:[NSNumber class]]) {
[NSException raise:NSInvalidArgumentException
format:@"MGLShapeSourceOptionMaximumZoomLevel must be an NSNumber."];
}
geoJSONOptions.minzoom = value.integerValue;
geoJSONOptions->minzoom = value.integerValue;
}
if (NSNumber *value = options[MGLShapeSourceOptionMaximumZoomLevel]) {
@@ -43,7 +43,7 @@ mbgl::style::GeoJSONOptions MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShap
[NSException raise:NSInvalidArgumentException
format:@"MGLShapeSourceOptionMaximumZoomLevel must be an NSNumber."];
}
geoJSONOptions.maxzoom = value.integerValue;
geoJSONOptions->maxzoom = value.integerValue;
}
if (NSNumber *value = options[MGLShapeSourceOptionBuffer]) {
@@ -51,7 +51,7 @@ mbgl::style::GeoJSONOptions MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShap
[NSException raise:NSInvalidArgumentException
format:@"MGLShapeSourceOptionBuffer must be an NSNumber."];
}
geoJSONOptions.buffer = value.integerValue;
geoJSONOptions->buffer = value.integerValue;
}
if (NSNumber *value = options[MGLShapeSourceOptionSimplificationTolerance]) {
@@ -59,7 +59,7 @@ mbgl::style::GeoJSONOptions MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShap
[NSException raise:NSInvalidArgumentException
format:@"MGLShapeSourceOptionSimplificationTolerance must be an NSNumber."];
}
geoJSONOptions.tolerance = value.doubleValue;
geoJSONOptions->tolerance = value.doubleValue;
}
if (NSNumber *value = options[MGLShapeSourceOptionClusterRadius]) {
@@ -67,7 +67,7 @@ mbgl::style::GeoJSONOptions MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShap
[NSException raise:NSInvalidArgumentException
format:@"MGLShapeSourceOptionClusterRadius must be an NSNumber."];
}
geoJSONOptions.clusterRadius = value.integerValue;
geoJSONOptions->clusterRadius = value.integerValue;
}
if (NSNumber *value = options[MGLShapeSourceOptionMaximumZoomLevelForClustering]) {
@@ -75,7 +75,7 @@ mbgl::style::GeoJSONOptions MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShap
[NSException raise:NSInvalidArgumentException
format:@"MGLShapeSourceOptionMaximumZoomLevelForClustering must be an NSNumber."];
}
geoJSONOptions.clusterMaxZoom = value.integerValue;
geoJSONOptions->clusterMaxZoom = value.integerValue;
}
if (NSNumber *value = options[MGLShapeSourceOptionClustered]) {
@@ -83,7 +83,7 @@ mbgl::style::GeoJSONOptions MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShap
[NSException raise:NSInvalidArgumentException
format:@"MGLShapeSourceOptionClustered must be an NSNumber."];
}
geoJSONOptions.cluster = value.boolValue;
geoJSONOptions->cluster = value.boolValue;
}
if (NSDictionary *value = options[MGLShapeSourceOptionClusterProperties]) {
@@ -133,7 +133,7 @@ mbgl::style::GeoJSONOptions MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShap
std::string keyString = std::string([key UTF8String]);
geoJSONOptions.clusterProperties.emplace(keyString, std::make_pair(std::move(map), std::move(reduce)));
geoJSONOptions->clusterProperties.emplace(keyString, std::make_pair(std::move(map), std::move(reduce)));
}
}
@@ -142,7 +142,7 @@ mbgl::style::GeoJSONOptions MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShap
[NSException raise:NSInvalidArgumentException
format:@"MGLShapeSourceOptionLineDistanceMetrics must be an NSNumber."];
}
geoJSONOptions.lineMetrics = value.boolValue;
geoJSONOptions->lineMetrics = value.boolValue;
}
return geoJSONOptions;
@@ -159,7 +159,7 @@ mbgl::style::GeoJSONOptions MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShap
- (instancetype)initWithIdentifier:(NSString *)identifier URL:(NSURL *)url options:(NSDictionary<NSString *, id> *)options {
auto geoJSONOptions = MGLGeoJSONOptionsFromDictionary(options);
auto source = std::make_unique<mbgl::style::GeoJSONSource>(identifier.UTF8String, geoJSONOptions);
auto source = std::make_unique<mbgl::style::GeoJSONSource>(identifier.UTF8String, std::move(geoJSONOptions));
if (self = [super initWithPendingSource:std::move(source)]) {
self.URL = url;
}
@@ -168,7 +168,7 @@ mbgl::style::GeoJSONOptions MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShap
- (instancetype)initWithIdentifier:(NSString *)identifier shape:(nullable MGLShape *)shape options:(NSDictionary<MGLShapeSourceOption, id> *)options {
auto geoJSONOptions = MGLGeoJSONOptionsFromDictionary(options);
auto source = std::make_unique<mbgl::style::GeoJSONSource>(identifier.UTF8String, geoJSONOptions);
auto source = std::make_unique<mbgl::style::GeoJSONSource>(identifier.UTF8String, std::move(geoJSONOptions));
if (self = [super initWithPendingSource:std::move(source)]) {
if ([shape isMemberOfClass:[MGLShapeCollection class]]) {
static dispatch_once_t onceToken;

4
platform/darwin/src/MGLShapeSource_Private.h
View File

@@ -1,6 +1,8 @@
#import "MGLFoundation.h"
#import "MGLShapeSource.h"
#include <mbgl/util/immutable.hpp>
NS_ASSUME_NONNULL_BEGIN
namespace mbgl {
@@ -10,7 +12,7 @@ namespace mbgl {
}
MGL_EXPORT
mbgl::style::GeoJSONOptions MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShapeSourceOption, id> *options);
mbgl::Immutable<mbgl::style::GeoJSONOptions> MGLGeoJSONOptionsFromDictionary(NSDictionary<MGLShapeSourceOption, id> *options);
@interface MGLShapeSource (Private)

16
platform/darwin/test/MGLShapeSourceTests.mm
View File

@@ -26,14 +26,14 @@
MGLShapeSourceOptionLineDistanceMetrics: @YES};
auto mbglOptions = MGLGeoJSONOptionsFromDictionary(options);
XCTAssertTrue(mbglOptions.cluster);
XCTAssertEqual(mbglOptions.clusterRadius, 42);
XCTAssertEqual(mbglOptions.clusterMaxZoom, 98);
XCTAssertEqual(mbglOptions.maxzoom, 99);
XCTAssertEqual(mbglOptions.buffer, 1976);
XCTAssertEqual(mbglOptions.tolerance, 0.42);
XCTAssertTrue(mbglOptions.lineMetrics);
XCTAssertTrue(!mbglOptions.clusterProperties.empty());
XCTAssertTrue(mbglOptions->cluster);
XCTAssertEqual(mbglOptions->clusterRadius, 42);
XCTAssertEqual(mbglOptions->clusterMaxZoom, 98);
XCTAssertEqual(mbglOptions->maxzoom, 99);
XCTAssertEqual(mbglOptions->buffer, 1976);
XCTAssertEqual(mbglOptions->tolerance, 0.42);
XCTAssertTrue(mbglOptions->lineMetrics);
XCTAssertTrue(!mbglOptions->clusterProperties.empty());
options = @{MGLShapeSourceOptionClustered: @"number 1"};
XCTAssertThrows(MGLGeoJSONOptionsFromDictionary(options));

8
src/mbgl/style/conversion/source.cpp
View File

@@ -116,12 +116,12 @@ static optional<std::unique_ptr<Source>> convertGeoJSONSource(const std::string&
return nullopt;
}
optional<GeoJSONOptions> options = convert<GeoJSONOptions>(value, error);
if (!options) {
return nullopt;
Immutable<GeoJSONOptions> options = GeoJSONOptions::defaultOptions();
if (optional<GeoJSONOptions> converted = convert<GeoJSONOptions>(value, error)) {
options = makeMutable<GeoJSONOptions>(std::move(*converted));
}
auto result = std::make_unique<GeoJSONSource>(id, *options);
auto result = std::make_unique<GeoJSONSource>(id, std::move(options));
if (isObject(*dataValue)) {
optional<GeoJSON> geoJSON = convert<GeoJSON>(*dataValue, error);

12
src/mbgl/style/sources/geojson_source.cpp
View File

@@ -12,8 +12,14 @@
namespace mbgl {
namespace style {
GeoJSONSource::GeoJSONSource(const std::string& id, optional<GeoJSONOptions> options)
: Source(makeMutable<Impl>(id, options)) {}
// static
Immutable<GeoJSONOptions> GeoJSONOptions::defaultOptions() {
static Immutable<GeoJSONOptions> options = makeMutable<GeoJSONOptions>();
return options;
}
GeoJSONSource::GeoJSONSource(std::string id, Immutable<GeoJSONOptions> options)
: Source(makeMutable<Impl>(std::move(id), std::move(options))) {}
GeoJSONSource::~GeoJSONSource() = default;
@@ -47,7 +53,7 @@ optional<std::string> GeoJSONSource::getURL() const {
}
const GeoJSONOptions& GeoJSONSource::getOptions() const {
return impl().getOptions();
return *impl().getOptions();
}
void GeoJSONSource::loadDescription(FileSource& fileSource) {

32
src/mbgl/style/sources/geojson_source_impl.cpp
View File

@@ -83,26 +83,26 @@ T evaluateFeature(const mapbox::feature::feature<double>& f,
}
// static
std::shared_ptr<GeoJSONData> GeoJSONData::create(const GeoJSON& geoJSON, const GeoJSONOptions& options) {
std::shared_ptr<GeoJSONData> GeoJSONData::create(const GeoJSON& geoJSON, Immutable<GeoJSONOptions> options) {
constexpr double scale = util::EXTENT / util::tileSize;
if (options.cluster && geoJSON.is<mapbox::feature::feature_collection<double>>() &&
if (options->cluster && geoJSON.is<mapbox::feature::feature_collection<double>>() &&
!geoJSON.get<mapbox::feature::feature_collection<double>>().empty()) {
mapbox::supercluster::Options clusterOptions;
clusterOptions.maxZoom = options.clusterMaxZoom;
clusterOptions.maxZoom = options->clusterMaxZoom;
clusterOptions.extent = util::EXTENT;
clusterOptions.radius = ::round(scale * options.clusterRadius);
clusterOptions.radius = ::round(scale * options->clusterRadius);
Feature feature;
clusterOptions.map = [&](const PropertyMap& properties) -> PropertyMap {
clusterOptions.map = [&, options](const PropertyMap& properties) -> PropertyMap {
PropertyMap ret{};
if (properties.empty()) return ret;
for (const auto& p : options.clusterProperties) {
for (const auto& p : options->clusterProperties) {
feature.properties = properties;
ret[p.first] = evaluateFeature<Value>(feature, p.second.first);
}
return ret;
};
clusterOptions.reduce = [&](PropertyMap& toReturn, const PropertyMap& toFill) {
for (const auto& p : options.clusterProperties) {
clusterOptions.reduce = [&, options](PropertyMap& toReturn, const PropertyMap& toFill) {
for (const auto& p : options->clusterProperties) {
if (toFill.count(p.first) == 0) {
continue;
}
@@ -116,18 +116,16 @@ std::shared_ptr<GeoJSONData> GeoJSONData::create(const GeoJSON& geoJSON, const G
}
mapbox::geojsonvt::Options vtOptions;
vtOptions.maxZoom = options.maxzoom;
vtOptions.maxZoom = options->maxzoom;
vtOptions.extent = util::EXTENT;
vtOptions.buffer = ::round(scale * options.buffer);
vtOptions.tolerance = scale * options.tolerance;
vtOptions.lineMetrics = options.lineMetrics;
vtOptions.buffer = ::round(scale * options->buffer);
vtOptions.tolerance = scale * options->tolerance;
vtOptions.lineMetrics = options->lineMetrics;
return std::make_shared<GeoJSONVTData>(geoJSON, vtOptions);
}
GeoJSONSource::Impl::Impl(std::string id_, optional<GeoJSONOptions> options_)
: Source::Impl(SourceType::GeoJSON, std::move(id_)) {
options = options_ ? std::move(*options_) : GeoJSONOptions{};
}
GeoJSONSource::Impl::Impl(std::string id_, Immutable<GeoJSONOptions> options_)
: Source::Impl(SourceType::GeoJSON, std::move(id_)), options(std::move(options_)) {}
GeoJSONSource::Impl::Impl(const GeoJSONSource::Impl& other, std::shared_ptr<GeoJSONData> data_)
: Source::Impl(other), options(other.options), data(std::move(data_)) {}
@@ -135,7 +133,7 @@ GeoJSONSource::Impl::Impl(const GeoJSONSource::Impl& other, std::shared_ptr<GeoJ
GeoJSONSource::Impl::~Impl() = default;
Range<uint8_t> GeoJSONSource::Impl::getZoomRange() const {
return { options.minzoom, options.maxzoom };
return {options->minzoom, options->maxzoom};
}
std::weak_ptr<GeoJSONData> GeoJSONSource::Impl::getData() const {

6
src/mbgl/style/sources/geojson_source_impl.hpp
View File

@@ -13,18 +13,18 @@ namespace style {
class GeoJSONSource::Impl : public Source::Impl {
public:
Impl(std::string id, optional<GeoJSONOptions>);
Impl(std::string id, Immutable<GeoJSONOptions>);
Impl(const GeoJSONSource::Impl&, std::shared_ptr<GeoJSONData>);
~Impl() final;
Range<uint8_t> getZoomRange() const;
std::weak_ptr<GeoJSONData> getData() const;
const GeoJSONOptions& getOptions() const { return options; }
const Immutable<GeoJSONOptions>& getOptions() const { return options; }
optional<std::string> getAttribution() const final;
private:
GeoJSONOptions options;
Immutable<GeoJSONOptions> options;
std::shared_ptr<GeoJSONData> data;
};

6
test/api/query.test.cpp
View File

@@ -48,9 +48,9 @@ std::vector<Feature> getTopClusterFeature(QueryTest& test) {
};
LatLng coordinate{0, 0};
GeoJSONOptions options{};
options.cluster = true;
auto source = std::make_unique<GeoJSONSource>("cluster_source"s, options);
Mutable<GeoJSONOptions> options = makeMutable<GeoJSONOptions>();
options->cluster = true;
auto source = std::make_unique<GeoJSONSource>("cluster_source"s, std::move(options));
source->setURL("http://url"s);
source->loadDescription(*test.fileSource);

8
test/style/source.test.cpp
View File

@@ -852,10 +852,8 @@ TEST(Source, RenderTileSetSourceUpdate) {
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": {}})"),
{});
auto geoJSONData = GeoJSONData::create(mapbox::geojson::parse(
R"({"geometry": {"type": "Point", "coordinates": [1.1, 1.1]}, "type": "Feature", "properties": {}})"));
source.setGeoJSONData(geoJSONData);
RenderGeoJSONSource renderSource{staticImmutableCast<GeoJSONSource::Impl>(source.baseImpl)};
@@ -879,4 +877,4 @@ TEST(Source, GeoJSONSourceTilesAfterDataReset) {
static_cast<RenderSource&>(renderSource)
.update(source.baseImpl, layers, true, true, test.tileParameters(MapMode::Static));
EXPECT_TRUE(renderSource.isLoaded()); // Tiles are reset in static mode.
}
}