-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
get rid of text-max-size, icon-max-size #1419
Conversation
if (values['icon-size']) { | ||
layout['icon-max-size'] = values['icon-size'].calculate(18, fakeZoomHistory); | ||
layout['icon-size'] = values['icon-size'].calculate(z + 1, fakeZoomHistory); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to work in all cases? What if there's a big discontinuity in the text-size
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be mostly ok. If the text is way bigger at high zoom levels than at low zoom levels, then the first label along a line will be placed slightly further from the end. I think the impact on density should be tiny and it already existed before this pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ansis Are you sure this won't affect density much? In our template styles, we originally had used -max-size
values that were always just equal to the largest value of a -size
ramp (which is typically pretty close to being the z18 size), and found that this impacted the density of labels at low zoom levels (when the -size
is relatively much smaller). That was the motivation for changing all the -max-size
values to use ramps that look something like this. From what you're saying though it sounds like using a ramps for -max-size
was not ideal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, yeah I didn't realize that. Thanks for flagging @nickidlugash and @lucaswoj
There are two parts here.
.calculate(18)
This value is used to calculate the label length that is used to calculate the initial offset. This means that at lower zoom levels labels may be offset a bit further from the beginning of the line than they were before. You're right, this is different than before. The differences don't seem to be bad. They're just different:
I think this is ok
.calculate(z + 1)
This is the size used to make the collision boxes. The boxes are a bit bigger now that we use z + 1 instead of just z. This means slightly lower density but also prevents some collisions. It is the same idea @nickidlugash suggested here: https://github.com/mapbox/app/issues/492#issuecomment-74004095
In most cases this won't have a big effect since font size doesn't grow much within a single zoom. When it does grow a lot within a single zoom this will help prevent collisions.
and make text-size, icon-size layout properties fixes mapbox/mapbox-gl-style-spec#255
8a6da8f
to
c343ced
Compare
and make text-size, icon-size layout properties.
fixes mapbox/mapbox-gl-style-spec#255
The
-size
properties are used as both paint and layout properties. In the spec they are layout properties instead of paint properties because the restrictions on layout properties are stronger. The implementation is a bit awkward because the property doesn't fit with existing concepts that well.other pull requests:
mapbox/mapbox-gl-test-suite#39
mapbox/mapbox-gl-style-spec#329
mapbox/mapbox-gl-styles#171