8000 Improve font weight guessing by Rufflewind · Pull Request #8551 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Improve font weight guessing #8551

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Improve font weight guessing #8551

wants to merge 3 commits into from

Conversation

Rufflewind
Copy link
Contributor
@Rufflewind Rufflewind commented Apr 30, 2017 •
edited
Loading

PR Summary

Bandaid-ish fix for #8550. Would like some feedback on whether these are good/bad ideas.

  1. Add more font weights and change some existing ones to better fit Mozzila's developer wiki:

    • Added:

      • thin (100 50)
      • extralight (200 100)
      • plain (400)
      • extrabold and ultrabold (800)

    • Changed:

      • ultralight from 100 to 200
      • light from 200 to 300
      • roman from 500 to 400
      • heavy from 800 to 900

  2. Improve the font-weight guessing logic. In particular, rather than trusting style_flags (which is very limited and inexpressive), the code now inspects style_name for keywords. Some limited testing this on the fonts of my system indicates that most sensible cases are covered. Otherwise, just bail with KeyError so that the font won’t get registered (mitigates Matplotlib chooses the wrong font for unrecognized weights #8550).

    The few fonts where the new logic fails have really exotic subfamilies like “Retina” or “Four Italic”, which don’t really map well to the existing font system anyway so I don’t think it’s much of a loss. We could still check for style_flags & BOLD, but testing revealed only one font where this mattered (Latin Modern Sans Quotation, which is actually not bold despite the bold flag being set!).

PR Checklist

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

@Rufflewind Rufflewind changed the title Fontweight Improve font weight guessing Apr 30, 2017
@tacaswell
Copy link
Member

I do not think we can change the mapping between string as integer weights as we take both as input.

I am mostly 👎 on no longer picking up fonts that we used to pick up (even if the weights are a bit off).

#7931 is likely related, but does not fully fix this problem?

@Rufflewind
Copy link
Contributor Author

#7931 is likely related, but does not fully fix this problem?

I tested it using HEAD (a9a8634) with Fira Sans and each run I seem to get a random choice of:

/usr/share/fonts/TTF/FiraSans-Book.ttf
/usr/share/fonts/TTF/FiraSans-Eight.ttf
/usr/share/fonts/TTF/FiraSans-Four.ttf
/usr/share/fonts/TTF/FiraSans-Hair.ttf
/usr/share/fonts/TTF/FiraSans-Regular.ttf
/usr/share/fonts/TTF/FiraSans-Thin.ttf
/usr/share/fonts/TTF/FiraSans-Two.ttf
/usr/share/fonts/TTF/FiraSans-Ultra.ttf

Tested using:

# while rm -fr ~/.cache/matplotlib && python foo.py; do :; done
import matplotlib
matplotlib.use("Agg")
matplotlib.rcParams["font.sans-serif"] = "Fira Sans"
matplotlib.rcParams["font.weight"] = "regular"
import matplotlib.pyplot as plt
print(matplotlib.font_manager.findfont("Fira Sans"))
plt.plot([1, 2], [3, 4])
plt.savefig("foo.png")

@Rufflewind
Copy link
Contributor Author

I do not think we can change the mapping between string as integer weights as we take both as input.

OK I've restored them and pushed "Thin" down to 50 instead of 100.

I am mostly 👎 on no longer picking up fonts that we used to pick up (even if the weights are a bit off).

Perhaps we could deprioritize them and mark their weights as “uncertain”. They should still get picked up as if their weight is Regular, but if a “definitely” Regular font is found, then it takes precedence.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone May 1, 2017
@kshramt
Copy link
Contributor
kshramt commented May 6, 2017

I tested it using HEAD (a9a8634) with Fira Sans and each run I seem to get a random choice of:

@Rufflewind I could not reproduce your results.
On my machine, your test script consistently prints /Users/kshramt/.fonts/FiraSans-Regular.otf.

@Rufflewind
Copy link
Contributor Author

I re-reproduced my results on a clean Arch container:

  • Python 3.6.1
  • Matplotlib 2.0.0+4221.ge232afe73
  • Fira Sans 4.202 (from the .tar.gz)
(venv) [rf@arch-container matplotlib]$ while rm -fr ~/.cache/matplotlib && python foo.py; do :; done
/home/rf/.fonts/FiraSans-Eight.otf
/home/rf/.fonts/FiraSans-Book.otf
/home/rf/.fonts/FiraSans-Eight.otf
/home/rf/.fonts/FiraSans-Two.otf
/home/rf/.fonts/FiraSans-Four.otf
/home/rf/.fonts/FiraSans-Hair.otf
/home/rf/.fonts/FiraSans-Four.otf
/home/rf/.fonts/FiraSans-Hair.otf
/home/rf/.fonts/FiraSans-Book.otf
/home/rf/.fonts/FiraSans-Regular.otf
/home/rf/.fonts/FiraSans-Thin.otf
/home/rf/.fonts/FiraSans-Eight.otf
/home/rf/.fonts/FiraSans-Book.otf
/home/rf/.fonts/FiraSans-Eight.otf
/home/rf/.fonts/FiraSans-Four.otf
…

@kshramt
Copy link
Contributor
kshramt commented May 7, 2017

@Rufflewind Thank you for re-reproducing the results.
I could reproduce your results with FiraSans font files from https://github.com/mozilla/Fira/archive/4.202.tar.gz.
You can reproduce my results in the previous comment with FiraSans font files from https://www.fontsquirrel.com/fonts/download/fira-sans (zip file).

fc-scan ~/.fonts/FiraSans-Regular.otf shows that fullname field have changed from "Fira Sans Regular" to "Fira Sans".
According to Name IDs section of https://www.microsoft.com/typography/otspec/name.htm,

If string 2 is "Regular", it is sometimes omitted from name ID 4.

Ref: #8587

@QuLogic QuLogic mentioned this pull request May 15, 2017
Open
@tacaswell tacaswell mentioned this pull request Jun 22, 2017
Closed
@tacaswell tacaswell closed this Jun 25, 2017
@tacaswell tacaswell reopened this Jun 25, 2017
@tacaswell
Copy link
Member
tacaswell commented Jun 25, 2017 •
edited
Loading

'powercycled' PR to re-trigger CI

@tacaswell
Copy link
Member

xref #5574

@tacaswell tacaswell mentioned this pull request Jun 25, 2017
Closed
6 tasks
@cy18
Copy link
cy18 commented Jun 26, 2017 •
edited
Loading

I made three modification in #8607. In my opinion, the first and third are not improving, but bug fix.

@tacaswell tacaswell mentioned this pull request Jun 26, 2017
Closed
@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Aug 29, 2017
@anntzer anntzer mentioned this pull request Jan 13, 2020
Merged
6 tasks
@anntzer
Copy link
Contributor
anntzer commented May 28, 2020

Superseded by #16203; thanks for the pr.

@anntzer anntzer closed this May 28, 2020
@story645 story645 removed this from the future releases milestone Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
topic: text/fonts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Rufflewind @tacaswell @kshramt @cy18 @anntzer @story645 @dstansby
0