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

[core] sort symbols using symbol-sort-key before placement #16023

Merged
merged 12 commits into from
Feb 11, 2020
Merged

[core] sort symbols using symbol-sort-key before placement #16023

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a95c480
sort symbols using symbol-sort-key before placement
ansis Dec 6, 2019
82ceec3
clang-format
ansis Dec 6, 2019
7f73d24
fixup
ansis Dec 6, 2019
410a2b4
update based on feedback
ansis Feb 10, 2020
143a2dd
fixup
ansis Feb 10, 2020
f3e8670
fixup
ansis Feb 10, 2020
a39935d
fixup
ansis Feb 10, 2020
3e00584
format
ansis Feb 10, 2020
4a267d2
fixup
ansis Feb 10, 2020
6c57da1
fixup
ansis Feb 10, 2020
5259e5c
fix based on feedback
ansis Feb 11, 2020
001113c
format
ansis Feb 11, 2020
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
Next Next commit
sort symbols using symbol-sort-key before placement
  • Loading branch information
@ansis
ansis committed Feb 10, 2020
commit a95c4802702a777f1ce6e65536c289cda299ed1f
7 changes: 7 additions & 0 deletions src/mbgl/layout/symbol_instance.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,11 @@ class SymbolInstance {
static constexpr uint32_t invalidCrossTileID() { return std::numeric_limits<uint32_t>::max(); }
};

class SortKeyRange {
public:
float sortKey;
size_t symbolInstanceStart;
size_t symbolInstanceEnd;
};

} // namespace mbgl
11 changes: 10 additions & 1 deletion src/mbgl/layout/symbol_layout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ SymbolLayout::SymbolLayout(const BucketParameters& parameters,

const bool hasSymbolSortKey = !leader.layout.get<SymbolSortKey>().isUndefined();
const auto symbolZOrder = layout->get<SymbolZOrder>();
const bool sortFeaturesByKey = symbolZOrder != SymbolZOrderType::ViewportY && hasSymbolSortKey;
sortFeaturesByKey = symbolZOrder != SymbolZOrderType::ViewportY && hasSymbolSortKey;
const bool zOrderByViewportY = symbolZOrder == SymbolZOrderType::ViewportY || (symbolZOrder == SymbolZOrderType::Auto && !sortFeaturesByKey);
sortFeaturesByY = zOrderByViewportY && (layout->get<TextAllowOverlap>() || layout->get<IconAllowOverlap>() ||
layout->get<TextIgnorePlacement>() || layout->get<IconIgnorePlacement>());
Expand Down Expand Up @@ -577,6 +577,14 @@ void SymbolLayout::addFeature(const std::size_t layoutFeatureIndex,
const bool anchorInsideTile = anchor.point.x >= 0 && anchor.point.x < util::EXTENT && anchor.point.y >= 0 && anchor.point.y < util::EXTENT;

if (mode == MapMode::Tile || anchorInsideTile) {
if (sortFeaturesByKey) {
if (sortKeyRanges.size() && sortKeyRanges.back().sortKey == feature.sortKey) {
sortKeyRanges.back().symbolInstanceEnd = symbolInstances.size() + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

sortKeyRanges.back().symbolInstanceEnd = symbolInstances.size(); shall be enough, no? begin() + size = end() 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like I'm setting it before adding the new symbol instance. I guess moving it after and removing the + 1 would be cleaner

} else {
sortKeyRanges.push_back({ feature.sortKey, symbolInstances.size(), symbolInstances.size() + 1 });
}
}

// For static/continuous rendering, only add symbols anchored within this tile:
// neighboring symbols will be added as part of the neighboring tiles.
// In tiled rendering mode, add all symbols in the buffers so that we can:
Expand Down Expand Up @@ -728,6 +736,7 @@ void SymbolLayout::createBucket(const ImagePositions&, std::unique_ptr<FeatureIn
sortFeaturesByY,
bucketLeaderID,
std::move(symbolInstances),
std::move(sortKeyRanges),
tilePixelRatio,
allowVerticalPlacement,
std::move(placementModes),
Expand Down
2 changes: 2 additions & 0 deletions src/mbgl/layout/symbol_layout.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class SymbolLayout final : public Layout {

const std::string bucketLeaderID;
std::vector<SymbolInstance> symbolInstances;
std::vector<SortKeyRange> sortKeyRanges;

static constexpr float INVALID_OFFSET_VALUE = std::numeric_limits<float>::max();
/**
Expand Down Expand Up @@ -115,6 +116,7 @@ class SymbolLayout final : public Layout {

bool iconsNeedLinear = false;
bool sortFeaturesByY = false;
bool sortFeaturesByKey = false;
bool allowVerticalPlacement = false;
bool iconsInText = false;
std::vector<style::TextWritingModeType> placementModes;
Expand Down
2 changes: 2 additions & 0 deletions src/mbgl/renderer/buckets/symbol_bucket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ SymbolBucket::SymbolBucket(Immutable<style::SymbolLayoutProperties::PossiblyEval
bool sortFeaturesByY_,
const std::string bucketName_,
const std::vector<SymbolInstance>&& symbolInstances_,
const std::vector<SortKeyRange>&& sortKeyRanges_,
float tilePixelRatio_,
bool allowVerticalPlacement_,
std::vector<style::TextWritingModeType> placementModes_,
Expand All @@ -40,6 +41,7 @@ SymbolBucket::SymbolBucket(Immutable<style::SymbolLayoutProperties::PossiblyEval
hasVariablePlacement(false),
hasUninitializedSymbols(false),
symbolInstances(symbolInstances_),
sortKeyRanges(sortKeyRanges_),
textSizeBinder(SymbolSizeBinder::create(zoom, textSize, TextSize::defaultValue())),
iconSizeBinder(SymbolSizeBinder::create(zoom, iconSize, IconSize::defaultValue())),
tilePixelRatio(tilePixelRatio_),
Expand Down
2 changes: 2 additions & 0 deletions src/mbgl/renderer/buckets/symbol_bucket.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class SymbolBucket final : public Bucket {
bool sortFeaturesByY,
const std::string bucketLeaderID,
const std::vector<SymbolInstance>&&,
const std::vector<SortKeyRange>&&,
const float tilePixelRatio,
bool allowVerticalPlacement,
std::vector<style::TextWritingModeType> placementModes,
Expand Down Expand Up @@ -117,6 +118,7 @@ class SymbolBucket final : public Bucket {
bool hasUninitializedSymbols : 1;

std::vector<SymbolInstance> symbolInstances;
std::vector<SortKeyRange> sortKeyRanges;

struct PaintProperties {
SymbolIconProgram::Binders iconBinders;
Expand Down
3 changes: 2 additions & 1 deletion src/mbgl/renderer/render_layer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ class RenderTile;
class TransformState;
class PatternAtlas;
class LineAtlas;
class SymbolBucket;

class LayerRenderData {
public:
Expand All @@ -29,7 +30,7 @@ class LayerRenderData {

class LayerPlacementData {
public:
std::reference_wrapper<Bucket> bucket;
std::reference_wrapper<SymbolBucket> bucket;
Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to keep generic interface here, otherwise LTO will not be able to reduce SymbolLayer-related symbols from the binary if the symbol layer is disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, thanks! I switched back to Bucket

std::reference_wrapper<const RenderTile> tile;
std::shared_ptr<FeatureIndex> featureIndex;
};
Expand Down
46 changes: 42 additions & 4 deletions src/mbgl/text/placement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <mbgl/tile/geometry_tile.hpp>
#include <mbgl/util/math.hpp>
#include <utility>
#include <list>

namespace mbgl {

Expand Down Expand Up @@ -111,9 +112,45 @@ Placement::Placement() : collisionIndex({}, MapMode::Static), collisionGroups(tr

void Placement::placeLayer(const RenderLayer& layer) {
std::set<uint32_t> seenCrossTileIDs;
std::list<BucketPlacementParameters> parameters;

for (const auto& item : layer.getPlacementData()) {
Bucket& bucket = item.bucket;
BucketPlacementParameters params{item.tile, layer.baseImpl->source, item.featureIndex};
SymbolBucket& bucket = item.bucket;
if (bucket.sortKeyRanges.size() == 0) {
BucketPlacementParameters params{
item.bucket,
item.tile,
projMatrix,
layer.baseImpl->source,
item.featureIndex,
showCollisionBoxes,
0.0f,
0,
bucket.symbolInstances.size()};
parameters.push_back(params);

} else {
for (const SortKeyRange& sortKeyRange : bucket.sortKeyRanges) {
BucketPlacementParameters params{
item.bucket,
item.tile,
projMatrix,
layer.baseImpl->source,
item.featureIndex,
showCollisionBoxes,
sortKeyRange.sortKey,
sortKeyRange.symbolInstanceStart,
sortKeyRange.symbolInstanceEnd};

auto sortPosition = std::upper_bound(parameters.cbegin(), parameters.cend(), params);
parameters.insert(sortPosition, std::move(params));
}
}
}


for (auto& params : parameters) {
SymbolBucket& bucket = params.bucket;
bucket.place(*this, params, seenCrossTileIDs);
}
}
Expand Down Expand Up @@ -656,8 +693,9 @@ void Placement::placeBucket(const SymbolBucket& bucket,
}

} else {
for (const SymbolInstance& symbol : bucket.symbolInstances) {
placeSymbol(symbol);
auto endIt = bucket.symbolInstances.begin() + params.symbolInstanceEnd;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we check that endIt <= bucket.symbolInstances.end()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, would an assert be what you're looking for?

Copy link
Contributor

Choose a reason for hiding this comment

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

assert is fine (as long as there are no possible code paths that could violate it 😄 )

for (auto it = bucket.symbolInstances.begin() + params.symbolInstanceStart; it != endIt; it++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

styling nit: better have auto beginIt = bucket.symbolInstances.begin() + params.symbolInstanceStar

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ++it

placeSymbol(*it);
}
}

Expand Down
6 changes: 6 additions & 0 deletions src/mbgl/text/placement.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,15 @@ class CollisionGroups {

class BucketPlacementParameters {
public:
friend bool operator<(const BucketPlacementParameters& lhs, const BucketPlacementParameters& rhs) { return lhs.sortKey < rhs.sortKey; }
SymbolBucket& bucket;
Copy link
Contributor

Choose a reason for hiding this comment

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

BucketPlacementParameters shall not know about SymbolBucket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be ok for BucketPlacementParameters to have a reference to Bucket?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead just pre-sort LayerPlacementData in RenderSymbolLayer::prepare() accordingly to the the symbol bucket sort key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, changed to your suggestion. This means that there are now possibly multiple entries into LayerPlacementData per tile which we have to account for elsewhere

const RenderTile& tile;
std::string sourceId;
std::shared_ptr<FeatureIndex> featureIndex;
bool showCollisionBoxes;
float sortKey;
size_t symbolInstanceStart;
size_t symbolInstanceEnd;
Copy link
Contributor

Choose a reason for hiding this comment

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

could the newly added fields be SymbolBucket properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These properties come from SortKeyRange. A bucket can have many SortKeyRanges

};

class Placement;
Expand Down
2 changes: 2 additions & 0 deletions test/gl/bucket.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ TEST(Buckets, SymbolBucket) {
bool sortFeaturesByY = false;
std::string bucketLeaderID = "test";
std::vector<SymbolInstance> symbolInstances;
std::vector<SortKeyRange> symbolRanges;

gl::Context context{ backend };
SymbolBucket bucket{std::move(layout),
Expand All @@ -134,6 +135,7 @@ TEST(Buckets, SymbolBucket) {
sortFeaturesByY,
bucketLeaderID,
std::move(symbolInstances),
std::move(symbolRanges),
1.0f,
false,
{},
Expand Down
20 changes: 20 additions & 0 deletions test/text/cross_tile_symbol_index.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ TEST(CrossTileSymbolLayerIndex, addBucket) {

OverscaledTileID mainID(6, 0, 6, 8, 8);
std::vector<SymbolInstance> mainInstances;
std::vector<SortKeyRange> mainRanges;
mainInstances.push_back(makeSymbolInstance(1000, 1000, u"Detroit"));
mainInstances.push_back(makeSymbolInstance(2000, 2000, u"Toronto"));
SymbolBucket mainBucket{layout,
Expand All @@ -58,6 +59,7 @@ TEST(CrossTileSymbolLayerIndex, addBucket) {
sortFeaturesByY,
bucketLeaderID,
std::move(mainInstances),
std::move(mainRanges),
1.0f,
false,
{},
Expand All @@ -72,6 +74,7 @@ TEST(CrossTileSymbolLayerIndex, addBucket) {

OverscaledTileID childID(7, 0, 7, 16, 16);
std::vector<SymbolInstance> childInstances;
std::vector<SortKeyRange> childRanges;
childInstances.push_back(makeSymbolInstance(2000, 2000, u"Detroit"));
childInstances.push_back(makeSymbolInstance(2000, 2000, u"Windsor"));
childInstances.push_back(makeSymbolInstance(3000, 3000, u"Toronto"));
Expand All @@ -85,6 +88,7 @@ TEST(CrossTileSymbolLayerIndex, addBucket) {
sortFeaturesByY,
bucketLeaderID,
std::move(childInstances),
std::move(childRanges),
1.0f,
false,
{},
Expand All @@ -103,6 +107,7 @@ TEST(CrossTileSymbolLayerIndex, addBucket) {

OverscaledTileID parentID(5, 0, 5, 4, 4);
std::vector<SymbolInstance> parentInstances;
std::vector<SortKeyRange> parentRanges;
parentInstances.push_back(makeSymbolInstance(500, 500, u"Detroit"));
SymbolBucket parentBucket{layout,
{},
Expand All @@ -113,6 +118,7 @@ TEST(CrossTileSymbolLayerIndex, addBucket) {
sortFeaturesByY,
bucketLeaderID,
std::move(parentInstances),
std::move(parentRanges),
1.0f,
false,
{},
Expand All @@ -130,6 +136,7 @@ TEST(CrossTileSymbolLayerIndex, addBucket) {
// grandchild
OverscaledTileID grandchildID(8, 0, 8, 32, 32);
std::vector<SymbolInstance> grandchildInstances;
std::vector<SortKeyRange> grandchildRanges;
grandchildInstances.push_back(makeSymbolInstance(4000, 4000, u"Detroit"));
grandchildInstances.push_back(makeSymbolInstance(4000, 4000, u"Windsor"));
SymbolBucket grandchildBucket{layout,
Expand All @@ -141,6 +148,7 @@ TEST(CrossTileSymbolLayerIndex, addBucket) {
sortFeaturesByY,
bucketLeaderID,
std::move(grandchildInstances),
std::move(grandchildRanges),
1.0f,
false,
{},
Expand Down Expand Up @@ -169,6 +177,7 @@ TEST(CrossTileSymbolLayerIndex, resetIDs) {

OverscaledTileID mainID(6, 0, 6, 8, 8);
std::vector<SymbolInstance> mainInstances;
std::vector<SortKeyRange> mainRanges;
mainInstances.push_back(makeSymbolInstance(1000, 1000, u"Detroit"));
SymbolBucket mainBucket{layout,
{},
Expand All @@ -179,6 +188,7 @@ TEST(CrossTileSymbolLayerIndex, resetIDs) {
sortFeaturesByY,
bucketLeaderID,
std::move(mainInstances),
std::move(mainRanges),
1.0f,
false,
{},
Expand All @@ -187,6 +197,7 @@ TEST(CrossTileSymbolLayerIndex, resetIDs) {

OverscaledTileID childID(7, 0, 7, 16, 16);
std::vector<SymbolInstance> childInstances;
std::vector<SortKeyRange> childRanges;
childInstances.push_back(makeSymbolInstance(2000, 2000, u"Detroit"));
SymbolBucket childBucket{layout,
{},
Expand All @@ -197,6 +208,7 @@ TEST(CrossTileSymbolLayerIndex, resetIDs) {
sortFeaturesByY,
bucketLeaderID,
std::move(childInstances),
std::move(childRanges),
1.0f,
false,
{},
Expand Down Expand Up @@ -233,6 +245,7 @@ TEST(CrossTileSymbolLayerIndex, noDuplicatesWithinZoomLevel) {

OverscaledTileID mainID(6, 0, 6, 8, 8);
std::vector<SymbolInstance> mainInstances;
std::vector<SortKeyRange> mainRanges;
mainInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // A
mainInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // B
SymbolBucket mainBucket{layout,
Expand All @@ -244,6 +257,7 @@ TEST(CrossTileSymbolLayerIndex, noDuplicatesWithinZoomLevel) {
sortFeaturesByY,
bucketLeaderID,
std::move(mainInstances),
std::move(mainRanges),
1.0f,
false,
{},
Expand All @@ -252,6 +266,7 @@ TEST(CrossTileSymbolLayerIndex, noDuplicatesWithinZoomLevel) {

OverscaledTileID childID(7, 0, 7, 16, 16);
std::vector<SymbolInstance> childInstances;
std::vector<SortKeyRange> childRanges;
childInstances.push_back(makeSymbolInstance(2000, 2000, u"")); // A'
childInstances.push_back(makeSymbolInstance(2000, 2000, u"")); // B'
childInstances.push_back(makeSymbolInstance(2000, 2000, u"")); // C'
Expand All @@ -264,6 +279,7 @@ TEST(CrossTileSymbolLayerIndex, noDuplicatesWithinZoomLevel) {
sortFeaturesByY,
bucketLeaderID,
std::move(childInstances),
std::move(childRanges),
1.0f,
false,
{},
Expand Down Expand Up @@ -295,6 +311,7 @@ TEST(CrossTileSymbolLayerIndex, bucketReplacement) {

OverscaledTileID tileID(6, 0, 6, 8, 8);
std::vector<SymbolInstance> firstInstances;
std::vector<SortKeyRange> firstRanges;
firstInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // A
firstInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // B
SymbolBucket firstBucket{layout,
Expand All @@ -306,13 +323,15 @@ TEST(CrossTileSymbolLayerIndex, bucketReplacement) {
sortFeaturesByY,
bucketLeaderID,
std::move(firstInstances),
std::move(firstRanges),
1.0f,
false,
{},
false /*iconsInText*/};
firstBucket.bucketInstanceId = ++maxBucketInstanceId;

std::vector<SymbolInstance> secondInstances;
std::vector<SortKeyRange> secondRanges;
secondInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // A'
secondInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // B'
secondInstances.push_back(makeSymbolInstance(1000, 1000, u"")); // C'
Expand All @@ -325,6 +344,7 @@ TEST(CrossTileSymbolLayerIndex, bucketReplacement) {
sortFeaturesByY,
bucketLeaderID,
std::move(secondInstances),
std::move(secondRanges),
1.0f,
false,
{},
Expand Down