Open tristen opened 5 years ago
@tristen Are you proposing that the array takes only 2 values -[a, b]
, where a
is the offset applied to all non-top
anchor values, and b
is applied only to top
?
This would be very confusing to document and explain. For example, when text-variable-anchor: ["top"]
, the single value should be used, but changing it to text-variable-anchor: ["top", right]
would require switching to an array [ <right_offset>, <top_offset>]
.
What if instead auto justification took into account the full height of symbols so that it anchors to the bottom of glyhps before adding the text-radial-offset
value?
cc @vakila @ansis
This would be very confusing to document and explain. For example, when
text-variable-anchor: ["top"]
, the single value should be used, but changing it totext-variable-anchor: ["top", right]
would require switching to an array[ <right_offset>, <top_offset>]
.
Good point.
What if instead auto justification took into account the full height of symbols so that it anchors to the bottom of glyhps before adding the
text-radial-offset
value?
đź‘Ť
Yes this is needed. One way to do this without confusion is to have the 'text-radial-offset' array contain either exactly two or four elements:
This way 'text-variable-anchor' could be changed without any trouble.
What if instead auto justification took into account the full height of symbols so that it anchors to the bottom of glyhps before adding the text-radial-offset value?
I agree this would be a far more elegant and desirable solution if it is possible.
I believe the underlying problem is that many of the fonts themselves to not seem to be properly vertically centered. While some fonts such as the default DIN Offc Pro and Open Sans seems to be properly centered, others such as Ubuntu and Lato seem to be higher than center. If for example the offset is set based off of either a default top or bottom anchor location, all of the other anchor locations will have incorrect spacing.
Perhaps there should be no need for this feature if font centering is fixed? #191
Capturing from chat with @tristen, I'm removing the high priority label because we can work around this issue by not using "top"
as an anchor position for release-r
, making https://github.com/mapbox/mapbox-gl-js/issues/8583 and https://github.com/mapbox/mapbox-gl-js/issues/8527 higher priority for release-r
.
@asheemmamoowala @pozdnyakov @alexshalamov @ansis
At the moment radial offset does not consider if the placed label has symbols with descent or ascent. We (@alexshalamov and myself) believe that the desired behavior shall be the following:
if the label is placed at top
anchor, engine should check whether the label's last line contains symbols with descent. If yes, the label shall be shifted up by the descent size.
if the label is placed at bottom
anchor, engine should check whether the label's first line contains symbols with ascent. If yes, the label shall be shifted down by the ascent size.
Unfortunately, ascent and descent are not available as part of GlyphMetrics
but we could estimate them based on the symbol size and top anchor.
Maybe there is no need to account for actually rendered character descenders and ascenders. A person viewing a map might expect for all text to have the same offset, regardless of the presence of descenders or ascenders. It's then just a matter of making sure that font itself is vertically centered so that the different label positions present a uniform offset. #191
However if the label is in all "UPPER CASE" then there might need to be another style option to account for that as that is a different case compared to "Title Case" and "lower case".
Maybe there is no need to account for actually rendered character descenders and ascenders
IMO it might affect the rendering of labels that do not have descenders and ascenders. Pls consider
Constant top and bottom offset
Wow those are very helpful illustrations, thank you @pozdnyakov. With whatever you come up with I'm sure it will be very good. I imagine it will just require a bunch of experimentation and tests as you go along to see whatever looks the best, so no point in me trying to imagine potential solutions. For us anyway it's not a major issue, the biggest potential problem we have is that some fonts themselves (including ones I've uploaded) are not vertically centered and so don't work well with text-variable-anchor at the moment.
@pozdnyakov Thanks for the clear illustrations. I do think that the best way to address this is to consider ascenders and descenders when computing the baseline before applying text-radial-offset
.
Looks like there has been recent interest in https://github.com/mapbox/mapbox-gl-js/issues/191#issuecomment-510349209, which would help solve not only this issue, but many other baseline-alignment related bugs as well.
I'd be in favor of helping that effort be pushed along to unblock the fix for this ticket. What do you think?
cc @ansis
the biggest potential problem we have is that some fonts themselves (including ones I've uploaded) are not vertically centered and so don't work well with text-variable-anchor at the moment.
Is there an issue for it? My guess is that this problem is caused by missing font meta-data, in particular a more accurate estimation of the glyph offset position, so fixing of https://github.com/mapbox/mapbox-gl-js/issues/191 looks crucial for solving this problem.
Sounds to me like #191 is crucial for solving this problem for some fonts, no?
If we’re interested in helping unblock this work, we should coordinate with @tristen who would appreciate it. @tristen and @springmeyer have begun pairing on #191. Please note, both @tristen and I consider #8527 higher priority (per the high priority
tag on #8527 and #8599 which indicates this issue is lower priority), which currently has no one working on it. It will be on @ansis plate but he’s investigating a higher priority thing right now. So, if there’s extra bandwidth, I’d prefer that we investigate #8527 in lieu of helping on #191 at this current moment.
Motivation
Like
text-offset
that accepts number or an array of two numbers that adjusts thex
andy
coordinates, it would be great iftext-radial-offset
allowed the same functionality. That would account for the extra distance top positioned labels need for characters with descenders (i.eg, p, q
):Design Alternatives
Not provide
top
as a list of possible positions totext-variable-anchor
but that's a little limiting.Design