Skip to content

Support scale in ttf composite glyphs #18081

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

Closed
wants to merge 7 commits into from

Conversation

jkseppan
Copy link
Member

@jkseppan jkseppan commented Jul 27, 2020

Fixes #17197

I added a test using Free Serif, which has plenty of composite characters such as arrows drawn by rotating other arrows.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/next_api_changes/* if API changed in a backward-incompatible way

@jkseppan jkseppan marked this pull request as ready for review July 27, 2020 13:36
@jkseppan
Copy link
Member Author

Interestingly, the test reveals that the Agg backend doesn't handle the font quite right.
ttconv_transforms

@jkseppan
Copy link
Member Author

Correct output, screenshot from a PDF viewer.

png

@jkseppan jkseppan force-pushed the ttconv-scale branch 2 times, most recently from e1d4421 to 8293741 Compare July 27, 2020 16:08
@anntzer
Copy link
Contributor

anntzer commented Jul 27, 2020

I'll trust you on the ttf parsing, but otherwise the patch looks reasonable.

@jkseppan jkseppan closed this Jul 27, 2020
@jkseppan jkseppan deleted the ttconv-scale branch July 27, 2020 16:47
@jkseppan jkseppan restored the ttconv-scale branch July 27, 2020 16:47
@jkseppan
Copy link
Member Author

Oops, made a typo with a push and accidentally closed this.

@jkseppan jkseppan reopened this Jul 27, 2020
@jkseppan
Copy link
Member Author

If someone wants to check the code against documentation, composite glyphs are described at https://docs.microsoft.com/en-us/typography/opentype/spec/glyf#composite-glyph-description and 2.14 fixed-point format at https://docs.microsoft.com/en-us/typography/opentype/otspec181/otff#data-types (search for F2DOT14).

@jkseppan jkseppan requested a review from anntzer July 29, 2020 11:25
anntzer
anntzer previously approved these changes Jul 29, 2020
@QuLogic
Copy link
Member

QuLogic commented Aug 1, 2020

Interestingly, the test reveals that the Agg backend doesn't handle the font quite right.

If I run outside the test framework, it appears to be okay?

@jkseppan
Copy link
Member Author

jkseppan commented Aug 1, 2020

Interestingly, the test reveals that the Agg backend doesn't handle the font quite right.

If I run outside the test framework, it appears to be okay?

Yes, I couldn't reproduce this. It might have been some problem with an intermediate version, as I might have forgotten to compile the extension when running some test.

@QuLogic
Copy link
Member

QuLogic commented Aug 1, 2020

Interestingly, the test reveals that the Agg backend doesn't handle the font quite right.

If I run outside the test framework, it appears to be okay?

Yes, I couldn't reproduce this. It might have been some problem with an intermediate version, as I might have forgotten to compile the extension when running some test.

To clarify, I can reproduce if running in the test suite, by removing the explicit extension from the comparison settings.

This is because tests disable hinting with plt.rcParams['text.hinting'] = 'none'; if you run the script outside the test suite, but add that, then you get the weird streaks. Any setting other than 'force_autohint' or 'auto' (same thing to FreeType, latter is what we call our default) will do it, so it seems to be something wrong in the font hinting itself.

@jkseppan
Copy link
Member Author

jkseppan commented Aug 1, 2020

The test failures are because of unrelated Pandas deprecations; apparently Pandas 1.1.0 is now available

@QuLogic
Copy link
Member

QuLogic commented Aug 1, 2020

This is already fixed on master; you should rebase if you're having trouble with it.

The GNU Free Serif font has a wide variety of characters with which
we can exercise the linear transforms used in TTF composite glyphs.
These could increase the file length in case of fonts with
many composite glyphs.
@QuLogic
Copy link
Member

QuLogic commented Aug 4, 2020

It looks like you may have accidentally reverted some things in that last rebase?

@QuLogic QuLogic added this to the v3.4.0 milestone Aug 4, 2020
jkseppan and others added 2 commits August 5, 2020 09:44
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@jkseppan
Copy link
Member Author

jkseppan commented Aug 5, 2020

It looks like you may have accidentally reverted some things in that last rebase?

You're right, it looks like I lost the suggestions committed via the Github web UI. I committed those again.

@anntzer
Copy link
Contributor

anntzer commented Aug 5, 2020

See #18181 for an alternate solution, though (which can reasonably avoid adding a new font file in the repo, and in any case seems more robust in the long term, as it can Type3-subset any font that FreeType can outline-decompose).

@anntzer anntzer dismissed their stale review August 5, 2020 10:24

Still approving, but perhaps make a decision wrt #18181 first.

@jkseppan jkseppan closed this Aug 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing character upon savefig() with Free Serif font
3 participants