-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Improve fallback font export tests #28784
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
FWIW, #20866 does appear to fix the EPS result. |
0129d60
to
4f6cf0e
Compare
Comparing in evince, the type 3 PDF glyphs are slightly cropped at the bottom vs the type42 file. But when @ksunden checked on macOS Preview, the whole glyphs changed thickness somewhat, so I'm going to chalk this up to renderer differences right now. |
4f6cf0e
to
1cff265
Compare
1cff265
to
9638129
Compare
I modified a couple other tests to use the same fonts/text, and removed a couple tests that seem to duplicate the others. |
Are we missing the CJK font checks now with the new test string? (It has a lot of accented characters, but not as many characters that require specific fonts) If the goal of the test is purely to test if fallback works, I gues this is probably enough, but do we also need to test the specific fallback for CJK characters? Because that feels like a specific usecase that is representative of real world usage. |
We are still checking Noto Sans CJK and WenQuanYi in I don't think CJK characters are sufficiently different from semi-Latin script (unlike e.g., Devanagari, etc.) in these cases. |
6844ee8
to
47fb508
Compare
Working on matplotlib#28765, I realized that we don't need an external font to test font fallback, as `cmr10`, and related, fonts have fairly low coverage compared to `DejaVu Sans`. Thus font fallback can easily be tested by starting with `cmr10`, and pulling additional characters from `DejaVu Sans`. In addition, this change increases unique characters in the figure to 491, which triggers some code paths for tables that are normally limited to 256 characters.
- matplotlib.tests.test_backend_pdf::test_embed_fonts is the same as `test_multi_font_type3` and `test_multi_font_type42` - matplotlib.tests.test_ft2font::test_font_fallback_chinese is the same as `test_fallback_smoke` except with Chinese fonts.
47fb508
to
921e49f
Compare
PR summary
Working on #28765, I realized that we don't need an external font to test font fallback, as
cmr10
, and related, fonts have fairly low coverage compared toDejaVu Sans
. Thus font fallback can easily be tested by starting withcmr10
, and pulling additional characters fromDejaVu Sans
.In addition, this change increases unique characters in the figure to 491, which triggers some code paths for tables that are normally limited to 256 characters.
The latter may be a bit overkill, but it shows that EPS is broken for a large set of unique characters, at least when viewed with evince. In that case, about half the bottom set of accented characters is missing, without any warnings produce. I have not yet determined the cause of this problem, but it looks like both type 3 and 42 use the
ttconv
extension to subset, though for large character sets, type 3 is forced to 42.It is possible that #20866 might be helpful here, but it would also be helpful if someone could confirm with a different EPS viewer.
PR checklist