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

[core] stable position of labels at tile borders in tile mode #16040

Merged
merged 11 commits into from
Jan 8, 2020

Conversation

pozdnyakov
Copy link
Contributor

This PR does two things in Tile mode:

  • variable labels, which intercept tile borders, always stick to the first available anchor
  • all labels, which intercept tile borders, are placed first

These changes allow to avoid cutting-off labels on tile borders if the variable text placement is enabled.

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

@pozdnyakov pozdnyakov self-assigned this Dec 12, 2019
@pozdnyakov pozdnyakov added Core The cross-platform C++ core, aka mbgl text rendering ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Dec 12, 2019
@pozdnyakov pozdnyakov marked this pull request as ready for review December 12, 2019 11:24
@pozdnyakov
Copy link
Contributor Author

Can be merged only after ✔️ from @mapbox/static-apis

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.

Few nits.

@pozdnyakov sorting along edge border will give 'border' symbols preferential placement, thus, rendering result might look unnatural, e.g., swarming along edges, other than that, PR looks fine.

src/mbgl/text/collision_index.cpp Outdated Show resolved Hide resolved
src/mbgl/text/placement.cpp Show resolved Hide resolved
src/mbgl/text/placement.cpp Outdated Show resolved Hide resolved
@riastrad riastrad requested a review from springmeyer December 16, 2019 15:22
Copy link
Contributor

@rmrice rmrice left a comment

Choose a reason for hiding this comment

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

This fix looks good on the @mapbox/static-apis side 👍

@pozdnyakov pozdnyakov removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jan 7, 2020
@pozdnyakov
Copy link
Contributor Author

Found few errors (visible after we've #16068 in place) will apply fix to this PR shortly.

@riastrad
Copy link
Contributor

riastrad commented Jan 7, 2020

Hooray for the new test runner! 😁

@pozdnyakov pozdnyakov force-pushed the mikhail_tile_variable_labels branch from c8d47ab to 2c1cae3 Compare January 7, 2020 20:39
@pozdnyakov
Copy link
Contributor Author

The commits 1b55ca8 and 2ac9679 make the difference comparing to the previous patch set and fix the discovered errors.

@pozdnyakov pozdnyakov force-pushed the mikhail_tile_variable_labels branch from 9f22914 to 3efb7ce Compare January 8, 2020 09:29
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, maybe worth adding changelog entry

@pozdnyakov pozdnyakov merged commit c46f0ba into master Jan 8, 2020
@pozdnyakov pozdnyakov deleted the mikhail_tile_variable_labels branch January 8, 2020 10:43
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 text rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants