Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Font 42 kerning#20615
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
Font 42 kerning#20615
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This commit adds an image comparison test to ensure that the text in a type 42font has proper kerning in the PDF output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good!
| Returns True if the font is able to provided the char in a PDF | ||
| For a Type 3 font, this method returns True only for single-byte | ||
| chars. For Type 42 fonts this method always returns True. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should this be explicit in the code and should we raise for other fonttypes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We already have rcParam checks that disallow other fonttypes..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, I've learned to be catious. If somebody implements a new fonttype, they may overlook this function. So, forcing future font types to make an explicit decision here is a plus (Unless you say that the default should generally be True and the Type-3 handling is a rare exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
At the moment, the function is only called in case of a Type 3 or 42, but I agree it's probably safer to raise aNotImplementedError (?) if the font type is neither.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You could imagine an object-oriented solution where the different types of fonts are different classes, and this would be a method on the font. But that would likely be unnecessary overengineering. Perhaps theNotImplementedError is sufficient defence against future accidents.
| def test_pdf_font42_kerning(): | ||
| plt.rcParams['pdf.fonttype'] = 42 | ||
| plt.figure() | ||
| plt.figtext(0.1, 0.5, "ATAVATAVATAVATAVATA", size=30) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this the only kerning you were fixing? If not, maybe some of the other kerning pairs would be useful here rather than repeating the same one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You mean pairings of glyphs? I'd hope the font defines kerning correction for more pairs. I chose this string because there the effect is obvious even without a pixel-by-pixel comparison. For this specific issue, the test targets the presence ofany kerning. However, I will modify the string in the next iteration.
Or, are you more worried about single-byte vs multi-byte vs beyond-BMP character pairings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, if you are sure this tests for any kerning and we don't need to test other kerning pairs, thats fine. (I have no idea what the different pairings are technically ;-))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The kerns come from_text_helpers.layout which basically just callsfont.get_kerning(prev_glyph_idx, glyph_idx, kern_mode) repeatedly. I'm convinced by this test - if the AV spacing was bad previously and good after this change, it means that kerns are now getting inserted where they weren't previously. Multibyte characters could plausibly still be missing kerns, but if so, I think it's fine to call it a separate issue.
The commit applies the same kerning algorithm used for Type 3 fonts also to Type42 fonts. A string is split into chunks with kerning between the chunks.
Thanks@sauerburger ! |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
This PRcloses#20614 by applying the kerning algorithm already employed for Type 3 fonts also for Type 42 fonts.
For Type 3 fonts, text is broken into chunks to support multibyte chars and to adjust the kerning between chunks. However, previously, a single
Tjcommand was used in the case of Type 42 fonts which results in a text output without kerning.With the PR, the same chunking algorithm is used also on Type 42 text. The new method
_font_supports_charis used to decide whether to split the string on an unsupported multibyte char. For Type 42 fonts, the chunks are only used to apply kerning corrections. Multibyte chars are directly referenced without XObjects.There is no change for Type 3 fonts.
I'm not even sure if we need the simple
Tjcase without kerning. Maybe for Type 1 fonts?PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).