Skip to content

Compute glyph widths similarly in Type 42 as in Type 3 #7961

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

Merged
merged 3 commits into from
Jan 29, 2017

Conversation

jkseppan
Copy link
Member

Here's my reasoning: Issue #7937 points out that text spacing is different between Type 42 and Type 3 embeddings of ttf fonts, and Type 3 looks better. One way to make them similar is to avoid the font cache and load a new FT2Font object, so I think the problem is that other users of these objects call set_size and that affects the glyph widths observed by this backend. Type 3 embeddings are not affected, and they use the LOAD_NO_SCALE flag, so here I change the Type 42 code to do the same and convert the sizes similarly.

In truth I have no clue how TrueType fonts work, so please review carefully.

@jkseppan jkseppan requested a review from mdboom January 27, 2017 13:19
@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Jan 27, 2017
@tacaswell tacaswell closed this Jan 27, 2017
@tacaswell tacaswell reopened this Jan 27, 2017
@tacaswell
Copy link
Member

guinea pig for new appveyor integration

@tacaswell
Copy link
Member

It seems the new job got fired (https://ci.appveyor.com/project/matplotlib/matplotlib) but the CI box is still showing the link to the old one. I assume this is just transient configuration getting crossed and the next PR will only file the new one (as I turned off the webhook to Mike's account).

Copy link
Member

@QuLogic 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 don't know where this 6 originated, but I came to the same conclusion earlier.

@@ -1040,8 +1040,9 @@ def embedTTFType42(font, characters, descriptor):
for c in characters:
ccode = c
Copy link
Member

Choose a reason for hiding this comment

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

The only suggestion I have is to clean this up; ccode can just be the name at the beginning of the loop.

@QuLogic
Copy link
Member

QuLogic commented Jan 28, 2017

Actually, was just thinking we should probably add a test for this.

@NelleV
Copy link
Member

NelleV commented Jan 28, 2017

👍 for a test

@NelleV NelleV changed the title Compute glyph widths similarly in Type 42 as in Type 3 [MRG+1] Compute glyph widths similarly in Type 42 as in Type 3 Jan 28, 2017
@NelleV NelleV added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 28, 2017
Adapted from the code samples in matplotlib#7937 by @TomDLT and @afvincent.
@jkseppan
Copy link
Member Author

Added a test case, but it can only test that the results of the type-42 embedding stay the same as they are after this change, not that they are correct.

@NelleV NelleV changed the title [MRG+1] Compute glyph widths similarly in Type 42 as in Type 3 [MRG+2] Compute glyph widths similarly in Type 42 as in Type 3 Jan 29, 2017
@NelleV NelleV merged commit 91e7425 into matplotlib:master Jan 29, 2017
@NelleV
Copy link
Member

NelleV commented Jan 29, 2017

Thanks @jkseppan !

NelleV added a commit that referenced this pull request Jan 29, 2017
Compute glyph widths similarly in Type 42 as in Type 3
@NelleV
Copy link
Member

NelleV commented Jan 29, 2017

backported in v2.0.x via 9b6f7c1

@QuLogic QuLogic changed the title [MRG+2] Compute glyph widths similarly in Type 42 as in Type 3 Compute glyph widths similarly in Type 42 as in Type 3 Jan 29, 2017
@jkseppan jkseppan deleted the issue7937 branch January 30, 2017 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: pdf Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants