Unify text layout paths. #14872
Unify text layout paths. #14872
Conversation
| gind = font.get_char_index(ccode) | ||
| for char_idx, newx in _text_layout.layout( | ||
| chunk, font, x0=newx, kern_mode=KERNING_UNFITTED): | ||
|
|
||
| if mode == 2 and chunk_type == 2: |
There was a problem hiding this comment.
In this section, the newx would originally not use kerning, while it would with this new layout, no? I'm surprised there are no errors.
There was a problem hiding this comment.
the previous version did use kerning, per newx += kern / 64 + glyph.linearHoriAdvance / 65536
There was a problem hiding this comment.
Yes, I forgot it's only used for intra-character adjustments, so it would take effect after the character at play.
There was a problem hiding this comment.
Actually, coming back to this again, the new code applies kerning of this-and-last character before yielding, whereas the PDF backend here applies kerning of this-and-last character after using the position (newx) it's found. This seems like a bug in the original, though...
There was a problem hiding this comment.
I just rewrote the whole thing in #14886...
... i.e. the logic for advancing one character at a time, positioning that character and taking kerning between consecutive characters into account. Such a factoring would also be useful if we ever get to use a text layout engine that supports right-to-left languages (we'd just need to fix `layout` (and the C-version used by agg)).
|
Actually looking at it again I think the new version for pdf fails to take inter-chunk kerning into account :-( (probably this was never tested) If you agree with that analysis, I'll just revert the changes to backend_pdf for now. |
|
|
|
Ah, ok, so it's not worse than before... |
|
Maybe open a separate issue for that, then? |
|
working on fixing that, but that'll be a separate pr |
|
Ugh, and there's no coverage link again. |
|
For some reason codecov very often fails to upload coverage for my PRs (even though they're done, here it's at https://codecov.io/gh/matplotlib/matplotlib/pull/14872), do you have any possible idea why? |
|
If you look at the commit, it says it's still waiting for CI, though it's received all the CI notifications that it would have on a working build. I pushed the button to send notifications, so maybe it'll show up now. |
PR Summary
... i.e. the logic for advancing one character at a time, positioning
that character and taking kerning between consecutive characters into
account.
Such a factoring would also be useful if we ever get to use a text
layout engine that supports right-to-left languages (we'd just need to
fix
layout(and the C-version used by agg)).PR Checklist