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

[core][tile mode] Labels priority fixes #16432

Merged
merged 5 commits into from
Apr 28, 2020

Conversation

pozdnyakov
Copy link
Contributor

This PR fixes label priority issues in the Tile mode. It does the following:

  • strictly arranges all the intersecting labels accordingly to the style-defined priorities
  • fixes placement order of the variable labels.
    Before this change, all variable labels that could potentially
    intersect tile borders were placed first, breaking the style
    label placement priority order. Now, all the variable labels, which
    do not actually intersect the tile borders, are placed accordingly
    to the style-defined priorities

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

@pozdnyakov pozdnyakov added the needs changelog Indicates PR needs a changelog entry prior to merging. label Apr 24, 2020
@pozdnyakov pozdnyakov self-assigned this Apr 24, 2020
@pozdnyakov pozdnyakov force-pushed the mikhail_tile_mode_label_priority branch from a51a0c7 to 4887430 Compare April 27, 2020 16:46
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.

Can we have a render test for it?

Before this change, all variable labels that could potentially
intersect tile borders were placed first, breaking the style
label placement priority order.
The most important variable labels a placed right away at
"populate intersections" phase, even if they do not actually
intersect the tile borders.
@pozdnyakov pozdnyakov force-pushed the mikhail_tile_mode_label_priority branch from 4887430 to d658f2d Compare April 28, 2020 10:00
@pozdnyakov pozdnyakov requested a review from a team as a code owner April 28, 2020 10:00
@pozdnyakov
Copy link
Contributor Author

Can we have a render test for it?

mapbox/mapbox-gl-js#9638

@pozdnyakov pozdnyakov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Apr 28, 2020
@chloekraw
Copy link
Contributor

chloekraw commented Apr 28, 2020

Can we have a render test for it?

mapbox/mapbox-gl-js#9638

@pozdnyakov What about a render test only for the core changes outside of tile mode? I'm evaluating whether this change is significant enough to merit mention in the Maps SDK changelog.

@pozdnyakov
Copy link
Contributor Author

@pozdnyakov What about a render test only for the core changes outside of tile mode? I'm evaluating whether this change is significant enough to merit mention in the Maps SDK changelog.

@chloekraw it does not change anything in Continuous/Static mode - no changes there, only a bit of refactoring..

@chloekraw
Copy link
Contributor

Ah okay, thanks!

@pozdnyakov pozdnyakov merged commit af14e1d into master Apr 28, 2020
@pozdnyakov pozdnyakov deleted the mikhail_tile_mode_label_priority branch April 28, 2020 17:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants