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

[Core] Ensure queryRenderedFeatures accounts for icons with *-offset … #13105

Merged
merged 5 commits into from
Oct 18, 2018
Merged

[Core] Ensure queryRenderedFeatures accounts for icons with *-offset … #13105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
10e9fd2
[Core] Ensure queryRenderedFeatures accounts for icons with *-offset …
Oct 16, 2018
f0cabe3
Update submodule
Oct 17, 2018
a4a07f3
Add comments
Oct 17, 2018
148a1b1
Fix cross_tile_symbol_index test
Oct 17, 2018
34ffc78
Add staging changelog
Oct 18, 2018
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
1 change: 0 additions & 1 deletion platform/node/test/ignores.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@
"render-tests/fill-pattern/update-feature-state": "skip - port https://github.com/mapbox/mapbox-gl-js/pull/6263 - needs issue",
"render-tests/geojson/inline-linestring-fill": "current behavior is arbitrary",
"render-tests/geojson/inline-polygon-symbol": "behavior needs reconciliation with gl-js",
"render-tests/icon-rotate/with-offset": "https://github.com/mapbox/mapbox-gl-native/issues/11872",
"render-tests/mixed-zoom/z10-z11": "https://github.com/mapbox/mapbox-gl-native/issues/10397",
"render-tests/raster-masking/overlapping-zoom": "https://github.com/mapbox/mapbox-gl-native/issues/10195",
"render-tests/real-world/bangkok": "https://github.com/mapbox/mapbox-gl-native/issues/10412",
Expand Down
4 changes: 4 additions & 0 deletions scripts/changelog_staging/queryrenderedfeatures-with-icon-rotate.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"core": "Fix bug in which using `icon-rotate` and `icon-offset` together prevented `queryRenderedFeatures` from returning the symbol",
"issue": 12909
}
7 changes: 4 additions & 3 deletions src/mbgl/layout/symbol_instance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,16 @@ SymbolInstance::SymbolInstance(Anchor& anchor_,
const std::size_t layoutFeatureIndex_,
const std::size_t dataFeatureIndex_,
const std::u16string& key_,
const float overscaling) :
const float overscaling,
const float rotate) :
anchor(anchor_),
line(line_),
hasText(false),
hasIcon(shapedIcon),

// Create the collision features that will be used to check whether this symbol instance can be placed
textCollisionFeature(line_, anchor, shapedTextOrientations.first, textBoxScale, textPadding, textPlacement, indexedFeature, overscaling),
iconCollisionFeature(line_, anchor, shapedIcon, iconBoxScale, iconPadding, indexedFeature),
textCollisionFeature(line_, anchor, shapedTextOrientations.first, textBoxScale, textPadding, textPlacement, indexedFeature, overscaling, rotate),
iconCollisionFeature(line_, anchor, shapedIcon, iconBoxScale, iconPadding, indexedFeature, rotate),
layoutFeatureIndex(layoutFeatureIndex_),
dataFeatureIndex(dataFeatureIndex_),
textOffset(textOffset_),
Expand Down
3 changes: 2 additions & 1 deletion src/mbgl/layout/symbol_instance.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ class SymbolInstance {
const std::size_t layoutFeatureIndex,
const std::size_t dataFeatureIndex,
const std::u16string& key,
const float overscaling);
const float overscaling,
const float rotate);

Anchor anchor;
GeometryCoordinates line;
Expand Down
39 changes: 20 additions & 19 deletions src/mbgl/layout/symbol_layout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ SymbolLayout::SymbolLayout(const BucketParameters& parameters,
auto feature = sourceLayer->getFeature(i);
if (!leader.filter(expression::EvaluationContext { this->zoom, feature.get() }))
continue;

SymbolFeature ft(std::move(feature));

ft.index = i;
Expand All @@ -112,7 +112,7 @@ SymbolLayout::SymbolLayout(const BucketParameters& parameters,
auto textTransform = layout.evaluate<TextTransform>(zoom, ft);
FontStack baseFontStack = layout.evaluate<TextFont>(zoom, ft);
FontStackHash baseFontStackHash = FontStackHasher()(baseFontStack);

ft.formattedText = TaggedString();
for (std::size_t j = 0; j < formatted.sections.size(); j++) {
const auto& section = formatted.sections[j];
Expand All @@ -122,7 +122,7 @@ SymbolLayout::SymbolLayout(const BucketParameters& parameters,
} else if (textTransform == TextTransformType::Lowercase) {
u8string = platform::lowercase(u8string);
}

ft.formattedText->addSection(applyArabicShaping(util::utf8_to_utf16::convert(u8string)),
section.fontScale ? *section.fontScale : 1.0,
section.fontStack ? FontStackHasher()(*section.fontStack) : baseFontStackHash);
Expand All @@ -133,7 +133,7 @@ SymbolLayout::SymbolLayout(const BucketParameters& parameters,
const bool canVerticalizeText = layout.get<TextRotationAlignment>() == AlignmentType::Map
&& layout.get<SymbolPlacement>() != SymbolPlacementType::Point
&& util::i18n::allowsVerticalWritingMode(ft.formattedText->rawText());

// Loop through all characters of this text and collect unique codepoints.
for (std::size_t j = 0; j < ft.formattedText->length(); j++) {
const auto& sectionFontStack = formatted.sections[ft.formattedText->getSectionIndex(j)].fontStack;
Expand Down Expand Up @@ -236,7 +236,7 @@ void SymbolLayout::prepareSymbols(const GlyphMap& glyphMap, const GlyphPositions
if (shapedTextOrientations.first || shapedIcon) {
addFeature(std::distance(features.begin(), it), feature, shapedTextOrientations, shapedIcon, glyphPositions);
}

feature.geometry.clear();
}

Expand All @@ -250,18 +250,18 @@ void SymbolLayout::addFeature(const std::size_t layoutFeatureIndex,
const GlyphPositions& glyphPositions) {
const float minScale = 0.5f;
const float glyphSize = 24.0f;

const float layoutTextSize = layout.evaluate<TextSize>(zoom + 1, feature);
const float layoutIconSize = layout.evaluate<IconSize>(zoom + 1, feature);
const std::array<float, 2> textOffset = layout.evaluate<TextOffset>(zoom, feature);
const std::array<float, 2> iconOffset = layout.evaluate<IconOffset>(zoom, feature);

// To reduce the number of labels that jump around when zooming we need
// to use a text-size value that is the same for all zoom levels.
// This calculates text-size at a high zoom level so that all tiles can
// use the same value when calculating anchor positions.
const float textMaxSize = layout.evaluate<TextSize>(18, feature);

const float fontScale = layoutTextSize / glyphSize;
const float textBoxScale = tilePixelRatio * fontScale;
const float textMaxBoxScale = tilePixelRatio * textMaxSize / glyphSize;
Expand All @@ -270,6 +270,7 @@ void SymbolLayout::addFeature(const std::size_t layoutFeatureIndex,
const float textPadding = layout.get<TextPadding>() * tilePixelRatio;
const float iconPadding = layout.get<IconPadding>() * tilePixelRatio;
const float textMaxAngle = layout.get<TextMaxAngle>() * util::DEG2RAD;
const float rotation = layout.evaluate<IconRotate>(zoom, feature);
const SymbolPlacementType textPlacement = layout.get<TextRotationAlignment>() != AlignmentType::Map
? SymbolPlacementType::Point
: layout.get<SymbolPlacement>();
Expand All @@ -291,10 +292,10 @@ void SymbolLayout::addFeature(const std::size_t layoutFeatureIndex,
textBoxScale, textPadding, textPlacement, textOffset,
iconBoxScale, iconPadding, iconOffset,
glyphPositions, indexedFeature, layoutFeatureIndex, feature.index,
feature.formattedText ? feature.formattedText->rawText() : std::u16string(), overscaling);
feature.formattedText ? feature.formattedText->rawText() : std::u16string(), overscaling, rotation);
}
};

const auto& type = feature.getType();

if (layout.get<SymbolPlacement>() == SymbolPlacementType::Line) {
Expand Down Expand Up @@ -408,7 +409,7 @@ void SymbolLayout::createBucket(const ImagePositions&, std::unique_ptr<FeatureIn
const bool zOrderByViewport = layout.get<SymbolZOrder>() == SymbolZOrderType::ViewportY;
const bool sortFeaturesByY = zOrderByViewport && (layout.get<TextAllowOverlap>() || layout.get<IconAllowOverlap>() ||
layout.get<TextIgnorePlacement>() || layout.get<IconIgnorePlacement>());

auto bucket = std::make_shared<SymbolBucket>(layout, layerPaintProperties, textSize, iconSize, zoom, sdfIcons, iconsNeedLinear, sortFeaturesByY, bucketLeaderID, std::move(symbolInstances));

for (SymbolInstance &symbolInstance : bucket->symbolInstances) {
Expand Down Expand Up @@ -437,20 +438,20 @@ void SymbolLayout::createBucket(const ImagePositions&, std::unique_ptr<FeatureIn
firstHorizontal = false;
}
}

if (symbolInstance.writingModes & WritingModeType::Vertical) {
bucket->text.placedSymbols.emplace_back(symbolInstance.anchor.point, symbolInstance.anchor.segment, sizeData.min, sizeData.max,
symbolInstance.textOffset, WritingModeType::Vertical, symbolInstance.line, CalculateTileDistances(symbolInstance.line, symbolInstance.anchor));
symbolInstance.placedVerticalTextIndex = bucket->text.placedSymbols.size() - 1;

PlacedSymbol& verticalSymbol = bucket->text.placedSymbols.back();
bool firstVertical = true;

for (const auto& symbol : symbolInstance.verticalGlyphQuads) {
size_t index = addSymbol(
bucket->text, sizeData, symbol,
symbolInstance.anchor, verticalSymbol);

if (firstVertical) {
verticalSymbol.vertexStartIndex = index;
firstVertical = false;
Expand All @@ -470,7 +471,7 @@ void SymbolLayout::createBucket(const ImagePositions&, std::unique_ptr<FeatureIn
symbolInstance.anchor, iconSymbol);
}
}

for (auto& pair : bucket->paintPropertyBinders) {
pair.second.first.populateVertexVectors(feature, bucket->icon.vertices.vertexSize(), {}, {});
pair.second.second.populateVertexVectors(feature, bucket->text.vertices.vertexSize(), {}, {});
Expand Down Expand Up @@ -520,15 +521,15 @@ size_t SymbolLayout::addSymbol(Buffer& buffer,
buffer.vertices.emplace_back(SymbolLayoutAttributes::vertex(labelAnchor.point, tr, symbol.glyphOffset.y, tex.x + tex.w, tex.y, sizeData));
buffer.vertices.emplace_back(SymbolLayoutAttributes::vertex(labelAnchor.point, bl, symbol.glyphOffset.y, tex.x, tex.y + tex.h, sizeData));
buffer.vertices.emplace_back(SymbolLayoutAttributes::vertex(labelAnchor.point, br, symbol.glyphOffset.y, tex.x + tex.w, tex.y + tex.h, sizeData));

// Dynamic/Opacity vertices are initialized so that the vertex count always agrees with
// the layout vertex buffer, but they will always be updated before rendering happens
auto dynamicVertex = SymbolDynamicLayoutAttributes::vertex(labelAnchor.point, 0);
buffer.dynamicVertices.emplace_back(dynamicVertex);
buffer.dynamicVertices.emplace_back(dynamicVertex);
buffer.dynamicVertices.emplace_back(dynamicVertex);
buffer.dynamicVertices.emplace_back(dynamicVertex);

auto opacityVertex = SymbolOpacityAttributes::vertex(1.0, 1.0);
buffer.opacityVertices.emplace_back(opacityVertex);
buffer.opacityVertices.emplace_back(opacityVertex);
Expand All @@ -543,7 +544,7 @@ size_t SymbolLayout::addSymbol(Buffer& buffer,
segment.indexLength += 6;

placedSymbol.glyphOffsets.push_back(symbol.glyphOffset.x);

return index;
}

Expand Down
26 changes: 24 additions & 2 deletions src/mbgl/text/collision_feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ CollisionFeature::CollisionFeature(const GeometryCoordinates& line,
const float padding,
const style::SymbolPlacementType placement,
IndexedSubfeature indexedFeature_,
const float overscaling)
const float overscaling,
const float rotate)
: indexedFeature(std::move(indexedFeature_))
, alongLine(placement != style::SymbolPlacementType::Point) {
if (top == 0 && bottom == 0 && left == 0 && right == 0) return;
Expand All @@ -35,7 +36,28 @@ CollisionFeature::CollisionFeature(const GeometryCoordinates& line,
GeometryCoordinate anchorPoint = convertPoint<int16_t>(anchor.point);
bboxifyLabel(line, anchorPoint, anchor.segment, length, height, overscaling);
} else {
boxes.emplace_back(anchor.point, Point<float>{ 0, 0 }, x1, y1, x2, y2);
if (rotate) {
// Account for *-rotate in point collision boxes
// Doesn't account for icon-text-fit
const float rotateRadians = rotate * M_PI / 180.0;

const Point<float> tl = util::rotate(Point<float>(x1, y1), rotateRadians);
const Point<float> tr = util::rotate(Point<float>(x2, y1), rotateRadians);
const Point<float> bl = util::rotate(Point<float>(x1, y2), rotateRadians);
const Point<float> br = util::rotate(Point<float>(x2, y2), rotateRadians);

// Collision features require an "on-axis" geometry,
// so take the envelope of the rotated geometry
// (may be quite large for wide labels rotated 45 degrees)
const float xMin = std::min({tl.x, tr.x, bl.x, br.x});
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you bring over the comments from GL JS here or something similar? I think the "we have to represent on-axis geometry, so take the envelope of the rotated geometry" step is fairly non-obvious to someone casually browsing the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied over your comments

const float xMax = std::max({tl.x, tr.x, bl.x, br.x});
const float yMin = std::min({tl.y, tr.y, bl.y, br.y});
const float yMax = std::max({tl.y, tr.y, bl.y, br.y});

boxes.emplace_back(anchor.point, Point<float>{ 0, 0 }, xMin, yMin, xMax, yMax);
} else {
boxes.emplace_back(anchor.point, Point<float>{ 0, 0 }, x1, y1, x2, y2);
}
}
}

Expand Down
13 changes: 8 additions & 5 deletions src/mbgl/text/collision_feature.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ class CollisionFeature {
const float padding,
const style::SymbolPlacementType placement,
const IndexedSubfeature& indexedFeature_,
const float overscaling)
: CollisionFeature(line, anchor, shapedText.top, shapedText.bottom, shapedText.left, shapedText.right, boxScale, padding, placement, indexedFeature_, overscaling) {}
const float overscaling,
const float rotate)
: CollisionFeature(line, anchor, shapedText.top, shapedText.bottom, shapedText.left, shapedText.right, boxScale, padding, placement, indexedFeature_, overscaling, rotate) {}

// for icons
// Icons collision features are always SymbolPlacementType::Point, which means the collision feature
Expand All @@ -66,7 +67,8 @@ class CollisionFeature {
optional<PositionedIcon> shapedIcon,
const float boxScale,
const float padding,
const IndexedSubfeature& indexedFeature_)
const IndexedSubfeature& indexedFeature_,
const float rotate)
: CollisionFeature(line, anchor,
(shapedIcon ? shapedIcon->top() : 0),
(shapedIcon ? shapedIcon->bottom() : 0),
Expand All @@ -75,7 +77,7 @@ class CollisionFeature {
boxScale,
padding,
style::SymbolPlacementType::Point,
indexedFeature_, 1) {}
indexedFeature_, 1, rotate) {}

CollisionFeature(const GeometryCoordinates& line,
const Anchor&,
Expand All @@ -87,7 +89,8 @@ class CollisionFeature {
const float padding,
const style::SymbolPlacementType,
IndexedSubfeature,
const float overscaling);
const float overscaling,
const float rotate);

std::vector<CollisionBox> boxes;
IndexedSubfeature indexedFeature;
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -11,7 +11,7 @@ SymbolInstance makeSymbolInstance(float x, float y, std::u16string key) {
style::SymbolLayoutProperties::Evaluated layout_;
IndexedSubfeature subfeature(0, "", "", 0);
Anchor anchor(x, y, 0, 0);
return {anchor, line, shaping, {}, layout_, 0, 0, 0, style::SymbolPlacementType::Point, {{0, 0}}, 0, 0, {{0, 0}}, positions, subfeature, 0, 0, key, 0 };
return {anchor, line, shaping, {}, layout_, 0, 0, 0, style::SymbolPlacementType::Point, {{0, 0}}, 0, 0, {{0, 0}}, positions, subfeature, 0, 0, key, 0, 0};
}


Expand Down