NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

Replace ttconv by plain python for pdf subsetting#18181

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

jklymak merged 2 commits intomatplotlib:masterfromanntzer:unttpdf

Aug 13, 2020

Merged

Replace ttconv by plain python for pdf subsetting#18181

jklymak merged 2 commits intomatplotlib:masterfromanntzer:unttpdf

Aug 13, 2020

Conversation

Copy link

Contributor

anntzer commentedAug 5, 2020•
edited
Loading

PR Summary

First commit:

Switch from a hand-written glyph outline decomposer to FreeType's one.

Instead of walking through FT_Outline ourselves, use FreeType's
FT_Outline_Decompose to decompose glyphs into paths.

Fixes incorrect positioning of first and last control points of certain
contours; compare e.g.

frompylabimport*frommatplotlib.textpathimportTextPathfrommatplotlib.patchesimportPathPatchrcParams["mathtext.fontset"]="stixsans"path=TextPath((0,0),r"$\delta$",size=72)plot(*path.vertices.T,".-",lw=.5)gca().add_patch(PathPatch(path,alpha=.5))gca().set(aspect="equal")show()

before and after the patch (before the patch, the inner loop was not
properly closed). (Technically, I believe the earlier bug affected
start/end-of-contours that are implicitly created per the ttf format at
the midpoint between two conic control points.)

before:

after:

A few SVG files need to be updated too.

Second commit:

Stop using ttconv for pdf.

Quite a bit of work goes into reproducing quirks of the old ttconv code
in order not to update baseline images, but this can easily be dropped
once the new baseline images system goes in.

figure_align_label.pdf gets modified because of tiny differences in
outline extraction (due to the ttconv emulation); there's already the
svg test that tests the fixed-dpi case so I feel OK deleting it.

This also fixes ttc subsetting for pdf output (cf. changes in
test_font_manager.py).

(PostScript is not done yet, but it's basically the same idea, it's just the header that's different.)

Fixes#17197. I'll skip adding a new font for the test, because in essence we're "just" relying on FreeType handing us the right outlines.

PR Checklist

Has Pytest style unit tests
Code isFlake 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

anntzer mentioned this pull request

Aug 5, 2020

Support scale in ttf composite glyphs#18081

Closed

6 tasks

anntzer force-pushed theunttpdf branch 5 times, most recently from1da1a4c to4a488faCompare

August 5, 2020 12:40

anntzer added backend: pdf topic: text/fonts labels

Aug 5, 2020

Copy link

Member

QuLogic commentedAug 7, 2020

Instead of walking through FT_Outline ourselves, use FreeType's
FT_Outline_Decompose to decompose glyphs into paths.

Cool, I wanted to do that, except for it breaking images, like you've found.

QuLogic reviewed

Aug 7, 2020

View reviewed changes

Copy link

Member

QuLogic left a comment

Choose a reason for hiding this comment

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

I didn't completely finish the second commit yet, though it seems good.

src/ft2font.cpp OutdatedShow resolvedHide resolved

src/_path.h Outdated

Comment on lines 1111 to 1113

		while (q >= str && *q == '0') { --q; }
		// If the end is a decimal point, delete that too
		if (q >= str && *q == '.') { --q; }

Copy link

Member

QuLogicAug 7, 2020

Choose a reason for hiding this comment

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

Stylistically, this might be against PEP7.

Copy link

ContributorAuthor

anntzerAug 7, 2020

Choose a reason for hiding this comment

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

I know, but I think splitting things on their own lines (as was done before) makes the code really harder to follow because the information density is so low...

Copy link

ContributorAuthor

anntzerAug 9, 2020

Choose a reason for hiding this comment

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

I went for a slight variation which does respect pep7.

anntzer force-pushed theunttpdf branch 2 times, most recently from586edd9 todd0482bCompare

August 7, 2020 09:37

jkseppan approved these changes

Aug 8, 2020

View reviewed changes

Copy link

Member

jkseppan left a comment

Choose a reason for hiding this comment

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

Nice! Looks like much of the complexity is for bug-compatibility with ttconv, which I hope we can remove later.

Switch from a hand-written glyph outline decomposer to FreeType's one.

4469385

Instead of walking through FT_Outline ourselves, use FreeType'sFT_Outline_Decompose to decompose glyphs into paths.Fixes incorrect positioning of first and last control points of certaincontours; compare e.g.```pythonfrom pylab import *from matplotlib.textpath import TextPathfrom matplotlib.patches import PathPatchrcParams["mathtext.fontset"] = "stixsans"path = TextPath((0, 0), r"$\delta$", size=72)plot(*path.vertices.T, ".-", lw=.5)gca().add_patch(PathPatch(path, alpha=.5))gca().set(aspect="equal")show()```before and after the patch (before the patch, the inner loop was notproperly closed).  (Technically, I believe the earlier bug affectedstart/end-of-contours that are implicitly created per the ttf format atthe midpoint between two conic control points.)A few SVG files need to be updated too.

anntzer force-pushed theunttpdf branch 2 times, most recently fromd81d35b to385894bCompare

August 9, 2020 22:13

QuLogic reviewed

Aug 11, 2020

View reviewed changes

src/_path.h

		--c;
		}
		try {
		buffer.append(str, c + 1);

Copy link

Member

QuLogicAug 11, 2020•
edited
Loading

Choose a reason for hiding this comment

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

Second parameter is size, isn't it? Or is cppreference wrong, since it seems to compile...

Suggested change

	buffer.append(str,c+1);
	buffer.append(str,c-str+1);

Copy link

ContributorAuthor

anntzerAug 12, 2020

Choose a reason for hiding this comment

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

This is using the

template< class InputIt >basic_string& append( InputIt first, InputIt last );

overload, not the

basic_string& append( const CharT* s, size_type count );

one.

lib/matplotlib/backends/backend_pdf.py OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

anntzer commentedAug 12, 2020

One thing I forgot to mention is that the Py_DTSF_ADD_DOT_0 change also fixes a separate bug in__add_number, whereby passingprecision = 0 would previously print values without the dot and then trailing but significant zeros would be incorrectly stripped out. This doesn't affect the patch here because this uses the separate bug-compat precision=-1 path, but the (future) backend_ps patch will use precision=0 and thus benefit from this.

Stop using ttconv for pdf.

19359c1

Quite a bit of work goes into reproducing quirks of the old ttconv codein order not to update baseline images, but this can easily be droppedonce the new baseline images system goes in.figure_align_label.pdf gets modified because of tiny differences inoutline extraction (due to the ttconv emulation); there's already thesvg test that tests the fixed-dpi case so I feel OK deleting it.This also fixes ttc subsetting for pdf output (cf. changes intest_font_manager.py).

anntzer force-pushed theunttpdf branch from385894b to19359c1Compare

August 12, 2020 22:40

QuLogic approved these changes

Aug 13, 2020

View reviewed changes

Copy link

Member

QuLogic commentedAug 13, 2020

Should we wait for the baseline image PR before merging this?

Copy link

ContributorAuthor

anntzer commentedAug 13, 2020

I put in all the bug-compat code explicitly to minimize the image diffs; I'd say the remaining svg diffs are nottoo large, but I'm not in a big hurry either.