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

Clear out GeometryTileWorker::layers during redoLayout #7648

Closed
wants to merge 3 commits into from
Closed

Clear out GeometryTileWorker::layers during redoLayout #7648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f9bfb9
[core] Keep symbol and non-symbol buckets segregated
jfirebaugh Jan 6, 2017
fa2a465
[core] Replace FeatureIndex::collisionTile with a method parameter
jfirebaugh Jan 6, 2017
d8eed43
[core] clear out GeometryTileWorker::layers during redoLayout
kkaefer Jan 9, 2017
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
3 changes: 3 additions & 0 deletions include/mbgl/util/optional.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,7 @@ namespace mbgl {
template <typename T>
using optional = std::experimental::optional<T>;

using nullopt_t = std::experimental::nullopt_t;
constexpr auto nullopt = std::experimental::nullopt;

} // namespace mbgl
7 changes: 2 additions & 5 deletions src/mbgl/geometry/feature_index.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ void FeatureIndex::query(
const optional<std::vector<std::string>>& filterLayerIDs,
const GeometryTileData& geometryTileData,
const CanonicalTileID& tileID,
const style::Style& style) const {
const style::Style& style,
const CollisionTile* collisionTile) const {

mapbox::geometry::box<int16_t> box = mapbox::geometry::envelope(queryGeometry);

Expand Down Expand Up @@ -154,8 +155,4 @@ void FeatureIndex::addBucketLayerName(const std::string& bucketName, const std::
bucketLayerIDs[bucketName].push_back(layerID);
}

void FeatureIndex::setCollisionTile(std::unique_ptr<CollisionTile> collisionTile_) {
collisionTile = std::move(collisionTile_);
}

} // namespace mbgl
6 changes: 2 additions & 4 deletions src/mbgl/geometry/feature_index.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ class FeatureIndex {
const optional<std::vector<std::string>>& layerIDs,
const GeometryTileData&,
const CanonicalTileID&,
const style::Style&) const;
const style::Style&,
const CollisionTile*) const;

static optional<GeometryCoordinates> translateQueryGeometry(
const GeometryCoordinates& queryGeometry,
Expand All @@ -53,8 +54,6 @@ class FeatureIndex {

void addBucketLayerName(const std::string& bucketName, const std::string& layerName);

void setCollisionTile(std::unique_ptr<CollisionTile>);

private:
void addFeature(
std::unordered_map<std::string, std::vector<Feature>>& result,
Expand All @@ -67,7 +66,6 @@ class FeatureIndex {
const float bearing,
const float pixelsToTileUnits) const;

std::unique_ptr<CollisionTile> collisionTile;
GridIndex<IndexedSubfeature> grid;
unsigned int sortIndex = 0;

Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/text/collision_tile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ Box CollisionTile::getTreeBox(const Point<float>& anchor, const CollisionBox& bo
};
}

std::vector<IndexedSubfeature> CollisionTile::queryRenderedSymbols(const GeometryCoordinates& queryGeometry, float scale) {
std::vector<IndexedSubfeature> CollisionTile::queryRenderedSymbols(const GeometryCoordinates& queryGeometry, float scale) const {
std::vector<IndexedSubfeature> result;
if (queryGeometry.empty() || (tree.empty() && ignoredTree.empty())) {
return result;
Expand Down
2 changes: 1 addition & 1 deletion src/mbgl/text/collision_tile.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class CollisionTile {
float placeFeature(const CollisionFeature&, bool allowOverlap, bool avoidEdges);
void insertFeature(CollisionFeature&, float minPlacementScale, bool ignorePlacement);

std::vector<IndexedSubfeature> queryRenderedSymbols(const GeometryCoordinates&, float scale);
std::vector<IndexedSubfeature> queryRenderedSymbols(const GeometryCoordinates&, float scale) const;

const PlacementConfig config;

Expand Down
13 changes: 7 additions & 6 deletions src/mbgl/tile/geometry_tile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <mbgl/style/layer_impl.hpp>
#include <mbgl/style/layers/background_layer.hpp>
#include <mbgl/style/layers/custom_layer.hpp>
#include <mbgl/style/layers/symbol_layer.hpp>
#include <mbgl/style/style.hpp>
#include <mbgl/storage/file_source.hpp>
#include <mbgl/geometry/feature_index.hpp>
Expand Down Expand Up @@ -105,7 +106,7 @@ void GeometryTile::redoLayout() {

void GeometryTile::onLayout(LayoutResult result) {
availableData = DataAvailability::Some;
buckets = std::move(result.buckets);
nonSymbolBuckets = std::move(result.nonSymbolBuckets);
featureIndex = std::move(result.featureIndex);
data = std::move(result.tileData);
observer->onTileChanged(*this);
Expand All @@ -115,10 +116,8 @@ void GeometryTile::onPlacement(PlacementResult result) {
if (result.correlationID == correlationID) {
availableData = DataAvailability::All;
}
for (auto& bucket : result.buckets) {
buckets[bucket.first] = std::move(bucket.second);
}
featureIndex->setCollisionTile(std::move(result.collisionTile));
symbolBuckets = std::move(result.symbolBuckets);
collisionTile = std::move(result.collisionTile);
observer->onTileChanged(*this);
}

Expand All @@ -128,6 +127,7 @@ void GeometryTile::onError(std::exception_ptr err) {
}

Bucket* GeometryTile::getBucket(const Layer& layer) {
const auto& buckets = layer.is<SymbolLayer>() ? symbolBuckets : nonSymbolBuckets;
const auto it = buckets.find(layer.baseImpl->id);
if (it == buckets.end()) {
return nullptr;
Expand All @@ -153,7 +153,8 @@ void GeometryTile::queryRenderedFeatures(
layerIDs,
*data,
id.canonical,
style);
style,
collisionTile.get());
}

} // namespace mbgl
9 changes: 6 additions & 3 deletions src/mbgl/tile/geometry_tile.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class GeometryTile : public Tile {

class LayoutResult {
public:
std::unordered_map<std::string, std::shared_ptr<Bucket>> buckets;
std::unordered_map<std::string, std::shared_ptr<Bucket>> nonSymbolBuckets;
std::unique_ptr<FeatureIndex> featureIndex;
std::unique_ptr<GeometryTileData> tileData;
uint64_t correlationID;
Expand All @@ -59,7 +59,7 @@ class GeometryTile : public Tile {

class PlacementResult {
public:
std::unordered_map<std::string, std::shared_ptr<Bucket>> buckets;
std::unordered_map<std::string, std::shared_ptr<Bucket>> symbolBuckets;
std::unique_ptr<CollisionTile> collisionTile;
uint64_t correlationID;
};
Expand All @@ -80,9 +80,12 @@ class GeometryTile : public Tile {
uint64_t correlationID = 0;
optional<PlacementConfig> requestedConfig;

std::unordered_map<std::string, std::shared_ptr<Bucket>> buckets;
std::unordered_map<std::string, std::shared_ptr<Bucket>> nonSymbolBuckets;
std::unique_ptr<FeatureIndex> featureIndex;
std::unique_ptr<const GeometryTileData> data;

std::unordered_map<std::string, std::shared_ptr<Bucket>> symbolBuckets;
std::unique_ptr<CollisionTile> collisionTile;
};

} // namespace mbgl
4 changes: 3 additions & 1 deletion src/mbgl/tile/geometry_tile_worker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ void GeometryTileWorker::redoLayout() {
BucketParameters parameters { id, obsolete, *featureIndex, mode };

std::vector<std::vector<std::unique_ptr<Layer>>> groups = groupByLayout(std::move(*layers));
layers = nullopt;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reset the optional


for (auto& group : groups) {
if (obsolete) {
return;
Expand Down Expand Up @@ -280,7 +282,7 @@ bool GeometryTileWorker::hasPendingSymbolDependencies() const {
}

void GeometryTileWorker::attemptPlacement() {
if (!data || !layers || !placementConfig) {
if (!placementConfig) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need access to data and layers anymore, since all of that is encoded in the SymbolLayout objects now. This change is necessary, since disengaging the optional will make this check fail when it is part of redoLayout() (called at the very end), meaning that there won't be any symbols until placement is triggered again.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling at least one of these guards was needed to ensure that the body of attemptPlacement does not run if setPlacementConfig is called before setData and setLayers.

return;
}

Expand Down
35 changes: 35 additions & 0 deletions test/tile/vector_tile.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,14 @@
#include <mbgl/map/transform.hpp>
#include <mbgl/style/style.hpp>
#include <mbgl/style/update_parameters.hpp>
#include <mbgl/style/layers/symbol_layer.hpp>
#include <mbgl/renderer/symbol_bucket.hpp>
#include <mbgl/text/collision_tile.hpp>
#include <mbgl/geometry/feature_index.hpp>
#include <mbgl/annotation/annotation_manager.hpp>

#include <memory>

using namespace mbgl;

class VectorTileTest {
Expand Down Expand Up @@ -47,3 +53,32 @@ TEST(VectorTile, onError) {
tile.onError(std::make_exception_ptr(std::runtime_error("test")));
EXPECT_TRUE(tile.isRenderable());
}

TEST(VectorTile, Issue7615) {
VectorTileTest test;
VectorTile tile(OverscaledTileID(0, 0, 0), "source", test.updateParameters, test.tileset);

style::SymbolLayer symbolLayer("symbol", "source");
auto symbolBucket = std::make_shared<SymbolBucket>(
MapMode::Continuous, style::SymbolLayoutProperties::Evaluated(), false, false);

// Simulate placement of a symbol layer.
tile.onPlacement(GeometryTile::PlacementResult {
{{
symbolLayer.getID(),
symbolBucket
}},
nullptr,
0
});

// Subsequent onLayout should not cause the existing symbol bucket to be discarded.
tile.onLayout(GeometryTile::LayoutResult {
{},
nullptr,
nullptr,
0
});

EXPECT_EQ(symbolBucket.get(), tile.getBucket(symbolLayer));
}