Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix text kerning calculations and some FT2Font cleanup#14940
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
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about folding this intorcParams['_internal.classic_mode']? (how many images would need to be regen'd?)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I opened#15054 to track these suggestions, but for tests here I just modified thercParam 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? |
QuLogic commentedAug 12, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 awhat's new entry. |
@tacaswell any reason to wait on this now that it's passing? |
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.
In general the changes seem fine, but I'm no freetype expert, so I'm not inclined to be the one to push the button. 😉
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. |
Uh oh!
There was an error while loading.Please reload this page.
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 glyphtransform and with content boxes) and kerning are in 26.6 fractionalpixels.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 oldimages do not need to be regenerated.
anntzer left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
doc failure is unrelated
@tacaswell feel free to remilestone if desired
Uh oh!
There was an error while loading.Please reload this page.
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