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

[core] Bring more stability to the symbol placement #16287

Merged
merged 7 commits into from
Mar 12, 2020
Merged

[core] Bring more stability to the symbol placement #16287

merged 7 commits into from
Mar 12, 2020

Conversation

pozdnyakov
Copy link
Contributor

@pozdnyakov pozdnyakov commented Mar 9, 2020 •
edited by chloekraw
Loading

This pull request improves the labels stability and performance for the tilted view. The following changes are done:

  • place already visible symbols first and hidden symbols last
  • stop squeezing the placement period (add 300ms delay) on new symbols appearance
  • the already placed variable labels stick to their latest-used anchor
  • collision index uses doubled padding with the tilted view

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

@pozdnyakov pozdnyakov self-assigned this Mar 9, 2020
@pozdnyakov pozdnyakov marked this pull request as ready for review March 9, 2020 15:02
@pozdnyakov pozdnyakov added Core The cross-platform C++ core, aka mbgl performance Speed, stability, CPU usage, memory usage, or power usage text rendering labels Mar 9, 2020
@pozdnyakov pozdnyakov added the needs changelog Indicates PR needs a changelog entry prior to merging. label Mar 9, 2020
@pozdnyakov pozdnyakov force-pushed the mikhail_stable_placement branch from e5330b8 to f81592c Compare March 10, 2020 21:10
@ansis
Copy link
Contributor

ansis commented Mar 11, 2020

Overall this looks good!

This looks like it prioritizes previously placed symbols within each sortkeyrange/layer but layer priority will still be respected? This seems like a great compromise to me.

the already placed variable labels stick to their latest-used anchor

It looks like they are allowed to be only their last used anchor. Did this make a large change for stability?

Overall this looks good to me! I think it's worth considering making this the default behavior instead of just for tilted views to keep things simpler

@pozdnyakov
Copy link
Contributor Author

the already placed variable labels stick to their latest-used anchor

It looks like they are allowed to be only their last used anchor. Did this make a large change for stability?

Yeah, the symbol once placed does not change its anchor - otherwise symbols are "jumping" while camera is moving

@pozdnyakov
Copy link
Contributor Author

Overall this looks good to me! I think it's worth considering making this the default behavior instead of just for tilted views to keep things simpler

the first change "place already visible symbols first and hidden symbols last" could be a default behavior indeed, however the other changes IMO are more navigation mode specific.

@pozdnyakov pozdnyakov force-pushed the mikhail_stable_placement branch from f81592c to 5d20959 Compare March 11, 2020 17:57
@chloekraw
Copy link
Contributor

@pozdnyakov this is great, thank you! Before you merge, may I suggest a small modification to the changelog:

Improve stability of symbol placement when the map is tilted

@pozdnyakov pozdnyakov force-pushed the mikhail_stable_placement branch from 718cea6 to 8b694de Compare March 11, 2020 18:26
@pozdnyakov pozdnyakov force-pushed the mikhail_stable_placement branch from 8b694de to aab5230 Compare March 11, 2020 18:40
@chloekraw chloekraw removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Mar 11, 2020
@pozdnyakov
[core] Place already visible symbols first and hidden symbols last
924bd6a
@pozdnyakov
[core] Do not squeeze placement period if the view is tilted
2589fe8 
If the view is not tilted, we want the new symbols to show up faster, so we squeeze the placement period.

On contrary, with the tilted view it's more important to make placement rarely for performance reasons and as the new symbols are normally "far away" and the user is not that interested to see them ASAP.
@pozdnyakov
[core] Variable labels stick to latest anchor if the view is tilted
7707010 
This is done in order to improve labels stability and for the performace reasons.
@pozdnyakov
[core] Collision index uses doubled padding with the tilted view
12b1720
@pozdnyakov
[core] Binary size fixup
3b82d87
@pozdnyakov pozdnyakov force-pushed the mikhail_stable_placement branch from aab5230 to 58d0ffc Compare March 12, 2020 20:16
@pozdnyakov pozdnyakov force-pushed the mikhail_stable_placement branch from 58d0ffc to 67ba54d Compare March 12, 2020 20:50
@pozdnyakov pozdnyakov merged commit b88fbe1 into master Mar 12, 2020
@pozdnyakov pozdnyakov deleted the mikhail_stable_placement branch March 12, 2020 21:35
@1ec5 1ec5 mentioned this pull request Mar 13, 2020
Merged
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chloekraw chloekraw chloekraw approved these changes

@tmpsantos tmpsantos tmpsantos approved these changes

@alexshalamov alexshalamov Awaiting requested review from alexshalamov

@ansis ansis Awaiting requested review from ansis

Assignees

@pozdnyakov pozdnyakov

Labels
Core The cross-platform C++ core, aka mbgl performance Speed, stability, CPU usage, memory usage, or power usage text rendering
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pozdnyakov @ansis @chloekraw @tmpsantos