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

ansis · 2019-12-06T21:48:14Z

fix #15964
partially port mapbox/mapbox-gl-js#9054

Currently symbol-sort-key works:

across tile boundaries with -allow-overlap: true
within tiles for collision detection

This pr makes it work for collisions across tile boundaries as well.

This splits the bucket into ranges of symbolInstances with the same sort key. All the ranges from all the tiles in a layer are inserted into a sorted list. Placement is done in the order of the list.

@pozdnyakov feel free to assign reviewing to someone else if you want

pozdnyakov · 2019-12-09T13:11:24Z

src/mbgl/renderer/render_layer.hpp

@@ -29,7 +30,7 @@ class LayerRenderData {

 class LayerPlacementData {
 public:
-    std::reference_wrapper<Bucket> bucket;
+    std::reference_wrapper<SymbolBucket> bucket;


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.

Right, thanks! I switched back to Bucket

pozdnyakov · 2019-12-09T14:07:59Z

src/mbgl/text/placement.hpp

+    friend bool operator<(const BucketPlacementParameters& lhs, const BucketPlacementParameters& rhs) {
+        return lhs.sortKey < rhs.sortKey;
+    }
+    SymbolBucket& bucket;


BucketPlacementParameters shall not know about SymbolBucket

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

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

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

pozdnyakov · 2019-12-09T14:17:28Z

src/mbgl/text/placement.hpp

    const RenderTile& tile;
    const mat4& projMatrix;
    std::string sourceId;
    std::shared_ptr<FeatureIndex> featureIndex;
    bool showCollisionBoxes;
+    float sortKey;
+    size_t symbolInstanceStart;
+    size_t symbolInstanceEnd;


could the newly added fields be SymbolBucket properties?

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

ansis · 2020-02-10T20:02:11Z

@pozdnyakov sorry about the delay getting back to this! I made changes based on your review so this is ready for another look.

The android-api-breakage ci run is failing. Any ideas why?

pozdnyakov · 2020-02-11T14:12:07Z

src/mbgl/renderer/layers/render_symbol_layer.cpp

+
+            bool firstInBucket = true;
+
+            if (bucket->sortKeyRanges.size() == 0) {


nit: bucket->sortKeyRanges.empty()

pozdnyakov · 2020-02-11T14:13:58Z

src/mbgl/renderer/layers/render_symbol_layer.cpp

+                for (const SortKeyRange& sortKeyRange : bucket->sortKeyRanges) {
+                    LayerPlacementData layerData{*bucket,
+                                                 renderTile,
+                                                 static_cast<const GeometryTile*>(tile)->getFeatureIndex(),


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

pozdnyakov · 2020-02-11T14:17:19Z

src/mbgl/text/placement.cpp

@@ -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;


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

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

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

pozdnyakov · 2020-02-11T14:20:51Z

src/mbgl/layout/symbol_layout.cpp


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


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

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

pozdnyakov · 2020-02-11T14:22:24Z

src/mbgl/text/placement.cpp

-        for (const SymbolInstance& symbol : bucket.symbolInstances) {
-            placeSymbol(symbol);
+        auto endIt = bucket.symbolInstances.begin() + params.symbolInstanceEnd;
+        for (auto it = bucket.symbolInstances.begin() + params.symbolInstanceStart; it != endIt; it++) {


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

fix #15964 partially port mapbox/mapbox-gl-js#9054

ansis changed the title ~~sort symbols using symbol-sort-key before placement~~ [core] sort symbols using symbol-sort-key before placement Dec 6, 2019

pozdnyakov requested review from pozdnyakov and alexshalamov December 9, 2019 12:41

pozdnyakov added Core The cross-platform C++ core, aka mbgl text rendering labels Dec 9, 2019

pozdnyakov reviewed Dec 9, 2019

View reviewed changes

chloekraw added this to the release-unicorn milestone Dec 18, 2019

chloekraw mentioned this pull request Jan 14, 2020

Floating points are not honored when evaluating the symbolSortKey #16114

Closed

asheemmamoowala added the GL JS parity For feature parity with Mapbox GL JS label Feb 3, 2020

ansis added 4 commits February 10, 2020 09:21

sort symbols using symbol-sort-key before placement

a95c480

clang-format

82ceec3

fixup

7f73d24

update based on feedback

410a2b4

ansis force-pushed the ansis-symbol-sort-key-collisions branch from 2534f87 to 410a2b4 Compare February 10, 2020 15:51

ansis added 6 commits February 10, 2020 10:57

fixup

143a2dd

fixup

f3e8670

fixup

a39935d

format

3e00584

fixup

4a267d2

fixup

6c57da1

pozdnyakov reviewed Feb 11, 2020

View reviewed changes

fix based on feedback

5259e5c

pozdnyakov approved these changes Feb 11, 2020

View reviewed changes

format

001113c

ansis merged commit 4f18d5f into master Feb 11, 2020

ansis deleted the ansis-symbol-sort-key-collisions branch February 11, 2020 16:24

ansis added a commit that referenced this pull request Feb 11, 2020

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

d7c506f

fix #15964 partially port mapbox/mapbox-gl-js#9054

ansis mentioned this pull request Feb 11, 2020

cherry pick #16023 (cross-tile sort-key for placement) to release-unicorn #16192

Merged

ansis added a commit that referenced this pull request Feb 11, 2020

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

bfec84d

fix #15964 partially port mapbox/mapbox-gl-js#9054

alexshalamov pushed a commit that referenced this pull request Feb 12, 2020

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

2ee59f3

fix #15964 partially port mapbox/mapbox-gl-js#9054

pengdev mentioned this pull request Feb 13, 2020

[docs] Update changelog for v9.0.0 after bumping gl-native. mapbox/mapbox-gl-native-android#176

Merged

alexshalamov pushed a commit that referenced this pull request Feb 14, 2020

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

ff131a3

fix #15964 partially port mapbox/mapbox-gl-js#9054

alexshalamov mentioned this pull request Feb 14, 2020

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

Merged

3 tasks

alexshalamov pushed a commit that referenced this pull request Feb 14, 2020

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

7709c3f

fix #15964 partially port mapbox/mapbox-gl-js#9054

alexshalamov pushed a commit that referenced this pull request Feb 14, 2020

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

761ef9c

fix #15964 partially port mapbox/mapbox-gl-js#9054

1ec5 mentioned this pull request Feb 21, 2020

Mapbox Maps SDK for iOS v5.5.1 mapbox/mapbox-gl-native-ios#174

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ansis commented Dec 6, 2019

pozdnyakov Dec 9, 2019

ansis Feb 10, 2020

pozdnyakov Dec 9, 2019

ansis Dec 9, 2019

pozdnyakov Dec 9, 2019

ansis Feb 10, 2020

pozdnyakov Dec 9, 2019

ansis Feb 10, 2020

ansis commented Feb 10, 2020

pozdnyakov Feb 11, 2020

pozdnyakov Feb 11, 2020

pozdnyakov Feb 11, 2020

ansis Feb 11, 2020

pozdnyakov Feb 11, 2020

pozdnyakov Feb 11, 2020

ansis Feb 11, 2020

pozdnyakov Feb 11, 2020

pozdnyakov Feb 11, 2020


		bool firstInBucket = true;

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

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

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

Conversation

ansis commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansis commented Feb 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment