Skip to content
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

Port fontconfig's font weight detection to font_manager. #16203

Merged
merged 3 commits into from May 28, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 13, 2020

PR Summary

This (should) fix the various font weight detection problems (#4822 #8551 #8607 #8788 #8800) by reusing fontconfig's font weight detection algorithm (specifically https://gitlab.freedesktop.org/fontconfig/fontconfig/blob/452be81/src/fcfreetype.c#L1859 https://gitlab.freedesktop.org/fontconfig/fontconfig/blob/452be81/src/fcfreetype.c#L1928 https://gitlab.freedesktop.org/fontconfig/fontconfig/blob/452be81/src/fcfreetype.c#L2006 https://gitlab.freedesktop.org/fontconfig/fontconfig/blob/452be81/src/fcfreetype.c#L2044).

The first commit is a straight port of fontconfig's algorithm, and outputs fontconfig weights; the second commit reverts to using CSS/OpenType weights, which is what we were using so far (see discussion in #10249).

To test this, delete your last fontlist-v310.json and regen one by importing matplotlib.font_manager with or without this PR and setting the PYTHONHASHSEED envvar to a fixed value -- this ensures that the json dumps stay in the same order, as we use sets, whose order depend on the hash seed.

(If we agree on the PR I'll need to bump the fontlist version number as well.)

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a straightforward conversion, except for one thing.

lib/matplotlib/font_manager.py Outdated Show resolved Hide resolved
lib/matplotlib/font_manager.py Show resolved Hide resolved
@QuLogic
Copy link
Member

QuLogic commented May 22, 2020

I tried with Fira Sans, which has a bunch of styles, and it went (heaviest to lightest) from:

FiraSans-Black black 900
FiraSans-BlackItalic black 900
FiraSans-Bold 700
FiraSans-BoldItalic 700
FiraSans-ExtraBold bold 700
FiraSans-ExtraBoldItalic bold 700
FiraSans-SemiBold semibold 600
FiraSans-SemiBoldItalic semibold 600
FiraSans-Medium medium 500
FiraSans-MediumItalic medium 500
FiraSans-Regular regular 400
FiraSans-Italic 400
FiraSans-Thin 400
FiraSans-ThinItalic 400
FiraSans-Light light 200
FiraSans-LightItalic light 200
FiraSans-ExtraLight light 200
FiraSans-ExtraLightItalic light 200

to:

FiraSans-Black 900
FiraSans-BlackItalic 900
FiraSans-ExtraBold 800
FiraSans-ExtraBoldItalic 800
FiraSans-Bold 700
FiraSans-BoldItalic 700
FiraSans-SemiBold 600
FiraSans-SemiBoldItalic 600
FiraSans-Medium 500
FiraSans-MediumItalic 500
FiraSans-Regular 400
FiraSans-Italic 400
FiraSans-Light 300
FiraSans-LightItalic 300
FiraSans-ExtraLight 275
FiraSans-ExtraLightItalic 275
FiraSans-Thin 250
FiraSans-ThinItalic 250

This is certainly better in terms of distinct values, and it matches the order shown on Google Fonts. However, Google Fonts calls ExtraLight 200, and Thin 100, and I think, so does fontconfig.

@anntzer
Copy link
Contributor Author

anntzer commented May 23, 2020

This is what I get:

python -c 'from matplotlib.font_manager import ft2font, ttfFontProperty; from pathlib import Path; [print(path, ttfFontProperty(ft2font.FT2Font(str(path))).weight) for path in Path("/usr/share/fonts/TTF").glob("FiraSans-*.ttf")]' | sort -n -k2
/usr/share/fonts/TTF/FiraSans-EightItalic.ttf 100
/usr/share/fonts/TTF/FiraSans-Eight.ttf 100
/usr/share/fonts/TTF/FiraSans-FourItalic.ttf 100
/usr/share/fonts/TTF/FiraSans-Four.ttf 100
/usr/share/fonts/TTF/FiraSans-HairItalic.ttf 100
/usr/share/fonts/TTF/FiraSans-Hair.ttf 100
/usr/share/fonts/TTF/FiraSans-ThinItalic.ttf 100
/usr/share/fonts/TTF/FiraSans-Thin.ttf 100
/usr/share/fonts/TTF/FiraSans-TwoItalic.ttf 100
/usr/share/fonts/TTF/FiraSans-Two.ttf 100
/usr/share/fonts/TTF/FiraSans-ExtraLightItalic.ttf 200
/usr/share/fonts/TTF/FiraSans-ExtraLight.ttf 200
/usr/share/fonts/TTF/FiraSans-UltraLightItalic.ttf 200
/usr/share/fonts/TTF/FiraSans-UltraLight.ttf 200
/usr/share/fonts/TTF/FiraSans-LightItalic.ttf 300
/usr/share/fonts/TTF/FiraSans-Light.ttf 300
/usr/share/fonts/TTF/FiraSans-BookItalic.ttf 350
/usr/share/fonts/TTF/FiraSans-Book.ttf 350
/usr/share/fonts/TTF/FiraSans-Italic.ttf 400
/usr/share/fonts/TTF/FiraSans-Regular.ttf 400
/usr/share/fonts/TTF/FiraSans-MediumItalic.ttf 500
/usr/share/fonts/TTF/FiraSans-Medium.ttf 500
/usr/share/fonts/TTF/FiraSans-SemiBoldItalic.ttf 600
/usr/share/fonts/TTF/FiraSans-SemiBold.ttf 600
/usr/share/fonts/TTF/FiraSans-BoldItalic.ttf 700
/usr/share/fonts/TTF/FiraSans-Bold.ttf 700
/usr/share/fonts/TTF/FiraSans-ExtraBoldItalic.ttf 800
/usr/share/fonts/TTF/FiraSans-ExtraBold.ttf 800
/usr/share/fonts/TTF/FiraSans-HeavyItalic.ttf 900
/usr/share/fonts/TTF/FiraSans-Heavy.ttf 900
/usr/share/fonts/TTF/FiraSans-UltraItalic.ttf 950
/usr/share/fonts/TTF/FiraSans-Ultra.ttf 950

that looks correct?

@QuLogic
Copy link
Member

QuLogic commented May 25, 2020

I pulled these fonts from Google Fonts, but maybe they're wrong?

The Fedora-packaged version are .otf, so I'm not sure if they're equivalent, but if I use those, the weights are the same as yours, except for ExtraLight, which is 250 instead of 200.

If I run fc-scan FiraSans-Thin.{ttf,otf}, then the otf has weight:0 while the ttf has weight:45.5.

@anntzer
Copy link
Contributor Author

anntzer commented May 25, 2020

Mine were from Arch's distro packages.
I agree with your observations re google's fonts. However the weight values are just directly read (in this case) from the OS/2 table's usWeightClass entry (https://docs.microsoft.com/en-us/typography/opentype/spec/os2#usweightclass), which is the first thing FreeType looks at. So I'm quite tempted to just claim that google's fonts have incorrect metadata...

@QuLogic
Copy link
Member

QuLogic commented May 25, 2020

I also confirmed in FontForge, so I guess I'll report a bug on Google Fonts.

lib/matplotlib/font_manager.py Outdated Show resolved Hide resolved
@QuLogic
Copy link
Member

QuLogic commented May 27, 2020

(If we agree on the PR I'll need to bump the fontlist version number as well.)

Don't forget this.

@anntzer
Copy link
Contributor Author

anntzer commented May 27, 2020

done

@jklymak
Copy link
Member

jklymak commented May 27, 2020

@tacaswell you self-requested a review on this. Did you want it to wait?

I'd merge on the basis of @anntzer and @QuLogic combined expertise, and because it seems an improvement, not because I particularly understand the guts of the font weight detection.

@tacaswell
Copy link
Member

If you think it is ok, merge it.

@jklymak jklymak merged commit a6f31e3 into matplotlib:master May 28, 2020
@AlvaroFurlani
Copy link

Is there any resolution for Google Fonts? I'm trying to use Public Sans and the weight in matplotlib for the Thin and ExtraLight styles are both 250, instead of 100 and 200, respectively.

@QuLogic
Copy link
Member

QuLogic commented Feb 4, 2022

Please open a new issue with an explanation of what you mean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants