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

[Core] Load adjacent tiles for better label placement #3357

Closed
wants to merge 1 commit into from

Conversation

bsudekum
Copy link

For a given view, tiles at the view boundary do not have knowledge of labels beyond the viewport. We can see this in action in a couple different situations:

1 - In iOS/android if a label is at the edge the adjacent tile, it will abruptly render to the map when the new tile is loaded

2

Subtle, but notice howe Colorado Springs comes into view. With this change, the label would already be rendered.

2 - When rendering tiles in node, a given tile has no concept of surrounding tiles. Because of this, labels are clipped at the tile boundary edge.

Notice how the label City of London is clipped between tiles

With this PR, we would download more tiles than necessary to account for labels on adjacent tiles.

With the fix, City of London renders on both tiles

This however, is a hack. I propose we :

  • Add flag to allow for this feature to be turned on and off. By default it should be false
  • Don't actually render the tile. It should be loaded, parsed and used in label collision math only

Let's not think about the cost of fetching more tiles, yet

/cc @kkaefer @jfirebaugh @tmpsantos @ansis

@mikemorris
Copy link
Contributor

Nice writeup and illustration of the bugs @bsudekum, refs #2829

@mikemorris mikemorris added the Node.js node-mapbox-gl-native label Dec 19, 2015
@jfirebaugh
Copy link
Contributor

Let's not think about the cost of fetching more tiles, yet

It's meaningless to evaluate this change without considering this, and I think for server side rendering, loading 9x the number of tiles to render a given viewport is a non-starter. Therefore I think this needs to be fixed at a style level (by reducing label size or ensuring that long labels have line breaks) or vector tile level (by increasing the buffer for features that are going to be labeled).

@bsudekum
Copy link
Author

After talking with @jfirebaugh, #2829 is the way to go and is mergable. Loading X more tiles on each request is too much.

@mikemorris do you have bandwidth to fix up #2829 ?

@miccolis
Copy link
Contributor

server side rendering, loading 9x the number of tiles to render a given viewport is a non-starter

I'm not sure this is the case and if this ends up being the only way to solve clipping problems we'll have no choice. Loading more tiles simply becomes the new baseline.

I'd be very curious if we could resurrect this PR and load test. Knowing exactly what the cost is would be very useful. @mikemorris what would that take?

@mikemorris
Copy link
Contributor

@miccolis Could rebase this onto master today and tag a node-mbgl prerelease to use for testing.

@mikemorris mikemorris self-assigned this Jan 11, 2016
@miccolis
Copy link
Contributor

@mikemorris sounds good. Anybody want to place a bet on how big the perf hit will be? I think we're looking at like 20% worse.

@jfirebaugh
Copy link
Contributor

At least 2x slower. It will be doing 9x the tile loading, parsing, and layout, but some of the loading overhead will be mitigated due to caching.

@yhahn
Copy link
Member

yhahn commented Jan 11, 2016

Let's not do this for now -- going to dig into the styles involved with clipped labels and see if they could be more strategic about label placement to make sure label rendering is deterministic across tiles.

@mikemorris
Copy link
Contributor

FWIW, this does ridiculously stupid things when combined with pitch, I gave up updating the vector tile fixtures in mapbox-gl-test-suite at that point.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Node.js node-mapbox-gl-native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants