Introduce descender and ascender for font metrics + fix several text rendering issues

[zmiao]

This pr is covering:

• Introduce descender and ascender for font metrics, which are important for aligning text baselines when mixed fonts are used.
  This pr will only load descender and ascender for fonts loaded from server-side ( background: Expose ascender/descender metadata properties in protobuf node-fontnik#160 will introduce new metadata ascender and descender, it would help fix the issue Use font-provided metadata to determine glyph baseline position #191. This pr only covers reading data from pbf and partially replacing hardcoded baseline for fixing mixed fonts dis-alignment ). Support for loading metrics for local fonts is not implemented yet.
• Fix vertical text shaping for characters that have very big part below baselines, such as p, g, y, etc.

Before	After

• Fix vertical text shaping with ZWSP and punctuations.

Before	After

• Fix vertical text rendering with text-offset property: text-offset property is broken for CJK text in vertical writing mode #10687
  • Add new render tests(point label and line label)
• Fix collision box generation for vertical text
• Fix text rendering when both text-offset and text-rotate are set

 "text-rotate": 45,
  "text-offset": [
      3,
      6
  ]

Before	After

• Enable corresponding ignored render tests
• Check render tests failure

Follow-up:
Introduce font metrics for local glyph generation (TinySDF).
Fix issue #8560

Launch Checklist

• briefly describe the changes in this PR
• include before/after visuals or gifs if this PR includes visual changes
• write tests for all new functionality
• manually test the debug page

[zmiao]

Some rendering results after applying the ascender and descender values as baseline for fixing the dis-alignment of mixed fonts:

With De/Ascender	Without De/Ascender

[chloekraw]

@zmiao very excited for this to land!

Looking at the third render test, I don't see a difference in the before vs. after photo. What am I missing?

Also, is it expected that vertical text will have differing gaps above/below characters depending on the character and the font?

It looks like there's too little space under the g, though perhaps there's not much we can or should do about that.

It also looks like there's not enough space under the p / too much space under the o. If characters are placed vertically, we may want to offset so that the space between characters begins after the descent line if there is a descender, rather than using the true baseline.

I think the eye is looking for the space between where one character ends and the next one starts to be even, and the idea of a baseline is basically moot when there's only a single character in the line.

[zmiao]

@chloekraw for your concern about the third picture,

Looking at the third render test, I don't see a difference in the before vs. after photo. What am I missing?

my intention of this is to show that there is no obvious rendering differences for vertical layout texts. Since the fix is mainly for horizontal text dis-aligned problem. Right now all the vertical texts aligned with the middle line of the shaping box.

It looks like there's too little space under the g, though perhaps there's not much we can or should do about that.

It also looks like there's not enough space under the p / too much space under the o. If characters are placed vertically, we may want to offset so that the space between characters begins after the descent line if there is a descender, rather than using the true baseline.

This could be anther text shaping issue to fix later in the future. Descender needs to be taken into account.

I think the eye is looking for the space between where one character ends and the next one starts to be even, and the idea of a baseline is basically moot when there's only a single character in the line.

Baseline is necessary even if there is only one character, we need to use baseline for creating glyph, and also taking baseline into account for text-variable-offset calculation, which is related to issue #8560 I plan to land pr after this pr is merged.

[chloekraw]

@zmiao gotcha, thanks! I misunderstood the scope of your pr.

Since the fix is mainly for horizontal text dis-aligned problem.

Makes sense!

[mourner]

This is great! Two nits / questions:

• It's unfortunate that this PR would add 36 MB of test data, practically doubling the size of the repo. Can we reduce this in any way?
• If ascender and descender are font-level attributes, do we have to save those in each glyph? Can we refactor the code to avoid the redundancy?

[zmiao]

Hi @mourner, for your concern:

• It's unfortunate that this PR would add 36 MB of test data, practically doubling the size of the repo. Can we reduce this in any way?

Thanks for providing this information. The *.pbf data I added is intended to test some characters I used in the test. I will remove some characters so that corresponding pbf file could be erased.

• If ascender and descender are font-level attributes, do we have to save those in each glyph? Can we refactor the code to avoid the redundancy?

This is a good suggestion! I will adapt the code. Thanks!

[zmiao]

@chloekraw one updates, actually this patch will also fix vertical text dis-alignment if there are multiple font scales in one line. Please check my updated test result picture.

[asheemmamoowala]

One question I have is whether this will affect local glyph generation adversely and by how much?

The PR looks great otherwise, barring a couple small notes

[chloekraw]

@chloekraw one updates, actually this patch will also fix vertical text dis-alignment if there are multiple font scales in one line. Please check my updated test result picture.

Thanks @zmiao, looks great. So to be clear, how did you fix this issue? Does this PR only fix vertical text misalignment if all text is CJK and uses the same font family?

[ansis]

Overall this looks good! I only have that one actual concern and a couple nits.

And a general question: It looks like a font range pbf shares one ascender and descender. Is this because a range never has multiple fonts in it?

[zmiao]

@asheemmamoowala

One question I have is whether this will affect local glyph generation adversely and by how much?

Good question. It won't affect individual glyph generation, it still uses the hardcoded value. But I am concerning that if it is possible for the following case:

• Only part of the glyphs for the fontstack could be generated locally, but remaining part of glyphs of the same fontstack needs to be requested from pbf data, then ascender and descender which are now fontstack level attributes will be messed up.

So from this point of view, ascender and descender still need to be glyph specific attributes.

[zmiao]

@ansis

And a general question: It looks like a font range pbf shares one ascender and descender. Is this because a range never has multiple fonts in it?

I cross checked the node-fontnik implementation mapbox/node-fontnik#160, for glyphs inside the same font-face, they share the same ascender and descender, but I will get confirmation that if a font-face equals to a new font type or not. Plus taking the concern from #8781 (comment), I think the safest way is still to put the ascender and descender as glyph specific attributes.

Do you agree about it? @ansis and @asheemmamoowala?

[zmiao]

@chloekraw

Thanks @zmiao, looks great. So to be clear, how did you fix this issue? Does this PR only fix vertical text misalignment if all text is CJK and uses the same font family?

This fix is working for mixed fonts. But to be honest, the vertical text misalignment is not fully fixed yet, if there are multiple lines inside the vertical text, and fonts are mixed, the look of the vertical shaped text is still not good.

My fix in this pr for the vertical dis-alignment is just replacing the hardcoded yOffset with the glyph offset when baseline is available. But still, vertical text shaping needs to be taken out as a separate issue and further fix which taking glyph's ascender and descender value into consideration is needed.

[asheemmamoowala]

Good question. It won't affect individual glyph generation, it still uses the hardcoded value. But I am concerning that if it is possible for the following case:

@zmiao Since local glyphs don't have these extra font metrics (baseline, ascender and descender) will the alignment between server generated and locally generated glyphs be worse that it is now?

[zmiao]

@asheemmamoowala

Since local glyphs don't have these extra font metrics (baseline, ascender and descender) will the alignment between server generated and locally generated glyphs be worse that it is now?

I talked with the team, so for the same font, all the glyphs shall be either local rasterized or fetched.

If it is local rasterized, for gl-native, the ascender and descender could be fetched from the platform dependent font API(Not implemented yet). But for gl-js, it is not possible. Right now both gl-native and gl-js will fall back to use Shaping::yOffset as a baseline. There will be a follow-up task in gl-native side to fetch ascender and descender from the font API for local rasterized glyphs. For gl-js, @tmpsantos wants to align with you at first and discuss about the possible solutions.

If it is fetched from server, then the ascender and descender value will directly be assigned from metadata got from the sever.

So all in all, it won't become worse, the worst case would just be using the hardcoded baseline, which is the same behavior before this fix.

[zmiao]

@ansis , updates about your question:

And a general question: It looks like a font range pbf shares one ascender and descender. Is this because a range never has multiple fonts in it?

yes, it is true. For the same fontstack, ascender and descender value will be global.

[ansis]

👍

[zmiao]

@ChrisLoer thank you very much for your comment, for your concerns:

Have you run perf benchmarks with the changes? I don't see anything that looks problematic, but there could be some surprises.

I run the benchmark tests, the result seems good:

Although this code has been in gl-native for a while, we haven't really exposed customers much to it yet, right? So how are we thinking about what their reaction will be if their map suddenly changes appearance because we went from guessing the baseline to actually loading it correctly from the font? In some cases they may have done something like added an offset to approximate the right baseline, or who knows what else. Is the idea just "this is a small change, it makes things overall better, some small tweaking to your map may be necessary?"

• The biggest change is about introducing the descender and ascender metrics, however, currently both server and local sides' glyphs don't contain these two metrics. So it won't bring any surprises for now. If both sever fonts and local fonts are used, as long as there is one font doesn't have descender and ascender metrics, we will still use the default baseline, which is the same as before, thus the glyphs rendering behavior will be almost the same for horizontal text.
  I also talked with @asheemmamoowala , the follow-up tasks would be 1. Investigate introducing font metrics for local fonts. 2. Preparation/test work to ensure the correct behavior with server fonts having ascender/descender, before we decide to land Expose ascender/descender metadata properties in protobuf node-fontnik#160. (Currently, I also add some render tests with glyph pbfs that containing the ascender/descender metrics generated with branch Expose ascender/descender metadata properties in protobuf node-fontnik#160)

• The other changes are mainly for fixing sevral text rendering issues(vertical text rendering, wrong position of text box with text-offset/text-rotate property), it won't affect the horizontal text's baseline. The texts will be rendered with a better look, so I assume the customers will accept them