Add clamping for halo-width values to avoid ugly boxes. #7441
Conversation
Fixes issue #7204 (or at least makes it less dramatic) Add explanatory warning to `icon-halo-width` in v8.json.
|
Huh, that's interesting, the |
|
No obvious change in the paint benchmark, although this would be hard to pick up. This is extra work for every single glyph we draw, but I think a clamp should be really cheap, so not too worried about it.
|
| highp float alpha = smoothstep( | ||
| clamp(buff - gamma_scaled, 0.0, 1.0), | ||
| clamp(buff + gamma_scaled, 0.0, 1.0), | ||
| dist); |
There was a problem hiding this comment.
This clamps the blur distance but I think it also shifts the center of the blur which is usually set by -halo-width. Is this ok? I think it probably is.
If we wanted to clamp just the -halo-blur we could do that with this I think:
highp float gamma_scaled = min(gamma * gamma_scale, min(buff, 1.0 - buff));There was a problem hiding this comment.
Ah good point. Went with the alternative solution of just not clamping values over 1.0 for the smoothstep, since it's really only when you hit the max "outside" distance that you get a noticeable problem. This way the "center" of the blur does still shift if you get clamped on the bottom side (but I think it's OK, we're compromising anyway at that point), but there's no change in the center of the blur as long as buff - gamma_scaled is > 0.
Using "0" wasn't consistent across different GL implementations.
|
Huh, so the behavior of smoothstep is technically undefined when both edges are the same (which is the case when we don't have a blur): https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/smoothstep.xhtml. I'm not sure if that might account for why CI got slightly different results than I got on my local machine, or if there was some difference in the texture implementation. At any rate, I switched to clamping to a min value of 1/256, which actually makes more sense than the previous clamping to 0 (since the whole "outside" of the image is filled with zeros). |
|
Huh, so the behavior of smoothstep is technically undefined when both edges are the same (which is the case when we don't have a blur): https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/smoothstep.xhtml. I'm not sure if that might account for why CI got slightly different results than I got on my local machine, or if there was some difference in the texture implementation. At any rate, I switched to clamping to a min value of 1/256, which actually makes more sense than the previous clamping to 0 (since the whole "outside" of the image is filled with zeros). Also updated the |
|
@peterqliu brought to my attention that he can think of at least one map style that intentionally uses the "overflow halo-width to fill the whole glyph box" behavior, e.g.:
So I guess it's time to tag @mapbox/studio and @mapbox/maps-design: is this a breaking change or backwards compatibility problem? I would have never thought to try to use an overflowing max-width to get that effect (and it looks weird when the glyphs have different baselines)... but on the other hand, it's kind of like a quick-and-dirty version of |
Clamping the upper value could change the center of the smoothstep function even when the halo was entirely within the maximum "outside" distance (and the outside distance is the part that would be visible).
|
@ChrisLoer filing the entire glyph box with a halo-color can simulate a rather nice effect stemming back to some wood-cut and engraved maps in which each label was made separately then placed as insets into the material that the map was cut or engraved out of. Or for wood-cut maps, a lot of wood material is removed around the labels for legibility. Here is an image of a wood-cut plate:
The effect produced is a slightly sloppy but distinct aesthetic where the labels have space in-between themselves and other map elements. In this example, note the neighborhood labels:
I've seen some examples where people will give the halo a different color than the background, creating a color-box background for labels. @ian29 used this technique to indicate label type below:
To my limited knowledge, extending the halo to it's maximum width is the only way to create this effect. If this PR proposes to remove this ability, is there another way background label boxes could be generated? |
|
That's great context, thanks @natslaughter! Sounds like we should hold off on implementing this clamping behavior until we at least have some way to intentionally get the box effect. |
|
Thanks all, we'll revisit if necessary for a better developer experience. It seems to me that we could do a better job of explaining #7204 (comment) in our documentation and directing developers who have issues there. cc/ @mapbox/docs @mapbox/support |
|
@natslaughter That text background is actually what I was looking for in #6856, except something that doesn't need |
|
Documenting here that the original behavior was the intended behavior when I implemented it back in 2013, specifically to allow the effects that @natslaughter outlined in |
Fixes issue #7204 (or at least makes it less dramatic)
Add explanatory warning to
icon-halo-widthin v8.json.The blur effect gets clamped at the same time, which means that if the "blur" pixels are the ones that get clamped the blur can end up being removed. I considered an alternative in which we'd reduce the halo_width even further to accommodate the blur, but it was more complicated and it didn't seem obvious that would be closer to the style designer's intent (also, in some cases there just might not be enough room for the blur).
Launch Checklist
@mapbox/studioand/or@mapbox/maps-designif this PR includes style spec changescc @atsarego @ansis