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

Allow choosing CoreText shaper #144

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simoncozens
Copy link

@simoncozens simoncozens commented Feb 17, 2021

Requires harfbuzz/uharfbuzz#72, of course.

@justvanrossum
Copy link
Owner

Thanks, this looks good upon first glance, will review more thoroughly later.

@simoncozens
Copy link
Author

FontGoggles

@justvanrossum
Copy link
Owner

Lovely! Can you point me to the font and the test string?

@simoncozens
Copy link
Author

Not really, I'm afraid, for this font. Apparently the difference is fixed in the Big Sur version of CoreText anyway. You might find some CoreText/Harfbuzz shaping differences here: https://raw.githack.com/icu-project/text-rendering-tests/master/reports/CoreText.html#SHLANA-1

@justvanrossum
Copy link
Owner

I'm getting a hard segfault crash when I choose the CoreText shaper on a UFO. Could easily be a mistake on FG's side, but I haven't been able to figure it out yet.

@justvanrossum
Copy link
Owner

Do you mind if I rebase this PR and add a test that reproduces the crash? It may be easier to debug from there.

@khaledhosny
Copy link
Contributor

The uharfbuzz issue is fixed, I think this PR can be merged.

@justvanrossum
Copy link
Owner

It doesn't segfault anymore, but it still doesn't work for the sparse fonts that FG builds from UFOs. We need to figure out which tables CoreText can't do without. Might be as simple as adding a dummy OS/2 table or whatnot.

@khaledhosny
Copy link
Contributor

I think FontBook might show what Core Text thinks is wrong with such fonts. How can I get the binary font FG is building from UFO?

@justvanrossum
Copy link
Owner

justvanrossum commented Nov 2, 2021

I bet FontBook will complain about missing glyf or CFF tables, but that should not be the problem.

The "sparse" compile code is here:

def compileUFOToPath(ufoPath, ttPath):

@khaledhosny
Copy link
Contributor

This makes hb-shape with coretext shaper work for me. The dummy values should be OK, except for the metrics (coretext shaper does not use HarfBuzz font functions, so it always needs an hmtx table):

index 98aad12..58bced8 100644
--- a/Lib/fontgoggles/compile/ufoCompiler.py
+++ b/Lib/fontgoggles/compile/ufoCompiler.py
@@ -12,6 +12,7 @@ from fontTools.fontBuilder import FontBuilder
 from fontTools.ttLib import newTable
 from fontTools.ufoLib import UFOReader
 from fontTools.ufoLib.glifLib import _BaseParser as BaseGlifParser
+from fontTools.pens.ttGlyphPen import TTGlyphPen
 from ufo2ft.featureCompiler import FeatureCompiler


@@ -36,6 +37,12 @@ def compileUFOToFont(ufoPath):
         glyphOrder.insert(0, ".notdef")
     cmap, revCmap, anchors = fetchCharacterMappingAndAnchors(glyphSet, ufoPath)
     fb = FontBuilder(round(info.unitsPerEm))
+    pen = TTGlyphPen(None)
+    glyph = pen.glyph()
+    fb.setupGlyf({n: glyph for n in glyphOrder})
+    fb.setupNameTable({"psName": "TestFont"})
+    fb.setupHorizontalHeader(ascent=800, descent=-200)
+    fb.setupHorizontalMetrics({n: [1000, 0] for n in glyphOrder})
     fb.setupGlyphOrder(glyphOrder)
     fb.setupCharacterMap(cmap)
     fb.setupPost()  # This makes sure we store the glyph names

@justvanrossum
Copy link
Owner

justvanrossum commented Nov 2, 2021

When using CoreText, are callbacks like the one set with hb_font_funcs_set_glyph_h_advance_func() not used at all?

@khaledhosny
Copy link
Contributor

Yes, external shapers like CoreText basically get the font blob and are on their own after that.

@khaledhosny
Copy link
Contributor

(I don’t even think CoreText provide an API like this for HarfBuzz to use)

@justvanrossum
Copy link
Owner

Ok, makes sense, but that makes it far less interesting to support for live UFO previewing. Considering to just fall back to the HB-native shaper in this case.

@khaledhosny
Copy link
Contributor

I don't use the UFO functionality, so I have no idea what makes sense for it. But I often need to test different OpenType implementations when doing complex OTL and I need compiled fonts for this anyway, so limiting the CoreText shaper to compiled fonts is not worse than the status quo.

@justvanrossum
Copy link
Owner

Good, let's do it that way. It may be as simple as appendng the "ot" shaper to the shapers list as fallback.

Btw. I notice that the shaper is saved to the project file, but the popup is not set from the value in the project file upon reading.

@khaledhosny
Copy link
Contributor

That would be a bit misleading, I’d rather not allow selecting CoreText in this case.

@lianghai
Copy link

Hey guys, what should we do to advance this progress?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants