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

Possibly incorrect WARN on monospaced build width consistency - some marks have zero advance width definitions #3053

Closed
chrissimpkins opened this issue Oct 5, 2020 · 6 comments
Assignees
Milestone

Comments

@chrissimpkins
Copy link
Member

chrissimpkins commented Oct 5, 2020

Glyphs source file -> static instance TTF font build with fontmake v2.2.1 pipeline. This is a weight axis monospaced typeface.

Observed behaviour

fontbakery raises a WARN level report that the combining marks *comb glyphs in our glyph set have inconsistent widths in a monospaced typeface.

The report is:

* WARN: Font is monospaced but 9 glyphs (3.91304347826087%) have a different width. You should check the widths of: ['uni0308', 'gravecomb', 'acutecomb', 'uni0302', 'uni030C', 'uni030A', 'tildecomb', 'uni0304', 'uni0327'] [code: mono-outliers]

and this appears to come from this block of the fontbakery source:

https://github.com/googlefonts/fontbakery/blob/42e872c92947569f5972a76bb52175e95666ed08/Lib/fontbakery/profiles/shared_conditions.py#L152-L173

I reviewed the Glyphs source files and the advance width for all of the reported glyphs is set at the expected value for the typeface (600).

Here is what I see in the binaries:

>>> for glyphname, metrics in glyph_metrics.items():
...     if metrics[0] != 600:
...         print(f"{glyphname}: {metrics}")
...
uni0308: (0, 140)
gravecomb: (0, 213)
acutecomb: (0, 213)
uni0302: (0, 146)
uni030C: (0, 146)
uni030A: (0, 190)
tildecomb: (0, 126)
uni0304: (0, 161)
uni0327: (0, 200)

And a visual of dieresiscomb:

2020-10-05_18-07-38

It looks like a subset of the marks (maybe all combining marks?) are compiled with a zero advance width value and should be excluded from this check.

Expected behaviour

This is not reported as a warning for monospaced typefaces if these metrics are valid.

@felipesanches
Copy link
Collaborator

@chrissimpkins please let me know if this fix is good for you. Otherwise, consider reopening the issue.

@felipesanches felipesanches reopened this Oct 20, 2020
@felipesanches
Copy link
Collaborator

earlier this week @chrissimpkins commented on the merged PR at #3062 (comment):

I'm not sure that this influences the issue report if I understand the change correctly Felipe. This looks like it is a "is it a monospaced font" side definition change. The fonts that I tested were designed as monospaced fonts. We successfully passed that test and the result was correct. The issue that I found was that when it is a monospaced font, there are glyphs outside of the ASCII set that are detected as inappropriately defined in a monospaced font b/c they have zero advance widths. These were in the combining mark glyph set which are compiled with zero advance widths. Please let me know if I can provide any additional information.

I've been re-evaluating this today and I think my changes in that PR are actually good. But I also think that the problem observed by Cris may be a corner-case occurrence. My theory is that perhaps the most common advance for glyphs on Chris' font may be zero, which would cause that bad log message. To address that, I think I may have to make one additional change so that only non-zero advance widths are considered when calculating the most common advance. In other words, we are actually interested in the most common non-zero advance value.

In order to confirm if that's the actual case we're dealing with here, I'd need access to a sample of your font files. Can you please send me some of those files, @chrissimpkins?

@kontur
Copy link
Contributor

kontur commented Oct 21, 2020

It would also be good to know if @chrissimpkins fontbakery includes the PR from #3036 — somewhat related issue possibly, no?

Combing marks should be 0 width also in monospace fonts afaik.

@chrissimpkins
Copy link
Member Author

chrissimpkins commented Oct 21, 2020

I've been re-evaluating this today and I think my changes in that PR are actually good.

No criticism about the changes. I think that they do not address my issue if I understand the changes correctly.

Combing marks should be 0 width also in monospace fonts afaik.

This is the root of the issue. FB appropriately detects our fonts as "monospaced" based on the FB assessment of the adv width of the ASCII set + the included FB glyph filters. FB is then in "monospaced" check mode and flags combining marks as not appropriate for a monospaced build because they do not share the adv width metrics of the ASCII set calculation. Combining marks are compiled with a zero advance width definition by fontmake so they will not have the adv width calculated by the ASCII set routine.

I think that the pseudo code to address it is ~

if MONOSPACED:
   for GLYPH in GLYPHSET:
       if GLYPH.advWidth != MONOSPACED_ADVWIDTH and GLYPH not in FILTER_LIST:
           FONTBAKERY_WARN/FAIL

I believe that the combining marks simply need to be added to the filter so that they are either not checked or are confirmed to have a zero adv width that can differ from the global adv width definition for the font.

@chrissimpkins
Copy link
Member Author

Can you please send me some of those files, @chrissimpkins?

Done! Thanks Felipe!

@felipesanches
Copy link
Collaborator

I've just verified that indeed commit 42e872c (as suggested by @kontur) fixed the problem last month and was included in the last two stable releases, 0.7.30 and 0.7.31

@chrissimpkins please update your fontbakery installation to benefit from the fix.

felipesanches added a commit to felipesanches/fontbakery that referenced this issue Oct 22, 2020
Then we should ignore it and get the second most common value.
In other words, what we want here is the most common nonzero value.

I think it is extremely unlikely to happen, but this ensures it will also be properly handled if it ever happens.
(issue fonttools#3053)
felipesanches added a commit to felipesanches/fontbakery that referenced this issue Oct 22, 2020
Then we should ignore it and get the second most common value.
In other words, what we want here is the most common nonzero value.

I think it is extremely unlikely to happen, but this ensures it will also be properly handled if it ever happens.
(issue fonttools#3053)
felipesanches added a commit that referenced this issue Oct 22, 2020
Then we should ignore it and get the second most common value.
In other words, what we want here is the most common nonzero value.

I think it is extremely unlikely to happen, but this ensures it will also be properly handled if it ever happens.
(issue #3053)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants