Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Avoid geometry collections copying #15201

Merged
merged 4 commits into from
Jul 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class StubGeometryTileFeature : public GeometryTileFeature {
return properties.count(key) ? properties.at(key) : optional<Value>();
}

GeometryCollection getGeometries() const override {
const GeometryCollection& getGeometries() const override {
return geometry;
}
};
2 changes: 1 addition & 1 deletion src/mbgl/annotation/annotation_tile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ FeatureIdentifier AnnotationTileFeature::getID() const {
return data->id;
}

GeometryCollection AnnotationTileFeature::getGeometries() const {
const GeometryCollection& AnnotationTileFeature::getGeometries() const {
return data->geometries;
}

Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/annotation/annotation_tile.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class AnnotationTileFeature : public GeometryTileFeature {
FeatureType getType() const override;
optional<Value> getValue(const std::string&) const override;
FeatureIdentifier getID() const override;
GeometryCollection getGeometries() const override;
const GeometryCollection& getGeometries() const override;

private:
std::shared_ptr<const AnnotationTileFeatureData> data;
Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/geometry/feature_index.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ void FeatureIndex::addFeature(
continue;
}

result[layerID].push_back(convertFeature(*geometryTileFeature, tileID));
result[layerID].emplace_back(convertFeature(*geometryTileFeature, tileID));
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/layout/pattern_layout.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ class PatternLayout : public Layout {
const auto i = patternFeature.i;
std::unique_ptr<GeometryTileFeature> feature = std::move(patternFeature.feature);
const PatternLayerMap& patterns = patternFeature.patterns;
GeometryCollection geometries = feature->getGeometries();
const GeometryCollection& geometries = feature->getGeometries();

bucket->addFeature(*feature, geometries, patternPositions, patterns);
featureIndex->insert(geometries, i, sourceLayerID, bucketLeaderID);
Expand Down
4 changes: 2 additions & 2 deletions src/mbgl/layout/symbol_feature.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ class SymbolFeature : public GeometryTileFeature {
public:
SymbolFeature(std::unique_ptr<GeometryTileFeature> feature_) :
feature(std::move(feature_)),
geometry(feature->getGeometries()) // we need a mutable copy of the geometry for mergeLines()
geometry(feature->getGeometries().clone()) // we need a mutable copy of the geometry for mergeLines()
{}

FeatureType getType() const override { return feature->getType(); }
optional<Value> getValue(const std::string& key) const override { return feature->getValue(key); };
std::unordered_map<std::string,Value> getProperties() const override { return feature->getProperties(); };
FeatureIdentifier getID() const override { return feature->getID(); };
GeometryCollection getGeometries() const override { return geometry; };
const GeometryCollection& getGeometries() const override { return geometry; };

friend bool operator < (const SymbolFeature& lhs, const SymbolFeature& rhs) {
return lhs.sortKey < rhs.sortKey;
Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/renderer/layers/render_circle_layer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ bool RenderCircleLayer::queryIntersectsFeature(
projectQueryGeometry(translatedQueryGeometry, posMatrix, transformState.getSize());
auto transformedSize = alignWithMap ? size * pixelsToTileUnits : size;

auto geometry = feature.getGeometries();
const auto& geometry = feature.getGeometries();
for (auto& ring : geometry) {
for (auto& point : ring) {
const GeometryCoordinate& transformedPoint = alignWithMap ? point : projectPoint(point, posMatrix, transformState.getSize());
Expand Down
35 changes: 24 additions & 11 deletions src/mbgl/renderer/layers/render_line_layer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -214,16 +214,22 @@ void RenderLineLayer::render(PaintParameters& parameters) {
}
}

optional<GeometryCollection> offsetLine(const GeometryCollection& rings, const double offset) {
if (offset == 0) return {};
namespace {

GeometryCollection offsetLine(const GeometryCollection& rings, double offset) {
assert(offset != 0.0f);
assert(!rings.empty());

GeometryCollection newRings;
Point<double> zero(0, 0);
newRings.reserve(rings.size());

const Point<double> zero(0, 0);
for (const auto& ring : rings) {
newRings.emplace_back();
auto& newRing = newRings.back();
newRing.reserve(ring.size());

pozdnyakov marked this conversation as resolved.
Show resolved Hide resolved
for (auto i = ring.begin(); i != ring.end(); i++) {
for (auto i = ring.begin(); i != ring.end(); ++i) {
auto& p = *i;

Point<double> aToB = i == ring.begin() ?
Expand All @@ -237,13 +243,15 @@ optional<GeometryCollection> offsetLine(const GeometryCollection& rings, const d
const double cosHalfAngle = extrude.x * bToC.x + extrude.y * bToC.y;
extrude *= (1.0 / cosHalfAngle);

newRing.push_back(convertPoint<int16_t>(extrude * offset) + p);
newRing.emplace_back(convertPoint<int16_t>(extrude * offset) + p);
}
}

return newRings;
}

} // namespace

bool RenderLineLayer::queryIntersectsFeature(
const GeometryCoordinates& queryGeometry,
const GeometryTileFeature& feature,
Expand All @@ -263,16 +271,21 @@ bool RenderLineLayer::queryIntersectsFeature(
// Evaluate function
auto offset = evaluated.get<style::LineOffset>()
.evaluate(feature, zoom, style::LineOffset::defaultValue()) * pixelsToTileUnits;
// Test intersection
const float halfWidth = getLineWidth(feature, zoom) / 2.0 * pixelsToTileUnits;

// Apply offset to geometry
auto offsetGeometry = offsetLine(feature.getGeometries(), offset);
if (offset != 0.0f && !feature.getGeometries().empty()) {
return util::polygonIntersectsBufferedMultiLine(
translatedQueryGeometry.value_or(queryGeometry),
offsetLine(feature.getGeometries(), offset),
halfWidth);
}

// Test intersection
const float halfWidth = getLineWidth(feature, zoom) / 2.0 * pixelsToTileUnits;
return util::polygonIntersectsBufferedMultiLine(
translatedQueryGeometry.value_or(queryGeometry),
offsetGeometry.value_or(feature.getGeometries()),
halfWidth);
translatedQueryGeometry.value_or(queryGeometry),
feature.getGeometries(),
halfWidth);
}

void RenderLineLayer::updateColorRamp() {
Expand Down
1 change: 0 additions & 1 deletion src/mbgl/style/expression/expression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ class GeoJSONFeature : public GeometryTileFeature {
}
PropertyMap getProperties() const override { return feature.properties; }
FeatureIdentifier getID() const override { return feature.id; }
GeometryCollection getGeometries() const override { return {}; }
optional<mbgl::Value> getValue(const std::string& key) const override {
auto it = feature.properties.find(key);
if (it != feature.properties.end()) {
Expand Down
18 changes: 11 additions & 7 deletions src/mbgl/tile/geojson_tile_data.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,17 @@ class GeoJSONTileFeature : public GeometryTileFeature {
return feature.id;
}

GeometryCollection getGeometries() const override {
GeometryCollection geometry = apply_visitor(ToGeometryCollection(), feature.geometry);

// https://github.com/mapbox/geojson-vt-cpp/issues/44
if (getType() == FeatureType::Polygon) {
geometry = fixupPolygons(geometry);
const GeometryCollection& getGeometries() const override {
if (!geometry) {
geometry = apply_visitor(ToGeometryCollection(), feature.geometry);

// https://github.com/mapbox/geojson-vt-cpp/issues/44
if (getType() == FeatureType::Polygon) {
geometry = fixupPolygons(*geometry);
}
}

return geometry;
return *geometry;
}

optional<Value> getValue(const std::string& key) const override {
Expand All @@ -43,6 +45,8 @@ class GeoJSONTileFeature : public GeometryTileFeature {
}
return optional<Value>();
}

mutable optional<GeometryCollection> geometry;
};

class GeoJSONTileLayer : public GeometryTileLayer {
Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/tile/geometry_tile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ void GeometryTile::querySourceFeatures(
continue;
}

result.push_back(convertFeature(*feature, id.canonical));
result.emplace_back(convertFeature(*feature, id.canonical));
}
}
}
Expand Down
30 changes: 17 additions & 13 deletions src/mbgl/tile/geometry_tile_data.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,32 +57,31 @@ std::vector<GeometryCollection> classifyRings(const GeometryCollection& rings) {
std::size_t len = rings.size();

if (len <= 1) {
polygons.push_back(rings);
polygons.emplace_back(rings.clone());
return polygons;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I briefly checked in debugger - in majority of cases len == 1.
Following how classifyRings is used for (auto& polygon : classifyRings(geometry)) { suggest to redesign clasifyRings so that it doesn't clone() but access the same const reference.

}

GeometryCollection polygon;
int8_t ccw = 0;

for (std::size_t i = 0; i < len; i++) {
double area = signedArea(rings[i]);

if (area == 0)
continue;
for (const auto& ring : rings) {
double area = signedArea(ring);
if (area == 0) continue;

if (ccw == 0)
if (ccw == 0) {
ccw = (area < 0 ? -1 : 1);
}

if (ccw == (area < 0 ? -1 : 1) && !polygon.empty()) {
polygons.push_back(polygon);
polygon.clear();
polygons.emplace_back(std::move(polygon));
}

polygon.push_back(rings[i]);
polygon.emplace_back(ring);
}

if (!polygon.empty())
polygons.push_back(polygon);
if (!polygon.empty()) {
polygons.emplace_back(std::move(polygon));
}

return polygons;
}
Expand Down Expand Up @@ -112,7 +111,7 @@ static Feature::geometry_type convertGeometry(const GeometryTileFeature& geometr
);
};

GeometryCollection geometries = geometryTileFeature.getGeometries();
const GeometryCollection& geometries = geometryTileFeature.getGeometries();

switch (geometryTileFeature.getType()) {
case FeatureType::Unknown: {
Expand Down Expand Up @@ -180,4 +179,9 @@ Feature convertFeature(const GeometryTileFeature& geometryTileFeature, const Can
return feature;
}

const GeometryCollection& GeometryTileFeature::getGeometries() const {
static const GeometryCollection dummy;
return dummy;
}

} // namespace mbgl
9 changes: 8 additions & 1 deletion src/mbgl/tile/geometry_tile_data.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ class GeometryCollection : public std::vector<GeometryCoordinates> {
GeometryCollection(Args&&... args) : std::vector<GeometryCoordinates>(std::forward<Args>(args)...) {}
GeometryCollection(std::initializer_list<GeometryCoordinates> args)
: std::vector<GeometryCoordinates>(std::move(args)) {}
GeometryCollection(GeometryCollection&&) = default;
GeometryCollection& operator=(GeometryCollection&&) = default;

GeometryCollection clone() const { return GeometryCollection(*this); }

private:
GeometryCollection(const GeometryCollection&) = default;
};

class GeometryTileFeature {
Expand All @@ -44,7 +51,7 @@ class GeometryTileFeature {
virtual optional<Value> getValue(const std::string& key) const = 0;
virtual PropertyMap getProperties() const { return PropertyMap(); }
virtual FeatureIdentifier getID() const { return NullValue {}; }
virtual GeometryCollection getGeometries() const = 0;
virtual const GeometryCollection& getGeometries() const;
};

class GeometryTileLayer {
Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/tile/geometry_tile_worker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ void GeometryTileWorker::parse() {
if (!filter(expression::EvaluationContext { static_cast<float>(this->id.overscaledZ), feature.get() }))
continue;

GeometryCollection geometries = feature->getGeometries();
const GeometryCollection& geometries = feature->getGeometries();
bucket->addFeature(*feature, geometries, {}, PatternLayerMap ());
featureIndex->insert(geometries, i, sourceLayerID, leaderImpl.id);
}
Expand Down
15 changes: 8 additions & 7 deletions src/mbgl/tile/vector_tile_data.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,15 @@ FeatureIdentifier VectorTileFeature::getID() const {
return feature.getID();
}

GeometryCollection VectorTileFeature::getGeometries() const {
const float scale = float(util::EXTENT) / feature.getExtent();
auto lines = feature.getGeometries<GeometryCollection>(scale);
if (feature.getVersion() >= 2 || feature.getType() != mapbox::vector_tile::GeomType::POLYGON) {
return lines;
} else {
return fixupPolygons(lines);
const GeometryCollection& VectorTileFeature::getGeometries() const {
if (!lines) {
const float scale = float(util::EXTENT) / feature.getExtent();
lines = feature.getGeometries<GeometryCollection>(scale);
if (feature.getVersion() < 2 && feature.getType() == mapbox::vector_tile::GeomType::POLYGON) {
lines = fixupPolygons(*lines);
}
}
return *lines;
}

VectorTileLayer::VectorTileLayer(std::shared_ptr<const std::string> data_,
Expand Down
3 changes: 2 additions & 1 deletion src/mbgl/tile/vector_tile_data.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@ class VectorTileFeature : public GeometryTileFeature {
optional<Value> getValue(const std::string& key) const override;
std::unordered_map<std::string, Value> getProperties() const override;
FeatureIdentifier getID() const override;
GeometryCollection getGeometries() const override;
const GeometryCollection& getGeometries() const override;

private:
mapbox::vector_tile::feature feature;
mutable optional<GeometryCollection> lines;
};

class VectorTileLayer : public GeometryTileLayer {
Expand Down
2 changes: 1 addition & 1 deletion test/gl/bucket.test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ TEST(Buckets, SymbolBucket) {

// SymbolBucket::addFeature() is a no-op.
GeometryCollection point { { { 0, 0 } } };
bucket.addFeature(StubGeometryTileFeature { {}, FeatureType::Point, point, properties }, point, {}, PatternLayerMap());
bucket.addFeature(StubGeometryTileFeature { {}, FeatureType::Point, std::move(point), properties }, point, {}, PatternLayerMap());
ASSERT_FALSE(bucket.hasData());
ASSERT_FALSE(bucket.needsUpload());

Expand Down
2 changes: 1 addition & 1 deletion test/src/mbgl/test/stub_geometry_tile_feature.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class StubGeometryTileFeature : public GeometryTileFeature {
return properties.count(key) ? properties.at(key) : optional<Value>();
}

GeometryCollection getGeometries() const override {
const GeometryCollection& getGeometries() const override {
return geometry;
}
};
Expand Down
Loading