huertatipografica / Alegreya

Serif family, part of the Alegreya "super family" www.huertatipografica.com
SIL Open Font License 1.1
164 stars 11 forks source link

review for PR of TN master #26

Closed pichotta closed 4 years ago

pichotta commented 5 years ago

@m4rc1e is this TN master branch ready for merge PR?

m4rc1e commented 5 years ago

Thanks @pichotta, I'll take a look now.

m4rc1e commented 5 years ago

Fontbakery report

Fontbakery version: 0.7.15

[4] Alegreya-Italic[wght].ttf
πŸ”₯ FAIL: Font has old ttfautohint applied? * [com.google.fonts/check/old_ttfautohint](https://font-bakery.readthedocs.io/en/latest/fontbakery/profiles/googlefonts.html#com.google.fonts/check/old_ttfautohint)
--- Rationale ---

This check finds which version of ttfautohint was used, by inspecting name
table entries and then finds which version of ttfautohint is currently
installed in the system.

* πŸ”₯ **FAIL** Failed to parse ttfautohint version values: installed = '1.8.3'; used_in_font = '1.8.1.43-b0c9' [code: parse-error]
πŸ”₯ FAIL: Are there caret positions declared for every ligature? * [com.google.fonts/check/ligature_carets](https://font-bakery.readthedocs.io/en/latest/fontbakery/profiles/googlefonts.html#com.google.fonts/check/ligature_carets)
--- Rationale ---

All ligatures in a font must have corresponding caret (text cursor) positions
defined in the GDEF table, otherwhise, users may experience issues with caret
rendering.

* πŸ”₯ **FAIL** Failed to lookup ligatures. This font file seems to be malformed. For more info, read: https://github.com/googlefonts/fontbakery/issues/1596 [code: malformed]
πŸ”₯ FAIL: Is there kerning info for non-ligated sequences? * [com.google.fonts/check/kerning_for_non_ligated_sequences](https://font-bakery.readthedocs.io/en/latest/fontbakery/profiles/googlefonts.html#com.google.fonts/check/kerning_for_non_ligated_sequences)
--- Rationale ---

Fonts with ligatures should have kerning on the corresponding non-ligated
sequences for text where ligatures aren't used (eg
https://github.com/impallari/Raleway/issues/14).

* πŸ”₯ **FAIL** Failed to lookup ligatures. This font file seems to be malformed. For more info, read: https://github.com/googlefonts/fontbakery/issues/1596 [code: malformed]
⚠ WARN: Stricter unitsPerEm criteria for Google Fonts. * [com.google.fonts/check/unitsperem_strict](https://font-bakery.readthedocs.io/en/latest/fontbakery/profiles/googlefonts.html#com.google.fonts/check/unitsperem_strict)
--- 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.

* ⚠ **WARN** Even though unitsPerEm (1000) in this font is reasonable. It is strongly advised to consider changing it to 2000, since it will likely improve the quality of Variable Fonts by avoiding excessive rounding of coordinates on interpolations. [code: legacy-value]

[4] Alegreya[wght].ttf
πŸ”₯ FAIL: Font has old ttfautohint applied? * [com.google.fonts/check/old_ttfautohint](https://font-bakery.readthedocs.io/en/latest/fontbakery/profiles/googlefonts.html#com.google.fonts/check/old_ttfautohint)
--- Rationale ---

This check finds which version of ttfautohint was used, by inspecting name
table entries and then finds which version of ttfautohint is currently
installed in the system.

* πŸ”₯ **FAIL** Failed to parse ttfautohint version values: installed = '1.8.3'; used_in_font = '1.8.1.43-b0c9' [code: parse-error]
πŸ”₯ FAIL: Are there caret positions declared for every ligature? * [com.google.fonts/check/ligature_carets](https://font-bakery.readthedocs.io/en/latest/fontbakery/profiles/googlefonts.html#com.google.fonts/check/ligature_carets)
--- Rationale ---

All ligatures in a font must have corresponding caret (text cursor) positions
defined in the GDEF table, otherwhise, users may experience issues with caret
rendering.

* πŸ”₯ **FAIL** Failed to lookup ligatures. This font file seems to be malformed. For more info, read: https://github.com/googlefonts/fontbakery/issues/1596 [code: malformed]
πŸ”₯ FAIL: Is there kerning info for non-ligated sequences? * [com.google.fonts/check/kerning_for_non_ligated_sequences](https://font-bakery.readthedocs.io/en/latest/fontbakery/profiles/googlefonts.html#com.google.fonts/check/kerning_for_non_ligated_sequences)
--- Rationale ---

Fonts with ligatures should have kerning on the corresponding non-ligated
sequences for text where ligatures aren't used (eg
https://github.com/impallari/Raleway/issues/14).

* πŸ”₯ **FAIL** Failed to lookup ligatures. This font file seems to be malformed. For more info, read: https://github.com/googlefonts/fontbakery/issues/1596 [code: malformed]
⚠ WARN: Stricter unitsPerEm criteria for Google Fonts. * [com.google.fonts/check/unitsperem_strict](https://font-bakery.readthedocs.io/en/latest/fontbakery/profiles/googlefonts.html#com.google.fonts/check/unitsperem_strict)
--- 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.

* ⚠ **WARN** Even though unitsPerEm (1000) in this font is reasonable. It is strongly advised to consider changing it to 2000, since it will likely improve the quality of Variable Fonts by avoiding excessive rounding of coordinates on interpolations. [code: legacy-value]

Summary

πŸ’” ERROR πŸ”₯ FAIL ⚠ WARN πŸ’€ SKIP β„Ή INFO 🍞 PASS πŸ”Ž DEBUG
0 6 2 127 12 146 0
0% 2% 1% 43% 4% 50% 0%

Note: The following loglevels were omitted in this report:

I'll look into the caret and lig fails later on today.


Regressions

Bracket layers missing on oslash/Oslash glyphs

oslash

Double accents have collisions

doublestack

cjdunn commented 5 years ago

@m4rc1e Hmm, I'm not sure what the problem is with the /Oslash

Currently the var font swaps /Oslash between Bold (has bar) and Extra Bold (has no bar). See screenshots attached. Oslash_bold Oslash_ex_bold

This seems to match the live Alegreya fonts on Google Fonts: https://fonts.google.com/specimen/Alegreya?selection.family=Alegreya files here: Alegreya-ExtraBold.ttf.zip Alegreya-Bold.ttf.zip

Am I missing something?

##############################

Regarding the double accents: I see what you mean about the collisions. Unfortunately it seems that fontmake doesn't do a good job of handling nested components, ie the /Otildeacute glyph in the Black named instance doesn't match the same glyph in the Black UFO master when it uses components. But when it is decomposed it matches. To me this is a fontmake bug. (see screenshot: background is with components, foreground is decomposed)

Screenshot 2019-11-14 11 28 05

I'll file a bug report for fontmake.

In the mean time, it looks like we'll have to decompose all nested components

cjdunn commented 5 years ago

Here's my fontmake bug report: https://github.com/googlefonts/fontmake/issues/595

pichotta commented 5 years ago

@m4rc1e submitting another pull request after revisions

pichotta commented 4 years ago

@juandelperal The weight axis variable font for Alegreya is complete. See Issue #23 Will you accept this pull request to merge the [wght].ttf and the .ufo sources into the master?

juandelperal commented 4 years ago

Thanks @pichotta Thanks for your work!

At a first glance I can see that:

Thanks

strarsis commented 4 years ago

This is awesome! When this PR is merged, will the Google Fonts API automatically provide the Variable font format for Alegraya?

strarsis commented 4 years ago

@pichotta: The PR is merged! Will this be automatically now used by the Google Fonts API?

pichotta commented 4 years ago

@juandelperal @strarsis Yes, it will be rolled into the Google Fonts API. Do the most recent commits address the concerns posted above on Sept 18?

juandelperal commented 4 years ago

Great! Thanks for fixing that. Now merging