Skip to content
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

Account for character descenders in text-radial-offset #8560

Open
tristen opened this issue Jul 26, 2019 · 13 comments
Open

Account for character descenders in text-radial-offset #8560

tristen opened this issue Jul 26, 2019 · 13 comments
Assignees
Labels
cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) feature 🍏

Comments

@tristen
Copy link
Member

tristen commented Jul 26, 2019

Motivation

Like text-offset that accepts number or an array of two numbers that adjusts the x and y coordinates, it would be great if text-radial-offset allowed the same functionality. That would account for the extra distance top positioned labels need for characters with descenders (i.e g, p, q):

Screen Shot 2019-07-26 at 10 09 31 AM

Design Alternatives

Not provide top as a list of possible positions to text-variable-anchor but that's a little limiting.

Design

{
  text-radial-offset: [0.5, 1] // x, y
}
@asheemmamoowala
Copy link
Contributor

@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

@tristen
Copy link
Member Author

tristen commented Jul 26, 2019

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>].

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?

👍

@mourner mourner added cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) feature 🍏 labels Jul 29, 2019
@mapcarta
Copy link

mapcarta commented Jul 30, 2019

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:

  • An array with two elements would specify the vertical (top and bottom) and horizontal (left and right) spacing, always in that order.
  • An array with four elements would specify the top, right, bottom and left offsets, always in that order.

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.

@mapcarta
Copy link

mapcarta commented Jul 31, 2019

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

@tristen tristen changed the title Allow array as value to text-radial-offset Account for character descenders in text-radial-offset Aug 1, 2019
@chloekraw chloekraw added this to the release-queso milestone Aug 2, 2019
@chloekraw chloekraw modified the milestones: release-queso, release-r Aug 5, 2019
@chloekraw
Copy link
Contributor

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 #8583 and #8527 higher priority for release-r.

@asheemmamoowala @pozdnyakov @alexshalamov @ansis

@pozdnyakov
Copy link
Contributor

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.

@pozdnyakov
Copy link
Contributor

Unfortunately, ascent and descent are not available as part of GlyphMetrics but we could estimate them based on the symbol size and top anchor.

@mapcarta
Copy link

mapcarta commented Aug 6, 2019

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".

@pozdnyakov
Copy link
Contributor

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

  • Current implementation:
    current_x

current_xgb

current_x

proposed_solution_xgb

  • Constant top and button offset

same_top_bottom_offset_x

proposed_solution_xgb

@mapcarta
Copy link

mapcarta commented Aug 6, 2019

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.

@asheemmamoowala
Copy link
Contributor

@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 #191 (comment), 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

@pozdnyakov
Copy link
Contributor

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 #191 looks crucial for solving this problem.

@chloekraw
Copy link
Contributor

chloekraw commented Aug 9, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) feature 🍏
Projects
None yet
Development

No branches or pull requests

8 participants