Skip to content

Increase maximum label width by factor of 2 - #6375

Merged
ChrisLoer merged 2 commits into
masterfrom
wider-labels
Mar 22, 2018
Merged

ChrisLoer merged 2 commits into
masterfrom
wider-labels

Conversation

@ChrisLoer

@ChrisLoer ChrisLoer commented Mar 21, 2018 •

Copy link
Copy Markdown
Contributor

This is a partial fix for #1531 -- wide labels can still overflow (at which point they stop rendering correctly), but they can now be twice as wide, which will hopefully cover a much wider variety of use cases. #5150 took care of the collision detection problem, so there's no added work necessary for doing collision detection on labels that are wider than the tile's symbol buffers. Also, we no longer use tile clipping for symbols, so there's no problem with labels bleeding far outside their origin tile (err, except for api-gl, wide labels will get rendered-correctly per-tile, but a very wide label in a neighboring tile may be clipped if it doesn't make it into the symbol buffers).

This decreases our glyph-placement precision from 1/64th pixel to 1/32nd pixel, which still seems plenty high. I can't notice any difference in rendering, and all of the render tests pass without change. Doing another halving to 1/16th pixel precision still looks fine to my eyes, but it causes two render tests to fail on (very slight) glyph position changes. For now, 1/32nd seems like a good conservative choice.

Test result before fix:

screenshot 2018-03-21 14 10 25

/cc @ansis @jfirebaugh @james-wagner11

@ChrisLoer
ChrisLoer requested review from ansis and mollymerp March 21, 2018 21:28
@james-wagner11

Copy link
Copy Markdown

Before:
4
After:
5

Thank you @ChrisLoer! I haven't found any labels that still overflow. The additional 2x width makes a big difference for us.

@ChrisLoer

Copy link
Copy Markdown
Contributor Author

@james-wagner11 that's great! Thanks for posting those pictures, and happy this turned out to be (knock on wood) an easy one.

@mollymerp mollymerp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice solution! seems very reasonable 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants