Expose ascender/descender metadata properties in protobuf

[tristen]

Pull what's applicable from #97 into this commit.

[springmeyer]

The branch is passing now on travis since I fixed this minor issue (#161) and merged that fix into this branch. The next steps I see for this work are:

• Ensure that the values are correct and what are needed
  • Would require testing some glyphs inside MBGL and adapting to the new data
• Based on MBGL testing, write more tests here as needed to assert the right data is being provided and won't regress
• Ensure this change is supported in fontstack compositing. Two options:
  • Option 1: Port https://github.com/mapbox/glyph-pbf-composite to understand these new fields (by updating the duplicated glyphs.proto at https://github.com/mapbox/glyph-pbf-composite/blob/master/proto/glyphs.proto)
    • Update downstream deps that use glyph-pbf-composite like https://github.com/mapbox/fontmachine
  • Option 2: Port glyph compositing into node-fontnik to keep all glyph write logic in one place. I've put forth a POC of this at Node v12 + protozero port + additional beta compositing support #163
    • Update downstream deps like https://github.com/mapbox/fontmachine to use fontnik compositing instead of glyph-pbf-composite
• Discuss back compatibly in across service: a) do we update all fonts to have this metadata or do we write opt-in logic in all our renderers to adapt to this information, when it exists in the pbf?

[zmiao]

Hi, I am currently testing the generated glyphs from this pr in MBGL, I am trying to fix the dis-alignment of texts with mixed fonts (Noto Sans CJK and Arial Unicode MS Regular). In order to fix it, I need to shift back the top value in the metrics in MBGL. Since the top value was generated from:
double top = static_cast<double>(glyph.top) - glyph.ascender;
In order to do so, I need the ascender in the metrics to be 26.6 pixel format instead of unscaled font units.
So I think ascender and descender are better to be set from ft_face->size->metrics.ascender and ft_face->size->metrics.descender.

[tristen]

👋 @zmiao excited to learn you are digging into this work!

So I think ascender and descender are better to be set from ft_face->size-metrics.ascender and ft_face->size->metrics.descender.

Feel free to make those modifications to this pull request if that's more accurate!

[zmiao]

@tristen thanks for the reply, I updated this pr.

[zmiao]

Some experimenting rendering results after applying the ascender value in MBGL for fixing the dis-alignment of mixed fonts:

After fix	Before fix

Next step for testing would be trying to figure out how to deal with issues in mapbox/mapbox-gl-js#8560

[zmiao]

@tristen @springmeyer
in the source code

node-fontnik/src/glyphs.cpp

Lines 297 to 310 in d65c204

	for (int i = 0; ft_face == 0 || i < num_faces; ++i) {
	ft_face_guard face_guard(&ft_face);
	FT_Error face_error = FT_New_Memory_Face(library, reinterpret_cast<FT_Byte const*>(baton->font_data), static_cast<FT_Long>(baton->font_size), i, &ft_face);
	if (face_error) {
	baton->error_name = std::string("could not open font");
	return;
	}
	if (num_faces == 0) {
	num_faces = ft_face->num_faces;
	}
	if (ft_face->family_name) {
	llmr::glyphs::fontstack* mutable_fontstack = glyphs.add_stacks();

, each font-face equals to a new fontstack in the pbf. So what does font-face mean? Does it imply a new font type?

I am confused because that I tried to use the executable build-glyphs to generate the pbf data from one .ttc file that containing multiple fonts, but finally the output is still the general ranges pbf, nothing is font related. Did I miss something, how can I generate font-specific pbf data if the *.ttc has multiple fonts inside? For example, the NotoSansCJK.ttc in https://www.google.com/get/noto/help/cjk/ has 36 OTF font files in total.

[springmeyer]

👋 @zmiao thanks for asking. This was also a confusion for me when I first considered .ttc behavior. My understanding is this:

• The node-fontnik code that handles reading faces was originally ported from code from mapnik, which supports .ttc
• But the ttc support in node-fontnik was never tested
• And downstream libraries on top of node-fontnik explicitly do not support ttc
• So, its unclear to what extent the ttc support in node-fontnik is working or not

A little bit of this history is evident in #125

So, overall, to support ttc would need someone to comprehensively test and perhaps fix issues in the entire dependency chain: node-fontnik -> fontmachine -> core-fonts -> api-styles -> mapbox-gl. Because several of those repos are private, if you have questions about them it would make most sense to discuss more in tickets at those repos.

[zmiao]

@springmeyer Thanks for the explanation, it is really helpful.

So based on your comments, right now we do not support generating pbf from multi-face ttc, but the weird thing is if I upload the ttc file via MapboxStudio, I could get multiple fonts, how did it work? I assume MapboxStudio uses the same node-fontnik engine, right?

[springmeyer]

Interesting @zmiao - I would have assumed you'd get an error due to this line: https://github.com/mapbox/fontmachine/blob/0e5eaab3fd99153083f08278d7a55792d2b6f3ce/index.js#L32

I assume MapboxStudio uses the same node-fontnik engine, right?

Yes, the backend of Mapbox Studio uses node-fontnik@0.5.3

[zmiao]

@springmeyer It was my bad. Actually for my case, I uploaded one otf file, but I got two fonts without any error prompt, I am wondering why it works?

And also if I use bin/build-glyphs to generate pbf from ttc file, I didn't get any errors neither, just have to wait for a very long while.

[tmpsantos]

Hi all, this has been stalled for a while. Do we have plans to merge it?