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

[buildcff2vf] Major rewrite for sparse font support #792

Merged
merged 10 commits into from
May 15, 2019

Conversation

readroberts
Copy link
Contributor

  • support sparse source fonts
  • subset full masters to source source fonts from subset definition file
  • check and fix flat curve and closepath outline incompatibilities
  • use argparse for arguments.

Copy link
Contributor

@cjchapman cjchapman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you've got test failures too, so I'll wait until you've addressed those and these two issues, and then I'll be happy to review this in more detail after that.

python/afdko/buildcff2vf.py Outdated Show resolved Hide resolved
python/afdko/buildcff2vf.py Outdated Show resolved Hide resolved
@readroberts
Copy link
Contributor Author

I will be pushing incremental updates to this branch for the next while - don't bother reviewing until I notify you that I am done.

@readroberts readroberts force-pushed the update-buildcff2vf branch 2 times, most recently from 678ba6b to 8b985ff Compare May 3, 2019 03:41
change options to require an option arg for each path parameter, and change the default from keeping glyph names ( with 'post' table format 2) to not keep glyph names (with 'post' table format 3.)
…ce CFF tables into a VF CFF2 table. This now all happens within the call to varLib.build().

Provided a new function for adding glyph names to the CF font.
Updated expected output ttx file. This differs from the prior version because:
- fontTools rounds the delta values from real to integer in the CFF2 CharStrings
- fontTools does a better job of optimizing CFF2 CharStrings

I verified that the old and new VF were functionally equivalent by manual inspection of two charstrings, and by writing a test script which did a 'tx -dump -6' of the old and VF fonts for all the source font design space coordinates , and for the coordinate (0.5,-0.5). The only differences were all in the same line segment in the same glyph. The difference was because fontTools reduces to co-linear line segments to one line segment.
…o fix common simple problems caused by charstring optimization:

- convert line segments to flat curves
- insert missing close-path line-segment when needed.
This happens in our CJK font sources.

Since it is hard to determine the designer's intentions when converting a line to a curve, I used a simple logic, and emit a warning that the segment should be inspected. The logic just sets each BCP to be 1/3 the length of the line segment.
…is specific to each font.

This is useful for workflows where the workflow builds a complete glyph set for each source font, even though not all glyphs use the same set of design axes or source fonts. An example case is SourceHanSans, where only 55 glyphs use more than two axes, and many of the 55 use different axes than the other special glyphs.
built  by fontTools within varLib.build()
miguelsousa
miguelsousa previously approved these changes May 10, 2019
Copy link
Member

@miguelsousa miguelsousa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@readroberts thanks for breaking up the changes.
@cjchapman please have another look.

python/afdko/buildcff2vf.py Outdated Show resolved Hide resolved
python/afdko/buildcff2vf.py Show resolved Hide resolved
…ndentation error that would cause a bug, and a redundant if clause.
@readroberts
Copy link
Contributor Author

@miguelsousa @cjchapman Requested changes made - please check.

On another issue, the builds are currently failing because a) the build scripts are installing fontTools 3.41.2, and b) in version 3.41.2, fontTools changed to keep glyph names in the 'post' table by default. This baffles me - the requirements.txt file for the update-buildcff2vf branch require fontTools 3.41.0. How come the requirements file for the latest node on 'develop' is being used?

Of course, what is needed is to change buildcff2vf to work with 3.41.2. This requires some work - changing buildcff2vf to suppress glyph names rather than add them is trivial, but I think the fontTools has a bug in adding glyph names when the sources are CID fonts. Am looking into this.

@cjchapman cjchapman dismissed their stale review May 14, 2019 23:23

I see that the issues noted in this review have been fixed, but build issues remain, so I am choosing "dismiss review" rather than "approve changes".

@anthrotype
Copy link
Member

Oops, i forgot about CID... 😬
@readroberts can you please send a PR to fix that? Thank you

… to 3.41.2

fontTools changed to keep postscript glyph names in the 'post' table by default. Change in buildcff2vf is to suppress rather than add postscript glyph names
@readroberts
Copy link
Contributor Author

@anthrotype @msousa @cjchapman Turns out there is no problem in fontTools with CID fonts - I was just suffering from versionitis - I haven't fully grasped when I need to re-install fontTools to have the latest sources take effect, when I have in installed in editable developer mode ( pip install -e .[...]).
Updated buildcff2vf to suppress postscript names, rather than add them, when appropriate.

@miguelsousa miguelsousa merged commit 168efdf into develop May 15, 2019
@miguelsousa miguelsousa deleted the update-buildcff2vf branch May 15, 2019 19:31
@miguelsousa miguelsousa mentioned this pull request May 15, 2019
3 tasks
@miguelsousa miguelsousa mentioned this pull request May 15, 2019
4 tasks
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