search

googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
152 stars 43 forks source link

Use new TTGlyphPointPen from fonttools #478

Closed jenskutilek closed 3 years ago

jenskutilek commented 3 years ago

... to avoid modification of TT outlines. Fixes #469.

jenskutilek commented 3 years ago

Depends on https://github.com/fonttools/fonttools/pull/2205.

anthrotype commented 3 years ago

we need to bump minimum required fonttools in setup.py

anthrotype commented 3 years ago

AttributeError: StubGlyph has no attribute "drawPoints"

https://github.com/googlefonts/ufo2ft/pull/478/checks?check_run_id=2608944622#step:5:1509

Some more specific tests would be nice

jenskutilek commented 3 years ago

What do you suggest should be tested? (which isn't covered by fonttools/TTGlyphPointPen or ufo2ft TrueType compiler branch)

anthrotype commented 3 years ago

You didn't need to change tests expectations after this patch (good, it means it continues to work) because none of the test ufo's glyphs contain off-curve points as starting points. How about, in outlineCompiler_test::OutlineTTFCompilerTest, you add a test that verifies that point list is not rotated even if a contour starts with off-curve point; e.g. you add a new glyph to the testufo (within the test code itself, not necessarily in the actual testufo on disk as that may affect other tests that use the same fixture) with a contour starting with an off-curve point and assert that the first point on the TTGlyph is still the same off-curve point.

anthrotype commented 3 years ago

do you mind also running black to fix the CI lint step. It's unrelated to your change (apparently black got pickier about docstrings), but it doesn't hurt to do it here

anthrotype commented 3 years ago

hm that's a lot of files. also not clear very much what "test_instructions" is really testing.. I was thinking more of you take the default testufo, add a glyph with starting off-curve (in the test python code itself, before you do the asserts) then check that the compiled glyph is as expected. A more tailored test, in other words. This one is basically like a whole intergration test

anthrotype commented 3 years ago

we'll definitely need an integration test for https://github.com/googlefonts/ufo2ft/pull/335, but for this I would like a unit test in outlineCompiler_test.py that checks that the resulting off-curve-starting glyph from OutlineCompiler is as expected

anthrotype commented 3 years ago

so annoying github wants me to approve the CI running each time you push a commit, otherwise they won't run automatically. I think it's to prevent bitcoin mining from first-time contributor. Oh well, I just made you a collaborator with write access, let's see if that works

jenskutilek commented 3 years ago

I have adapted a quadratic test UFO that will be added by #335 anyway. Here the compiled font has no hinting though, but in glyph o there is the offcurve start point that must be preserved.

It is very tricky, because even calling the preProcessor (in particular the CubicToQuadraticFilter) on an already quadratic UFO will change the start point to the next oncurve point. (This cost me a couple of debugging hours this morning...)

So testing the OutlineCompiler may not be enough.

anthrotype commented 3 years ago

calling the preProcessor (in particular the CubicToQuadraticFilter) on an already quadratic UFO will change the start point

yes, you want to disable that filter if you have a UFO with quadratic curves, there's an option for it

testing the OutlineCompiler may not be enough.

well, it tests that OutlineCompiler doesn't mess with the points anymore

anthrotype commented 3 years ago

This cost me a couple of debugging hours this morning

sorry about that. yes, quadratic ufos should avoid passing through cu2qu (which probably uses segment pen protocol so it rotates) and also disable reversing contour direction; if hinted, you should also disable removeOverlaps. But you figured that out already

jenskutilek commented 3 years ago

This cost me a couple of debugging hours this morning

sorry about that. yes, quadratic ufos should avoid passing through cu2qu (which probably uses segment pen protocol so it rotates) and also disable reversing contour direction; if hinted, you should also disable removeOverlaps. But you figured that out already

Hm, not sure, there maybe a bug: The Cu2QuPointPen uses a BasePointToSegmentPen, but it seems to restore the original start point, but maybe that doesn't work?

https://github.com/googlefonts/cu2qu/blob/main/Lib/cu2qu/pens.py#L166

Also, I thought adding com.github.googlei18n.cu2qu.curve_type = quadratic in the UFO might help, but that is only evaluated if compileTTF is called with inplace=True ...

jenskutilek commented 3 years ago

so annoying github wants me to approve the CI running each time you push a commit, otherwise they won't run automatically. I think it's to prevent bitcoin mining from first-time contributor. Oh well, I just made you a collaborator with write access, let's see if that works

Thanks, it works now :)

jenskutilek commented 3 years ago

I think this is ready to merge. Any objections @anthrotype?

anthrotype commented 3 years ago

i still think it's unnecessary to add such a huge set of test files just for testing this relatively small change from TTGlyphPen to TTGlyphPointPen, so I cherry picked your relevant commits and added a smaller test in #503 - please take a look and say "@googlebot I consent." if you're happy with me committing something on your behalf