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

Add Inter type family #1908

Merged
merged 1 commit into from
Jan 30, 2020
Merged

Add Inter type family #1908

merged 1 commit into from
Jan 30, 2020

Conversation

thundernixon
Copy link
Collaborator

@thundernixon thundernixon commented Mar 27, 2019

Inter, by @rsms, is an impressive open source type project.

It's still in active development, but it's already very popular and useful for a lot of sites (GitHub uses it for marketing site headlines, for one), so it's worth adding to the GF directory.

The only remaining FontBakery issues are around VF file naming and copyright formatting. Filename issues will be resolved once there are up-to-date FontBakery checks and gftools to handle these, and copyright issues (as far as I know) are due to over-strict tests.

  • I have found an issue with vertical metrics which will be very useful to change before we merge this. It shouldn't take too long, but will be very useful to sort out before publishing the fonts.

Upstream repo: https://github.com/rsms/inter
QA work at: https://github.com/thundernixon/inter/tree/qa/misc/googlefonts-qa
PR closes: #1455

@gf-bot

This comment has been minimized.

@m4rc1e m4rc1e self-requested a review March 28, 2019 09:22
Copy link
Collaborator

@m4rc1e m4rc1e left a comment

Choose a reason for hiding this comment

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

Love it!

Thanks @thundernixon. All image plots look good and fb errors are fine.

Could you mentioned the upstream repo and which commit has been used in the commit message? This info is useful for debugging purposes..

@thundernixon

This comment has been minimized.

@gf-bot

This comment has been minimized.

@rsms
Copy link

rsms commented Mar 31, 2019

FYI: I just fixed the copyright value to include just one year ("2019") instead of a range, plus "RSMS" has been registered with Microsoft re "OS/2 VendorID".

@rsms
Copy link

rsms commented Apr 2, 2019

Version 3.5 has been published, which contains some important fixes to vertical metrics for macOS apps, by @thundernixon

https://github.com/rsms/inter/releases/tag/v3.5

@thundernixon
Copy link
Collaborator Author

I've worked in the fresh updates to this latest push. 👍

@gf-bot
Copy link

gf-bot commented Apr 2, 2019

Fontbakery report

Fontbakery version: 0.7.1

[2] Family checks
WARN: METADATA.pb: Fontfamily is listed on Google Fonts API?
WARN: Is the command `ftxvalidator` (Apple Font Tool Suite) available?

[8] Inter-Italic.ttf
🔥 FAIL: Checking file is named canonically.
  • com.google.fonts/check/canonical_filename
  • 🔥 FAIL This is a variable font, but it is using a naming scheme typical of a static font.
  • 🔥 FAIL Please change the font filename to use one of the following valid suffixes for variable fonts: VF, Italic-VF, Roman-VF
🔥 FAIL: Copyright notices match canonical pattern in METADATA.pb
🔥 FAIL: Copyright notices match canonical pattern in fonts
🔥 FAIL: METADATA.pb: Filename is set canonically?
🔥 FAIL: Stricter unitsPerEm criteria for Google Fonts.
  • com.google.fonts/check/unitsperem_strict
  • 🔥 FAIL Font em size (unitsPerEm) is 2816. If possible, please consider using 1000 or even 2000 (which is ideal for Variable Fonts). The acceptable values for unitsPerEm, though, are: [16, 32, 64, 128, 256, 500, 512, 1000, 1024, 2000, 2048].
🔥 FAIL: Copyright field for this font on METADATA.pb matches all copyright notice entries on the name table ?
🔥 FAIL: Checking OS/2 usWinAscent & usWinDescent.
  • com.google.fonts/check/family/win_ascent_and_descent
  • 🔥 FAIL OS/2.usWinAscent value should be equal or greater than 3072, but got 2728 instead [code: ascent]
  • 🔥 FAIL OS/2.usWinDescent value should be equal or greater than 764, but got 680 instead [code: descent]
WARN: Checking unitsPerEm value is reasonable.
  • com.google.fonts/check/unitsperem
  • WARN In order to optimize performance on some legacy renderers, the value of unitsPerEm at the head table should idealy be a power of between 16 to 16384. And values of 1000 and 2000 are also common and may be just fine as well. But we got upm=2816 instead.

[8] Inter-Regular.ttf
🔥 FAIL: Checking file is named canonically.
  • com.google.fonts/check/canonical_filename
  • 🔥 FAIL This is a variable font, but it is using a naming scheme typical of a static font.
  • 🔥 FAIL Please change the font filename to use one of the following valid suffixes for variable fonts: VF, Italic-VF, Roman-VF
🔥 FAIL: Copyright notices match canonical pattern in METADATA.pb
🔥 FAIL: Copyright notices match canonical pattern in fonts
🔥 FAIL: METADATA.pb: Filename is set canonically?
🔥 FAIL: Stricter unitsPerEm criteria for Google Fonts.
  • com.google.fonts/check/unitsperem_strict
  • 🔥 FAIL Font em size (unitsPerEm) is 2816. If possible, please consider using 1000 or even 2000 (which is ideal for Variable Fonts). The acceptable values for unitsPerEm, though, are: [16, 32, 64, 128, 256, 500, 512, 1000, 1024, 2000, 2048].
🔥 FAIL: Copyright field for this font on METADATA.pb matches all copyright notice entries on the name table ?
🔥 FAIL: Checking OS/2 usWinAscent & usWinDescent.
  • com.google.fonts/check/family/win_ascent_and_descent
  • 🔥 FAIL OS/2.usWinAscent value should be equal or greater than 3072, but got 2728 instead [code: ascent]
  • 🔥 FAIL OS/2.usWinDescent value should be equal or greater than 764, but got 680 instead [code: descent]
WARN: Checking unitsPerEm value is reasonable.
  • com.google.fonts/check/unitsperem
  • WARN In order to optimize performance on some legacy renderers, the value of unitsPerEm at the head table should idealy be a power of between 16 to 16384. And values of 1000 and 2000 are also common and may be just fine as well. But we got upm=2816 instead.

Summary

💔 ERROR 🔥 FAIL ⚠ WARN 💤 SKIP ℹ INFO 🍞 PASS
0 14 4 45 13 199
0% 5% 1% 16% 5% 72%

Note: The following loglevels were omitted in this report:

  • SKIP
  • INFO
  • PASS

Diff images: qa.zip

@thundernixon
Copy link
Collaborator Author

Currently working out how to make the vertical metrics work well on Windows. Following FontBakery's suggestion doesn't quite fix it. The result is detailed at Inter PR rsms/inter#150.

@thundernixon
Copy link
Collaborator Author

Unless someone else wants to pick this up, I think it's probably best to ignore FontBakery's suggestion of changing the winAscent value, as detailed by tests and comparisons at rsms/inter#150 (comment).

I've tried to thoroughly understand the vertical metrics here, but there are still parts of it that are not entirely clear to me – in large part because applications aren't responding to metrics changes as I would expect them to. For example, I managed to fix the previous problems on varying bounding box height between different styles of Inter, but I am now seemingly unable to recreate those issues – even with old releases of Inter, and even when I rename these to avoid potential cacheing conflicts.

All that said, it has been confirmed by rsms that the updates to vertical metrics solved known problems, and applying FontBakery's winAscent suggestion was confirmed by Chris Simpkins to cause line height issues in MS Word. Things are working well in all web browsers, and I think we may have to live with imperfections caused by MS Word. At this point, the bigger loss is in holding Inter back from Google Fonts for longer than in being flexible on this vertical metrics check.

@laerm0

This comment has been minimized.

@thundernixon

This comment has been minimized.

@micah

This comment has been minimized.

@thundernixon

This comment has been minimized.

@micah

This comment has been minimized.

@laerm0

This comment has been minimized.

@micah

This comment has been minimized.

@laerm0

This comment has been minimized.

@laerm0

This comment has been minimized.

@thundernixon
Copy link
Collaborator Author

Haha, all good @laerm0 & @micah. Someday, I'll need to have a similar chat with whoever (a)stephen is

For now, I'll hide the comments, so they don't distract from the PR. 🙂

@jakesylvestre
Copy link

Is this good to go?

@thundernixon
Copy link
Collaborator Author

This seems good to go. It's still up-to-date with the latest 3.5 release from inter. Can we push it?

@davelab6
Copy link
Member

davelab6 commented May 3, 2019

I'll merge this soon

@thundernixon
Copy link
Collaborator Author

Actually, I'm seeing that there are a few commits in inter that were made after the latest push, so I'm going to rebuild these and repush them today, just so the PR is as fresh as the upstream.

@rsheeter
Copy link
Collaborator

Including static/ and not referencing it from metadata is fine if there is value to having a committed copy of the instances. It'll be a little weird because they won't match what you'd get if you downloaded the instances from GF.

@thundernixon
Copy link
Collaborator Author

@rsheeter I'm happy to go either way on the statics – it's ultimately up to you, Marek, & Dave.

For what it's worth, I suspect that there won't be much of an overlap between folks who clone the repo for the fonts and folks who download from the GF frontend (but it would be pretty nice if the GF eng instancing code could be moved into something like a Travis CI and auto-generate statics from VF pull requests... though I'm not sure if that is possible / something that has already been considered).

@rsheeter
Copy link
Collaborator

If having static/ is the only remaining concern I suggest merging (with or without static/ as you see fit), closing this ticket, and filing a separate issue issue discuss the need and approach for static/ dirs.

@gf-bot
Copy link

gf-bot commented Jan 22, 2020

Fontbakery report

Fontbakery version: 0.7.17

[1] Family checks
WARN: Is the command `ftxvalidator` (Apple Font Tool Suite) available?
--- Rationale ---

There's no reasonable (and legal) way to run the command `ftxvalidator` of the
Apple Font Tool Suite on a non-macOS machine. I.e. on GNU+Linux or Windows etc.

If Font Bakery is not running on an OSX machine, the machine running Font
Bakery could access `ftxvalidator` on OSX, e.g. via ssh or a remote procedure
call (rpc).

There's an ssh example implementation at:
https://github.com/googlefonts/fontbakery/blob/master/prebuilt/workarounds/ftxvalidator/ssh-implementation/ftxvalidator


  • WARN ftxvalidator is not available.

[8] Inter[slnt,wght].ttf
💔 ERROR: Check variable font instances have correct coordinate values
🔥 FAIL: Check `Google Fonts Latin Core` glyph coverage.
--- Rationale ---

Google Fonts expects that fonts in its collection support at least the minimal
set of characters defined in the `GF-latin-core` glyph-set.


  • 🔥 FAIL Missing required codepoints: 0x2215 (DIVISION SLASH) [code: missing-codepoints]
🔥 FAIL: METADATA.pb weight matches postScriptName.
🔥 FAIL: Stricter unitsPerEm criteria for Google Fonts.
--- Rationale ---

Even though the OpenType spec allows unitsPerEm to be any value between 16 and
16384, the Google Fonts project aims at a narrower set of reasonable values.

The spec suggests usage of powers of two in order to get some performance
improvements on legacy renderers, so those values are acceptable.

But value of 500 or 1000 are also acceptable, with the added benefit that it
makes upm math easier for designers, while the performance hit of not using a
power of two is most likely negligible nowadays.

Another acceptable value is 2000. Since TT outlines are all integers (no
floats), then instances in a VF suffer rounding compromises, and therefore a
1000 UPM is to small because it forces too many such compromises.

Therefore 2000 is a good 'new VF standard', because 2000 is a simple 2x
conversion from existing fonts drawn on a 1000 UPM, and anyone who knows what
10 units can do for 1000 UPM will know what 20 units does too.

Additionally, values above 2048 would result in filesize increases with not
much added benefit.


  • 🔥 FAIL Font em size (unitsPerEm) is 2816. If possible, please consider using 1000 or even 2000 (which is ideal for Variable Fonts). The acceptable values for unitsPerEm, though, are: [16, 32, 64, 128, 256, 500, 512, 1000, 1024, 2000, 2048]. [code: bad-value]
🔥 FAIL: A static fonts directory with at least two fonts must accompany variable fonts
--- Rationale ---

Variable font family directories kept in the google/fonts git repo must include
a static/ subdir containing static fonts.
These files are meant to be served for users that still lack support for
variable fonts in their web browsers.


  • 🔥 FAIL Please create a subdirectory called "static/" and include in it static font files. [code: missing]
🔥 FAIL: Checking OS/2 usWinAscent & usWinDescent.
--- Rationale ---

A font's winAscent and winDescent values should be greater than the head
table's yMax, abs(yMin) values. If they are less than these values, clipping
can occur on Windows platforms
(https://github.com/RedHatBrand/Overpass/issues/33).

If the font includes tall/deep writing systems such as Arabic or Devanagari,
the winAscent and winDescent can be greater than the yMax and abs(yMin) to
accommodate vowel marks.

When the win Metrics are significantly greater than the upm, the linespacing
can appear too loose. To counteract this, enabling the OS/2 fsSelection bit 7
(Use_Typo_Metrics), will force Windows to use the OS/2 typo values instead.
This means the font developer can control the linespacing with the typo values,
whilst avoiding clipping by setting the win values to values greater than the
yMax and abs(yMin).


  • 🔥 FAIL OS/2.usWinAscent value should be equal or greater than 3072, but got 2728 instead [code: ascent]
  • 🔥 FAIL OS/2.usWinDescent value should be equal or greater than 900, but got 680 instead [code: descent]
WARN: METADATA.pb: Fontfamily is listed on Google Fonts API?
WARN: Checking unitsPerEm value is reasonable.
--- Rationale ---

According to the OpenType spec:

The value of unitsPerEm at the head table must be a value between 16 and 16384.
Any value in this range is valid.

In fonts that have TrueType outlines, a power of 2 is recommended as this
allows performance optimizations in some rasterizers.

But 1000 is a commonly used value. And 2000 may become increasingly more common
on Variable Fonts.


  • WARN In order to optimize performance on some legacy renderers, the value of unitsPerEm at the head table should idealy be a power of between 16 to 16384. And values of 1000 and 2000 are also common and may be just fine as well. But we got 2816 instead. [code: suboptimal]

Summary

💔 ERROR 🔥 FAIL ⚠ WARN 💤 SKIP ℹ INFO 🍞 PASS 🔎 DEBUG
1 5 3 36 8 110 0
1% 3% 2% 22% 5% 67% 0%

Note: The following loglevels were omitted in this report:

  • SKIP
  • INFO
  • PASS
  • DEBUG

Diff images: qa.zip

@thundernixon
Copy link
Collaborator Author

thundernixon commented Jan 22, 2020

Updates here:

  • I've removed the static sub-directory here. That should maybe still be added for users who download fonts directly from this repo, but I don't want it holding up this PR. If we do add these statics, I think they should have a family name suffix, to avoid font menu issues. I've made a simple FontTools name patcher for this: thundernixon/inter@07bd35a
  • I've updated the variable font to make sure it has the latest rsms/inter git hash in the version names, names 3 & 5. thundernixon/inter@3644302
  • I've added -Regular to the postscript name (name 6) with a FontTools patch, to make FontBakery happy (and potentially also other internal metadata-based tools). thundernixon/inter@c22496e
  • --force pushed to keep this PR to just a single commit

@gf-bot

This comment has been minimized.

@gf-bot

This comment has been minimized.

@gf-bot

This comment has been minimized.

update metadata full_name for fontbakery
@efstajas
Copy link

It looks like all open issues are resolved, unless I'm missing anything?

@thundernixon thundernixon merged commit 907e074 into master Jan 30, 2020
@thundernixon
Copy link
Collaborator Author

I've been instructed to merge this into the repo. It can be further updated if needed.

As with any other typeface in here, internal Google Fonts folks will ultimately decide when and how to bring this into production, and it may be held back temporarily. A current obstacle is that no modern browsers currently properly support the font-style properly as specified by the CSS Level 4 Fonts Module for variable fonts with a slnt axis, like Inter. For a full explanation and test case of this, plus links to filed browser bugs, please see https://arrowtype.github.io/vf-slnt-test/.

@andyjgf
Copy link
Contributor

andyjgf commented Jan 31, 2020

FYI I changed your METADATA.pb file from SANS-SERIF to SANS_SERIF.

@rsheeter
Copy link
Collaborator

@m4rc1e please add Inter to to_sandbox when ready. It's fine if it specifies the family has slnt. Note that slnt won't be enabled on prod for the time being due to the issues @thundernixon notes above.

@rsms
Copy link

rsms commented Mar 6, 2020

Could we add the single-axis (weight axis) italic as well? That’s how Inter is served from its main website (two single-axis VFs) which is a workaround that works everywhere.

@knadh
Copy link

knadh commented Apr 1, 2020

For everyone watching this thread, Inter is live on Google Fonts - https://fonts.google.com/specimen/Inter

Not sure when it happened.

@arrowtype
Copy link
Collaborator

@rsms I will make a PR to do that next week. Hopefully, if I don't change much from the first one, it will be a relatively quick merge

@rsheeter
Copy link
Collaborator

rsheeter commented Apr 2, 2020

FYI, slnt is now enabled in our api. For example, https://codepen.io/rs42/full/WNvWRqM.

@davelab6
Copy link
Member

davelab6 commented Apr 3, 2020

@rsms I will make a PR to do that next week. Hopefully, if I don't change much from the first one, it will be a relatively quick merge

We've developed lint and regression tools to try to keep pace with projects like Inter, where they can be integrated into a CI process to prevent regressions from snowballing :)

@andrewvarga
Copy link

Is there a reason there is a single ttf file "Inter[slnt,wght].ttf" as opposed to the structure of most other fonts where there is a separate file for each weight/style eg. "InriaSerif-Bold.ttf" ?
I'm using a script which parses the font directory but the usual naming convention doesn't apply here, which makes it a bit hard to work with.

@arrowtype
Copy link
Collaborator

@andrewvarga It's a variable font! Please see https://variablefonts.io/ for more info

@m4rc1e m4rc1e deleted the inter branch May 18, 2020 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Inter