Fix text kerning calculations and some FT2Font cleanup #14940
Conversation
| @@ -1285,7 +1285,8 @@ def is_opentype_cff_font(filename): | |||
| def get_font(filename, hinting_factor=None): | |||
| if hinting_factor is None: | |||
| hinting_factor = rcParams['text.hinting_factor'] | |||
| return _get_font(os.fspath(filename), hinting_factor) | |||
| return _get_font(os.fspath(filename), hinting_factor, | |||
| _kerning_factor=rcParams['text.kerning_factor']) | |||
There was a problem hiding this comment.
What about folding this into rcParams['_internal.classic_mode']? (how many images would need to be regen'd?)
There was a problem hiding this comment.
Well, I didn't update the non-classic tests yet, so you can see from the CI results it'd be 21 failures.
There was a problem hiding this comment.
Had a quick look at the failures. A few of them can be improved:
- regen test_acorr, test_image_interps without any text (remove_text)
- test_titletwiny can be rewritten to a non-image-comparison, just checking the title position, like test_titlesetpos which is just below it
- for test_hatching the text labels can probably removed? (can we generate legends with empty labels? not sure...)
- I think the others images would need to be regen'd (assuming we fold the switch into _internal.classic_mode) and need to keep the text to be meaningful.
Or perhaps make this a private rcParam (leading underscore, like _internal.classic_mode)? Or perhaps I'm just overthinking this and text.kerning_factor is fine.
There was a problem hiding this comment.
Well, I added the rcParam for downstream testing's benefits; not sure if only classic mode will be helpful there.
There was a problem hiding this comment.
Sure. I guess my preferred solution would be _internal.old_kerning or something like that, but won't insist on it.
There was a problem hiding this comment.
I opened #15054 to track these suggestions, but for tests here I just modified the rcParam to use the old value.
|
Does this need further discussion? How many images will have to be rebuilt, and can this be done at the same time as other tests that need an image rebuild? |
|
The only question remaining is what to name the rcParam. With that, we can allow classic tests to continue working (~100 tests) and then the remaining ones can patch the parameter to avoid regenerating results (16 tests). Then they can be removed piece-by-piece as images are regenerated, or all at once with the larger font fixes. |
|
Given that there's already text.hinting_factor which should be considered a relic of the past, text.kerning_factor looks fine to me. |
|
Rebased for fixed docs and added a what's new entry. |
|
@tacaswell any reason to wait on this now that it's passing? |
|
Seems like a pity to wait for 3.3 so I remilestoned this to 3.2. @tacaswell please ping if you think this should wait, otherwise I'll merge. |
For the straight `get_kerning` call, the result should be in subpixels, since the callers (in Python) correctly scale it by 64 themselves. For the glyph advancement calculation, both `pen` (used for glyph transform and with content boxes) and kerning are in 26.6 fractional pixels. Therefore, there's no need to scale either case by 2**6 (64).
This is enabled for the classic test style, so that the 100 or so old images do not need to be regenerated.
There was a problem hiding this comment.
doc failure is unrelated
@tacaswell feel free to remilestone if desired
PR Summary
Kerning values were incorrectly divided by 64 (possibly some confusion from FreeType using both 26.6 and 16.16 types) rendering them effectively 0, unless used with very large fonts.
This corrects that error, but also adds an rcParam (
text.kerning_factor) to restore the old behaviour. This is set to the old value in the_classic_teststyle to allow ~100 test images to continue working. There are about 20 more tests using the new style that fail now. If this looks like a reasonable way to write out the rcParam, I will add a line in those tests so their images do not need to be regenerated.Fixes #14887.
PR Checklist