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

[core][tile mode] Reduce cut-off labels (part 2) #16369

Merged
merged 7 commits into from
Apr 8, 2020

Conversation

pozdnyakov
Copy link
Contributor

@pozdnyakov pozdnyakov commented Apr 2, 2020

Now, the intersecting symbols are placed across all layers symbol by symbol according to the following rules:

  1. First we look, which of the tile border(s) the symbol intersects and prioritize the the symbol placement accordingly (high priority -> low priority): vertical & horizontal -> vertical -> horizontal
  2. For the symbols that intersect the same tile border(s), assuming the tile border split symbol into several sections, we look at the minimal section length. The symbol with a larger minimal section length is placed first.
  3. For the symbols that intersect the same tile border(s), and have equal minimal section length, we look at the anchor coordinates.
  4. Finally, if all the previous criteria are the same, we look at the symbol key hashes.

Fixes https://github.com/mapbox/mapbox-gl-native-team/issues/223

@pozdnyakov pozdnyakov added Core The cross-platform C++ core, aka mbgl needs changelog Indicates PR needs a changelog entry prior to merging. labels Apr 2, 2020
@pozdnyakov pozdnyakov requested a review from alexshalamov April 2, 2020 10:50
@pozdnyakov pozdnyakov self-assigned this Apr 2, 2020
@pozdnyakov pozdnyakov force-pushed the mikhail_tile_mode_placement_fixes_2 branch from 39c284c to 2eed488 Compare April 2, 2020 11:10
@pozdnyakov pozdnyakov changed the title [core][tile mode] Reduce cut-off labels #2 [core][tile mode] Reduce cut-off labels (part 2) Apr 2, 2020
@pozdnyakov pozdnyakov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Apr 2, 2020
@pozdnyakov pozdnyakov requested a review from tmpsantos April 2, 2020 12:38
@pozdnyakov pozdnyakov force-pushed the mikhail_tile_mode_placement_fixes_2 branch from d45aaae to c9a712c Compare April 2, 2020 12:42
render-test/runner.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@tmpsantos tmpsantos left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but would be great if we could have more 👀 here.

src/mbgl/text/collision_index.cpp Show resolved Hide resolved
src/mbgl/text/placement.cpp Show resolved Hide resolved
src/mbgl/text/placement.cpp Show resolved Hide resolved
render-test/runner.cpp Outdated Show resolved Hide resolved
src/mbgl/text/placement.cpp Show resolved Hide resolved
@pozdnyakov
Copy link
Contributor Author

@alexshalamov @tmpsantos I've extracted render tests changes to a separate pr #16380 and considered your comments there, ptal.

@pozdnyakov pozdnyakov force-pushed the mikhail_tile_mode_placement_fixes_2 branch 3 times, most recently from d3ce30c to 2bcf354 Compare April 6, 2020 16:17
Make `placeSymbol()` a method and introduce copiable `PlacementContext`.
Now, the intersecting symbols are placed across all layers according to the following rules:
1) First we look, which of the tile border(s) the symbol intersects and prioritize the the symbol accordingly (high priority -> low priority):
vertical & horizontal -> vertical -> horizontal

2) For the symbols that intersect the same tile border(s), assuming the tile border split symbol into several sections, we look at the minimal section length.
The symbol with a larger minimal section length is placed first.

3) Finally, for the symbols that intersect the same tile border(s), and have equal minimal section length, we look at the anchor Y cordinate.
@pozdnyakov pozdnyakov force-pushed the mikhail_tile_mode_placement_fixes_2 branch from 2bcf354 to c1cf1da Compare April 7, 2020 11:48
@pozdnyakov pozdnyakov force-pushed the mikhail_tile_mode_placement_fixes_2 branch from c1cf1da to 4327c2e Compare April 7, 2020 15:56
Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

lgtm

One idea, maybe we should use overlap area as an input for prioritizing placement for overlapping labels.

if (flagsA != flagsB) return flagsA > flagsB;
// If both intersects the same border(s), look for a more noticable cut-off.
if (a.status.minSectionLength != b.status.minSectionLength) {
return a.status.minSectionLength > b.status.minSectionLength;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should vertical and horizontal length be checked separately? Also, should it be total overlap area vs single 'section length'? Label that is in a tile corner can have bigger overlap area, yet, smaller section length.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the proposal! will consider it separately

@pozdnyakov pozdnyakov merged commit 7f53cec into master Apr 8, 2020
@pozdnyakov pozdnyakov deleted the mikhail_tile_mode_placement_fixes_2 branch April 8, 2020 08:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants