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
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/mbgl/layout/symbol_instance.hpp
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
45 changes: 35 additions & 10 deletions src/mbgl/layout/symbol_layout.cpp
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 @@ -572,23 +572,47 @@ void SymbolLayout::addFeature(const std::size_t layoutFeatureIndex,
}
}

auto addSymbolInstance = [&] (Anchor& anchor, std::shared_ptr<SymbolInstanceSharedData> sharedData) {
auto addSymbolInstance = [&](Anchor& anchor, std::shared_ptr<SymbolInstanceSharedData> sharedData) {
assert(sharedData);
const bool anchorInsideTile = anchor.point.x >= 0 && anchor.point.x < util::EXTENT && anchor.point.y >= 0 && anchor.point.y < util::EXTENT;
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:
// (1) render symbols that overlap into this tile
// (2) approximate collision detection effects from neighboring symbols
symbolInstances.emplace_back(anchor, std::move(sharedData), shapedTextOrientations,
shapedIcon, verticallyShapedIcon,
textBoxScale, textPadding, textPlacement, textOffset,
iconBoxScale, iconPadding, iconOffset, indexedFeature,
layoutFeatureIndex, feature.index,
feature.formattedText ? feature.formattedText->rawText() : std::u16string(),
overscaling, iconRotation, textRotation, variableTextOffset, allowVerticalPlacement, iconType);
symbolInstances.emplace_back(anchor,
std::move(sharedData),
shapedTextOrientations,
shapedIcon,
verticallyShapedIcon,
textBoxScale,
textPadding,
textPlacement,
textOffset,
iconBoxScale,
iconPadding,
iconOffset,
indexedFeature,
layoutFeatureIndex,
feature.index,
feature.formattedText ? feature.formattedText->rawText() : std::u16string(),
overscaling,
iconRotation,
textRotation,
variableTextOffset,
allowVerticalPlacement,
iconType);
}
};

Expand Down Expand Up @@ -728,6 +752,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
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
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
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
29 changes: 28 additions & 1 deletion src/mbgl/renderer/layers/render_symbol_layer.cpp
Expand Up @@ -424,6 +424,8 @@ void RenderSymbolLayer::render(PaintParameters& parameters) {
assert(bucket.paintProperties.find(getID()) != bucket.paintProperties.end());
const auto& bucketPaintProperties = bucket.paintProperties.at(getID());

bucket.justReloaded = false;

auto addRenderables = [&renderableSegments, &tile, renderData, &bucketPaintProperties, it = renderableSegments.begin()] (auto& segments, const SymbolType type) mutable {
for (auto& segment : segments) {
it = renderableSegments.emplace_hint(it, SegmentWrapper{std::ref(segment)}, tile, *renderData, bucketPaintProperties, segment.sortKey, type);
Expand Down Expand Up @@ -580,7 +582,32 @@ void RenderSymbolLayer::prepare(const LayerPrepareParameters& params) {
const Tile* tile = params.source->getRenderedTile(renderTile.id);
assert(tile);
assert(tile->kind == Tile::Kind::Geometry);
placementData.push_back({*bucket, renderTile, static_cast<const GeometryTile*>(tile)->getFeatureIndex()});

bool firstInBucket = true;

if (bucket->sortKeyRanges.size() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: bucket->sortKeyRanges.empty()

placementData.push_back({*bucket,
renderTile,
static_cast<const GeometryTile*>(tile)->getFeatureIndex(),
firstInBucket,
0.0f,
0,
bucket->symbolInstances.size()});
} else {
for (const SortKeyRange& sortKeyRange : bucket->sortKeyRanges) {
LayerPlacementData layerData{*bucket,
renderTile,
static_cast<const GeometryTile*>(tile)->getFeatureIndex(),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might make sense to cache static_cast<const GeometryTile*>(tile)->getFeatureIndex() in a local variable

firstInBucket,
sortKeyRange.sortKey,
sortKeyRange.symbolInstanceStart,
sortKeyRange.symbolInstanceEnd};
auto sortPosition = std::upper_bound(placementData.cbegin(), placementData.cend(), layerData);
placementData.insert(sortPosition, std::move(layerData));

firstInBucket = false;
}
}
}
}
}
Expand Down
16 changes: 11 additions & 5 deletions src/mbgl/renderer/render_layer.hpp
@@ -1,11 +1,11 @@
#pragma once
#include <list>
#include <mbgl/layout/layout.hpp>
#include <mbgl/renderer/render_pass.hpp>
#include <mbgl/renderer/render_source.hpp>
#include <mbgl/style/layer_properties.hpp>
#include <mbgl/tile/geometry_tile_data.hpp>
#include <mbgl/util/mat4.hpp>

#include <memory>
#include <string>

Expand All @@ -20,6 +20,7 @@ class RenderTile;
class TransformState;
class PatternAtlas;
class LineAtlas;
class SymbolBucket;

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

class LayerPlacementData {
public:
friend bool operator<(const LayerPlacementData& lhs, const LayerPlacementData& rhs) {
return lhs.sortKey < rhs.sortKey;
}
std::reference_wrapper<Bucket> bucket;
std::reference_wrapper<const RenderTile> tile;
std::shared_ptr<FeatureIndex> featureIndex;
bool firstInBucket;
float sortKey;
size_t symbolInstanceStart;
size_t symbolInstanceEnd;
};

class LayerPrepareParameters {
Expand Down Expand Up @@ -95,9 +103,7 @@ class RenderLayer {

virtual void prepare(const LayerPrepareParameters&);

const std::vector<LayerPlacementData>& getPlacementData() const {
return placementData;
}
const std::list<LayerPlacementData>& getPlacementData() const { return placementData; }

// Latest evaluated properties.
Immutable<style::LayerProperties> evaluatedProperties;
Expand Down Expand Up @@ -126,7 +132,7 @@ class RenderLayer {
// evaluated StyleProperties object and is updated accordingly.
RenderPass passes = RenderPass::None;

std::vector<LayerPlacementData> placementData;
std::list<LayerPlacementData> placementData;

private:
// Some layers may not render correctly on some hardware when the vertex attribute limit of
Expand Down
26 changes: 16 additions & 10 deletions src/mbgl/text/placement.cpp
@@ -1,11 +1,11 @@
#include <mbgl/text/placement.hpp>

#include <list>
#include <mbgl/layout/symbol_layout.hpp>
#include <mbgl/renderer/bucket.hpp>
#include <mbgl/renderer/buckets/symbol_bucket.hpp>
#include <mbgl/renderer/render_layer.hpp>
#include <mbgl/renderer/render_tile.hpp>
#include <mbgl/renderer/update_parameters.hpp>
#include <mbgl/text/placement.hpp>
#include <mbgl/tile/geometry_tile.hpp>
#include <mbgl/util/math.hpp>
#include <utility>
Expand Down Expand Up @@ -113,7 +113,12 @@ void Placement::placeLayer(const RenderLayer& layer) {
std::set<uint32_t> seenCrossTileIDs;
for (const auto& item : layer.getPlacementData()) {
Bucket& bucket = item.bucket;
BucketPlacementParameters params{item.tile, layer.baseImpl->source, item.featureIndex};
BucketPlacementParameters params{item.tile,
layer.baseImpl->source,
item.featureIndex,
item.sortKey,
item.symbolInstanceStart,
item.symbolInstanceEnd};
bucket.place(*this, params, seenCrossTileIDs);
}
}
Expand Down Expand Up @@ -656,18 +661,17 @@ 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);
}
}

bucket.justReloaded = false;

// As long as this placement lives, we have to hold onto this bucket's
// matching FeatureIndex/data for querying purposes
retainedQueryData.emplace(std::piecewise_construct,
std::forward_as_tuple(bucket.bucketInstanceId),
std::forward_as_tuple(bucket.bucketInstanceId, params.featureIndex, overscaledID));
std::forward_as_tuple(bucket.bucketInstanceId),
std::forward_as_tuple(bucket.bucketInstanceId, params.featureIndex, overscaledID));
}

void Placement::commit() {
Expand Down Expand Up @@ -737,7 +741,9 @@ void Placement::commit() {
void Placement::updateLayerBuckets(const RenderLayer& layer, const TransformState& state, bool updateOpacities) const {
std::set<uint32_t> seenCrossTileIDs;
for (const auto& item : layer.getPlacementData()) {
item.bucket.get().updateVertices(*this, updateOpacities, state, item.tile, seenCrossTileIDs);
if (item.firstInBucket) {
item.bucket.get().updateVertices(*this, updateOpacities, state, item.tile, seenCrossTileIDs);
}
}
}

Expand Down
3 changes: 3 additions & 0 deletions src/mbgl/text/placement.hpp
Expand Up @@ -91,6 +91,9 @@ class BucketPlacementParameters {
const RenderTile& tile;
std::string sourceId;
std::shared_ptr<FeatureIndex> featureIndex;
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
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