Glyph outline checks #3088

simoncozens · 2020-11-09T11:52:55Z

Description

This pull request addresses the problems described at issue #3064 amongst others. Creates a new path profile with tests for:

Alignment against baseline/x-height/ascender/descender/etc.
Short segments
Colinear vectors
Jaggy segments
Semi-vertical / semi-horizontal lines

As these are heuristic tests they create warnings rather than failures.

To Do

update CHANGELOG.md
wait for checks to pass (except github/pages, which is stuck)
request a review

* Alignment against baseline/x-height/ascender/descender/etc. * Short segments * Colinear vectors * Jaggy segments (fixes #3064) * Semi-vertical / semi-horizontal lines

tests/profiles/path_test.py

felipesanches · 2020-11-09T14:16:31Z

Lib/fontbakery/profiles/path.py

+                        warnings.append(glyphname)
+
+    if warnings:
+        formatted_list = "\t* " + pretty_print_list(list(set(warnings)), sep="\n\t* ")


here you convert a list into a set and back into a list again. Is that meant to remove duplicates?

Also, it would be good to sort this list to avoid problems like the one described at #3038

felipesanches · 2020-11-09T14:23:39Z

I'd like to suggest renaming everything (profile, check-IDs, log messages, etc) to use the term "outlines" instead of "paths".

Also, I am not so sure we need to put these into a separate profile. In general I prefer to avoid "profile proliferation". Whenever possible we should try to assign checks to the common profiles. Ideally this should go into the universal profile.

But maybe it is useful to keep them into this outlines profile so that it is easy to run this set of checks and nothing else with a check-outlines command. If we decide to keep the checks in this new profile, I'd propose that universal should include it so that it would inherit this set of checks.

simoncozens · 2020-11-09T14:30:22Z

Yes, I wasn't sure what to do with the profile. I agree it's best not to have a separate one but it was very helpful for testing/developing to be able to call the fontbakery tool on a profile and get the test results. See what you think about these checks and I would be happy o migrate them to universal if you like them...

felipesanches · 2020-11-09T14:35:54Z

yeah, I think they should all be included in universal (unless someone disagrees ;-D)

graphicore · 2020-11-09T14:49:00Z

But maybe it is useful to keep them into this outlines profile so that it is easy to run this set of checks and nothing else with a check-outlines command.

To select checks with a common part in the id can be done with the -c flag e.g.

$ fontbakery check-universal -c /outline/ *.ttf

chrissimpkins · 2020-11-09T18:24:33Z

This is great Simon! Do you have any information on the time impact for a routine universal profile run across a 'typical' font with this addition as a default? If this is significant, it may be worth either opting in/out with a command line flag or keeping this in a new outline check profile.

felipesanches · 2020-11-09T18:28:25Z

If it is a WARN-level check, then it is OK to have fonts still not passing.

I want to avoid adding too many new flags to the command line. The set of flags we have is already a bit too complex, in my opinion.

I will try to run this on the entire collection and post a summary here later.

chrissimpkins · 2020-11-09T18:32:10Z

Sorry I revised/clarified the question. I was attempting to ask about how much extra time it takes to run the profile with glyph level outline checks.

felipesanches · 2020-11-09T18:33:25Z

Lib/fontbakery/profiles/outline.py

+    for glyphname, outlines in outlines_dict.items():
+        for p in outlines:
+            segments = p.asSegments()
+            outline_area = None


Travis noticed this variable is defined but not used. And the same thing for another var with the same name at line 147:

https://travis-ci.org/github/googlefonts/fontbakery/jobs/742453976

felipesanches · 2020-11-09T18:36:56Z

Sorry I revised/clarified the question. I was attempting to ask about how much extra time it takes to run the profile with glyph level outline checks.

oh yeah, that might indeed be a bit of a concern. If it is way too heavy then it may be good to provide means to disable it. If it is just a bit of extra processing time, on the other hand, I think we can deal with it. I think we'd prefer better fonts even if it takes a bit longer to bake them. ;-)

simoncozens · 2020-11-09T20:49:11Z

It is fairly slow, but not crazy slow - 4 seconds to process Source Sans Pro on my machine.

One reason you may not want to run them by default is that there will almost always be lots of false positives, depending on your design. For example, with Source Sans, there are many legitimate "short segments" around the ink traps, apex of A etc, and some of these things - particularly collinear vectors and short segments - may be necessary for interpolation between masters. I don't want to train designers to ignore warnings as potentially nothing worth looking at...

felipesanches · 2020-11-10T01:01:00Z

I think it may be reasonable to mention this high likelihood of false positives in the rationale text. And also suggest that users should consider looking at some of the results from time-to-time at least to double-check the correctness of their designs.

felipesanches · 2020-11-10T01:02:52Z

Any idea why Travis is having a hard-time here?

************* Module fontbakery.profiles.outline

Lib/fontbakery/profiles/outline.py:1:0: E0401: Unable to import 'beziers.path' (import-error)

https://travis-ci.org/github/googlefonts/fontbakery/jobs/742549872#L264

chrissimpkins · 2020-11-10T01:50:13Z

I think it may be reasonable to mention this high likelihood of false positives in the rationale text. And also suggest that users should consider looking at some of the results from time-to-time at least to double-check the correctness of their designs.

Simon's point is a good one. I think it boils down to volume. Tens of false positives per font is a nuisance. Hundreds per font breaks the tool. Users would need to filter out the check(s) or all WARN and lower logging in order to understand what is going on with all other issues that are checked in the universal profile.

chrissimpkins · 2020-11-10T01:57:47Z

It is fairly slow, but not crazy slow - 4 seconds to process Source Sans Pro on my machine.

This is for a single font or a full
family?

felipesanches · 2020-11-10T02:25:58Z

Simon's point is a good one. I think it boils down to volume.

Yes, I agree!
Maybe the check could count the number of glyphs with problems and only report them explicitely if they are just a few cases. Otherwise we could come up with some other mechanism to get a detailed report only upon user demand.

Unfortunately, this makes me anxious because I believe we do already have too many verbosity control flags and I do not want to suggest yet another one in here :-P

simoncozens · 2020-11-11T10:20:42Z

OK, so I've:

Added lots of rationale text, explaining about which tests generate false-positives and what you should do about them.
Capped the list for each "heuristic" test at 100 warnings, at which point it just gives up and reports a PASS because there situation is so common as to probably be by design, rather than accidental.
Fixed a couple of bugs.

Any idea why Travis is having a hard-time here?

Because you have a set of requirements in requirements.txt and a different set of requirements in setup.py. $RUDE_WORD.

This is for a single font or a full family?

I've made it a bit faster (by making it give up testing after 100 warnings), so now it's 2s a font, 23 seconds for the whole Source Sans family.

felipesanches · 2020-11-11T15:49:10Z

setup.py

@@ -73,6 +73,7 @@
        'ttfautohint-py',
        'ufolint',
        'unidecode',
+        'beziers>=0.1.0'


if versions before 0.1.0 are not compatible, please add a comment here describing the reason. Otherwise, do not specify a minimum version requirement here on setup.py

felipesanches · 2020-11-11T16:10:15Z

Capped the list for each "heuristic" test at 100 warnings

a hundred WARNs is still a whole lot of output from a verbosity point of view. Maybe we could shorten it to just a dozen or so?

felipesanches · 2020-11-11T16:14:28Z

Lib/fontbakery/profiles/outline.py

+    segments. As this test is liable to generate significant numbers of false
+    positives, the test will pass if there are more than {FALSE_POSITIVE_CUTOFF}
+    reported short segments.
+""",


Rationale text must use 8-spaces of indentation and must not be manually line-wrapped. FontBakery reporters will take care of performing text layout optimized for each type of output generated (HTML, Markdown, text-terminal, etc)

The test tester should test for this!

check tester!

Yes, indeed, the minimum indentation level could be auto detected with a code-test. But only a human can grasp the difference between line breaking to separate paragraphs versus line-braking in the middle of sentences to manually fit text into some arbitrary text-block max char per line.

simoncozens · 2020-11-11T16:23:47Z

Capped the list for each "heuristic" test at 100 warnings

a hundred WARNs is still a whole lot of output from a verbosity point of view. Maybe we could shorten it to just a dozen or so?

It's only one WARN, using pretty_print_list to cap the output at 10 lines. But the point is that if there are more than 100 glyphs with the problem, you get a PASS instead.

felipesanches · 2020-11-11T18:17:39Z

Capped the list for each "heuristic" test at 100 warnings

a hundred WARNs is still a whole lot of output from a verbosity point of view. Maybe we could shorten it to just a dozen or so?

It's only one WARN, using pretty_print_list to cap the output at 10 lines. But the point is that if there are more than 100 glyphs with the problem, you get a PASS instead.

That's perfect! Thanks :-)

felipesanches · 2020-11-11T18:21:43Z

I'm happy with this PR and I will squash&merge as soon as we get a green on its last Travis (because I master-rebased it right now)

simoncozens · 2020-11-11T19:04:40Z

Needs changelog.

felipesanches · 2020-11-11T19:08:20Z

Needs changelog.

Indeed! Please add an entry. Thanks for remembering that! ;-)

felipesanches · 2020-11-11T19:16:18Z

Lib/fontbakery/profiles/outline.py

+                )
+
+    if warnings:
+        formatted_list = "\t* " + pretty_print_list(list(set(warnings)), sep="\n\t* ")


please also perform sorting on this list due to issue #3038

felipesanches · 2020-11-11T19:18:16Z

Lib/fontbakery/profiles/outline.py

+                        warnings.append(f"{glyphname}: {s}")
+
+    if warnings:
+        formatted_list = "\t* " + pretty_print_list(list(set(warnings)), sep="\n\t* ")


sorting here as well (#3038)

felipesanches · 2020-11-11T20:31:26Z

yeah, I think they should all be included in universal (unless someone disagrees ;-D)

I also noticed that we haven't yet included the outlines profile into the universal profile. Please do so. You can take a look (as an example of how this is done) at the universal profile itself, where it includes by default all checks from the opentype profile.

felipesanches · 2020-11-11T20:32:47Z

alternatively, if you prefer, you can leave it to me by opening an issue and I will then address this ASAP after we finish work on this PR

tphinney · 2020-11-11T21:55:54Z

Capped the list for each "heuristic" test at 100 warnings

a hundred WARNs is still a whole lot of output from a verbosity point of view. Maybe we could shorten it to just a dozen or so?

Rather than reporting a PASS when there are a large number of warnings, or itemizing them all at length, perhaps just note the number of warnings and the first ten or dozen glyphs that exemplify the problem?

(Side note: a pet peeve of mine is a test that turns off itemizing altogether when there are a large number of issues. Even a few examples can be very helpful.)

RosaWagner · 2020-11-12T09:45:42Z

Nice checks ! We could probably join forces with the outline quality checklist we are working on with Vivi. It is intended for designers and their source files, although, it lists all kind of path issues that resist Fontmake.

We could sync our rationales, or have shorts ones in fontbakery which link to this checklist which could be more elaborate (and more accurate, please help us)
Components/composites/compounds section could be nice checks to add.

simoncozens · 2020-11-12T17:34:51Z

Rather than reporting a PASS when there are a large number of warnings, or itemizing them all at length, perhaps just note the number of warnings and the first ten or dozen glyphs that exemplify the problem?

I've done this in cases where the algorithm can be fairly sure that there really is a problem. But, for example, the short-segments finder fires a lot in certain kinds of design, but hardly at all in others. If it seems like the designer is deliberately using short segments, that's OK, and a test warning about them is just noise. If they're not deliberately using short segments, and we only find a few of them, it's worth flagging them up.

felipesanches · 2020-11-12T19:20:32Z

Thanks, @simoncozens! I will now commit a set of minor tweaks to code-style here and then merge it

google-cla · 2020-11-12T19:36:41Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

felipesanches · 2020-11-12T19:50:20Z

@googlebot I consent.

felipesanches · 2020-11-12T21:15:14Z

:-D

simoncozens added 4 commits November 9, 2020 11:45

Add beziers.py requirement

e1da1f6

"Wonky" version of Source Sans for path tests

6dbc254

Add "is_not_variable_font" condition

9858d5c

Various path tests

90a844d

* Alignment against baseline/x-height/ascender/descender/etc. * Short segments * Colinear vectors * Jaggy segments (fixes #3064) * Semi-vertical / semi-horizontal lines

felipesanches reviewed Nov 9, 2020

View reviewed changes

tests/profiles/path_test.py Outdated Show resolved Hide resolved

felipesanches reviewed Nov 9, 2020

View reviewed changes

tests/profiles/path_test.py Outdated Show resolved Hide resolved

felipesanches reviewed Nov 9, 2020

View reviewed changes

Don't use TTFont, fix bad test

00afd64

simoncozens added 4 commits November 9, 2020 14:25

Improve short segment algorithm

d4ee351

Test short segment algorithm

53e2388

Improve error reporting (and spelling) for colinear test

dc25d78

Rename path->outline

c8cfada

felipesanches reviewed Nov 9, 2020

View reviewed changes

simoncozens added 2 commits November 9, 2020 20:43

Remove unused vars

200ccd7

sort output

aff8324

felipesanches reviewed Nov 11, 2020

View reviewed changes

simoncozens and others added 3 commits November 11, 2020 16:26

Fix requirements

7ae5733

Don't wrap rationales

1bcb791

Merge branch 'master' into path-tests

f9e453f

felipesanches reviewed Nov 11, 2020

View reviewed changes

simoncozens added 3 commits November 12, 2020 17:35

Sort warning output

880ca91

Update changelog

2c8def4

Too many close brackets

36bbf0a

felipesanches added 3 commits November 12, 2020 16:32

tweaks to code-style

f7c5c66

import outline checks in the universal profile

9967e7c

update CHANGELOG

0382c0f

felipesanches merged commit a78cb80 into fonttools:master Nov 12, 2020

felipesanches changed the title ~~Path tests~~ Glyph outline checks Nov 12, 2020

Glyph outline checks #3088

Glyph outline checks #3088

Conversation

simoncozens commented Nov 9, 2020 • edited

Description

To Do

felipesanches Nov 9, 2020

Choose a reason for hiding this comment

felipesanches commented Nov 9, 2020

simoncozens commented Nov 9, 2020

felipesanches commented Nov 9, 2020

graphicore commented Nov 9, 2020

chrissimpkins commented Nov 9, 2020 • edited

felipesanches commented Nov 9, 2020

chrissimpkins commented Nov 9, 2020

felipesanches Nov 9, 2020

Choose a reason for hiding this comment

felipesanches commented Nov 9, 2020

simoncozens commented Nov 9, 2020 • edited

felipesanches commented Nov 10, 2020

felipesanches commented Nov 10, 2020

chrissimpkins commented Nov 10, 2020

chrissimpkins commented Nov 10, 2020

felipesanches commented Nov 10, 2020

simoncozens commented Nov 11, 2020

felipesanches Nov 11, 2020

Choose a reason for hiding this comment

felipesanches commented Nov 11, 2020

felipesanches Nov 11, 2020

Choose a reason for hiding this comment

simoncozens Nov 11, 2020

Choose a reason for hiding this comment

felipesanches Nov 11, 2020

Choose a reason for hiding this comment

simoncozens commented Nov 11, 2020

felipesanches commented Nov 11, 2020

felipesanches commented Nov 11, 2020

simoncozens commented Nov 11, 2020

felipesanches commented Nov 11, 2020

felipesanches Nov 11, 2020 • edited

Choose a reason for hiding this comment

felipesanches Nov 11, 2020 • edited

Choose a reason for hiding this comment

felipesanches commented Nov 11, 2020

felipesanches commented Nov 11, 2020

tphinney commented Nov 11, 2020

RosaWagner commented Nov 12, 2020

simoncozens commented Nov 12, 2020

felipesanches commented Nov 12, 2020

google-cla bot commented Nov 12, 2020

felipesanches commented Nov 12, 2020

felipesanches commented Nov 12, 2020

simoncozens commented Nov 9, 2020 •

edited

chrissimpkins commented Nov 9, 2020 •

edited

simoncozens commented Nov 9, 2020 •

edited

felipesanches Nov 11, 2020 •

edited

felipesanches Nov 11, 2020 •

edited