Fix confusion between text height and ascent in metrics calculations.#31107
Fix confusion between text height and ascent in metrics calculations.#31107QuLogic merged 1 commit intomatplotlib:text-overhaulfrom
Conversation
lib/matplotlib/text.py
Outdated
| xys = M.transform(offset_layout) - (offsetx, offsety) | ||
|
|
||
| return bbox, list(zip(lines, zip(ws, hs), *xys.T)), descent | ||
| return bbox, list(zip(lines, wads, *xys.T)), descent |
There was a problem hiding this comment.
Do we still need to return descent now that we have d in wads?
There was a problem hiding this comment.
Good catch, it was anyways unused; removed.
|
You need to also update this call: matplotlib/lib/matplotlib/offsetbox.py Line 802 in d68c7e3
|
|
Oops, I missed that call site; fixed. |
|
Code looks good. Not checked, but I assume the image comparison failures are from slight text changes? What's the plan to handle them? |
|
I don't see anything obviously wrong in the code so far. This PR targets the There are 248 test image failures with this change, but they are a bit inconsistent in how they manifest. For example, in |
|
Agreed that it is hard to judge whether most changes (in non-multiline text) are really better or worse, except the patheffect legend (test_patheffect3) which appears clearly better (more vertically centered in the box) to me. old |
|
My read is that this makes the code match what the comments said it actually did. |
timhoffm
left a comment
There was a problem hiding this comment.
Looks good apart from the vauge naming.
|
I've pushed the image changes to the |
|
Test failures look like the |
The text height includes both the ascender and the descender, but the logic in _get_layout is that multiline texts should be treated as having an ascent at least as large as "l" and a descent at least as large as "p" (not a height at least as large as "lp" and a descent at least as large as "p") to prevent lines from bumping into each other (see changes to test_text/test_multiline, where the topmost superscript was close to bumping into the "p" descender previously).
Fix confusion between text height and ascent in metrics calculations.
This includes images changes for the following pull requests / commits: * [Fix center of rotation with rotation_mode='anchor'](matplotlib#29199) (c44db77) * [Remove ttconv backwards-compatibility code](matplotlib#30145) (8caff88) * [Remove kerning_factor from tests](matplotlib#29816) (7b4d725) * [Set text hinting to defaults](matplotlib#29816) (8255ae2) * [Update FreeType to 2.13.3](matplotlib#29816) (89c054d) * [Implement text shaping with libraqm](matplotlib#30000) (b0ded3a, 9813523) * [Add language parameter to Text objects](matplotlib#29794) (7ce8eae) * [Fix auto-sized glyphs with BaKoMa fonts](matplotlib#29936) (3ba2c13) * [pdf: Improve text with characters outside embedded font limits](matplotlib#30512) (b70fb88, 6cedcf7) * [Prepare `CharacterTracker` for advanced font features](matplotlib#30608) (8274e17, 70dc388, df670cf, ed5e074) * [Add font feature API to Text](matplotlib#29695) (972a688) * [Fix spacing in r"$\max f$"](matplotlib#30715) (4a99a83) * [Implement libraqm for vector outputs](matplotlib#30607) (bd17cd4) * [Drop the FT2Font intermediate buffer](matplotlib#30059) (9d7d7b4) * [Rasterize dvi files without dvipng](matplotlib#30039) (7627118) * [Update bundled FreeType and HarfBuzz libraries](matplotlib#30938) (a161658, 9619bcc) * [Fix positioning of wide mathtext accents](matplotlib#31069) (c2fa7ba) * [Refactor RendererAgg.draw_{mathtext,text,tex} to use same base algorithm](matplotlib#31085) (931bcf3) * [Implement TeX's fraction and script alignment](matplotlib#31046) (94ff452, 4bfa0f9, 1cd8510) * [Fix confusion between text height and ascent in metrics calculations](matplotlib#31107) (60f2310) * [mathtext: Fetch quad width & axis height from font metrics](matplotlib#31110) (692df3f, 383028b) * [mathtext: add mathnormal and distinguish between normal and italic family](matplotlib#31121) (a6913f3) * [ENH: Ignore empty text for tightbbox](matplotlib#31285) (d772043) * [Drop axis_artist tickdir image compat, due to text-overhaul merge](matplotlib#31281) (2057583) * [text: Use font metrics to determine line heights](matplotlib#31291) (3ab6a27, d961462, 97f4943) * [ps/pdf: Override font height metrics to support AFM files](matplotlib#31371) (e0913d4) * [TST: Cleanup back-compat code in tests touched by text overhaul](matplotlib#31295) (7c33379) * [TST: Set tests touched by text overhaul to mpl20 style](matplotlib#31300) (41c4d8d)
The text height includes both the ascender and the descender, but the logic in _get_layout is that multiline texts should be treated as having an ascent at least as large as "l" and a descent at least as large as "p" (not a height at least as large as "lp" and a descent at least as large as "p") to prevent lines from bumping into each other (see changes to test_text/test_multiline, where the topmost superscript was close to bumping into the "p" descender previously).
Partial fix for #21653; see #21653 (comment).
old
new
PR summary
PR checklist