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 check: Grade axis doesn't have any reflow #3187

Closed
davelab6 opened this issue Mar 4, 2021 · 19 comments
Closed

Add check: Grade axis doesn't have any reflow #3187

davelab6 opened this issue Mar 4, 2021 · 19 comments
Assignees
Labels
New check proposal We expect new check proposals to include a detailed rationale description and a suggested check-id P2 Important
Milestone

Comments

@davelab6
Copy link
Contributor

davelab6 commented Mar 4, 2021

Observed behaviour

The grade (GRAD) axis should not change any advanceWidth or kerning data across its design space.

Expected behaviour

We should have a FB check for that.

Resources and exact process needed to replicate

@madig wrote this script for @chrissimpkins to report advance width issues. Kerning is a good 2nd step after this check ships.

# flake8: noqa

from pathlib import Path

from ufoLib2 import Font


TARGET_DIR = Path("source/")
DEFAULT = {
    "Family-opsz17-wght380-GRAD-50.ufo": "Family-opsz17-wght380-GRAD0.ufo",
    "Family-opsz17-wght380-GRAD200.ufo": "Family-opsz17-wght380-GRAD0.ufo",
    "Family-opsz18-wght380-GRAD-50.ufo": "Family-opsz18-wght380-GRAD0.ufo",
    "Family-opsz18-wght380-GRAD200.ufo": "Family-opsz18-wght380-GRAD0.ufo",
    "FamilyItalic-opsz17-wght380-GRAD-50.ufo": "FamilyItalic-opsz17-wght380-GRAD0.ufo",
    "FamilyItalic-opsz17-wght380-GRAD200.ufo": "FamilyItalic-opsz17-wght380-GRAD0.ufo",
    "FamilyItalic-opsz18-wght380-GRAD-50.ufo": "FamilyItalic-opsz18-wght380-GRAD0.ufo",
    "FamilyItalic-opsz18-wght380-GRAD200.ufo": "FamilyItalic-opsz18-wght380-GRAD0.ufo",
}

comparison_ufos = {}

for ufo_path, comparison_ufo_path in DEFAULT.items():
    ufo = Font.open(TARGET_DIR / ufo_path)
    if comparison_ufo_path in comparison_ufos:
        comparison_ufo = comparison_ufos[comparison_ufo_path]
    else:
        comparison_ufo = Font.open(TARGET_DIR / comparison_ufo_path)
        comparison_ufos[comparison_ufo_path] = comparison_ufo

    mismatches = [
        glyph.name for glyph in ufo if glyph.width != comparison_ufo[glyph.name].width
    ]
    if mismatches:
        print(f"UFO {ufo_path} width mismatches compared to {comparison_ufo_path}:")
        print("\n".join(f"  {x}" for x in mismatches))
@davelab6 davelab6 added this to the 0.7.35 milestone Mar 4, 2021
@chrissimpkins
Copy link
Member

Apache License 2.0. Gtg if any of the source is helpful.

@felipesanches
Copy link
Collaborator

this code-snippet references some files. Can you provide them as well (under a free license) so that we can refactor this into a fontbakery check?

@felipesanches
Copy link
Collaborator

I mean... to use the files in the code-test for the proposed new check

@felipesanches felipesanches added the New check proposal We expect new check proposals to include a detailed rationale description and a suggested check-id label Mar 4, 2021
@felipesanches
Copy link
Collaborator

"The grade (GRAD) axis should not change any advanceWidth or kerning data across its design space."

@davelab6 has this happened in a real font project? If so, can you point me to it so that we have real-world samples of the problem?

@chrissimpkins
Copy link
Member

chrissimpkins commented Mar 4, 2021

Sorry those aren't available to distribute under a free license but we can make a simple test case font. A positive test case includes an advance width that changes across masters that do not differ by anything other than GRAD.

e.g.,

  • wght=400, opsz=6, GRAD=-50
  • wght=400, opsz=6, GRAD=0
  • wght=400, opsz=6, GRAD=200

This assumes that GRAD should never change advance width. I think that this is a correct assumption based on how it is defined.

@felipesanches
Copy link
Collaborator

cool, thanks.

the code snippet deals with UFOs. Could this be checked on a VF ttf intead?

FontBakery can check UFOs, but we typically implement checks on TTFonts

@chrissimpkins
Copy link
Member

It may be possible to do it with wizardry that I do not possess using fonttools. I'd definitely be interested to know how to do that.

We discussed use of uharfbuzz shaper output to do this at the binary level. That should be straightforward but adds a dependency.

@chrissimpkins
Copy link
Member

but adds a dependency.

Though I think that Simon is working on the addition of shaping regression testing here? Perhaps uharfbuzz based?

@felipesanches
Copy link
Collaborator

if a new dependency is needed for something important/useful and it is a simple python package, then I have no worries adding it. Installation of python dependencies is trivial both locally as well as on our continuous integration setup.

I think @simoncozens was using something custom on his shaping check, that's why we have not merged that one yet. But he's also planning to make that custom code into a python package, so that will solve the dependency problem in that case.

@chrissimpkins
Copy link
Member

chrissimpkins commented Mar 4, 2021

If Simon's dependency will be coming in soon, it may be best to see if he feels that we can implement this on his shaping lib.

I spoke with David Berlow about this today. He told me that, given fixed values on all other design axes, there should be a constant adv width as GRAD changes. This is a valid assumption and the way that GRAD is intended to work. I think that this could land in a universally applicable test profile for testing on any project that includes a GRAD axis. He did raise the issue of avar on GRAD 🤯 . But I don't think that this changes the test. For a given glyph with all other design axes at fixed values, adv width should be a constant value over the entire axis so it doesn't matter how GRAD bends off of a line from min to max. That only influences the "rate" of things that do change.

@chrissimpkins
Copy link
Member

chrissimpkins commented Mar 5, 2021

Here's a test font. Only includes numeral one (U+0031) with different widths across the three GRAD masters (GRAD -50, 0, 200). Source and designspace file included in case you need to expand further for the tests.

You can build with:

$ fontmake -m BadGrades.designspace -o variable

BadGrades.zip

@davelab6
Copy link
Contributor Author

davelab6 commented Mar 5, 2021 via email

@simoncozens
Copy link
Collaborator

You don’t need a shaping engine for this. Just check the gvar table. There’ll be a phantom point at the end of each glyph which shows the delta to the horizontal advance.

@kontur
Copy link
Contributor

kontur commented Mar 5, 2021

This assumes that GRAD should never change advance width. I think that this is a correct assumption based on how it is defined.

Out of curiosity where is this axis defined?

@m4rc1e
Copy link
Collaborator

m4rc1e commented Mar 5, 2021

@kontur https://github.com/google/fonts/blob/main/axisregistry/grade.textproto

@madig
Copy link
Contributor

madig commented Mar 5, 2021

from fontTools.ttLib import TTFont
from fontTools.ttLib.tables._g_v_a_r import table__g_v_a_r

font = TTFont("secretproject.ttf")
gvar: table__g_v_a_r = font["gvar"]
for glyph, deltas in gvar.variations.items():
    for delta in deltas:
        if "GRAD" not in delta.axes:
            continue
        if any(c is not None and c != (0, 0) for c in delta.coordinates[-4:]):
            print(glyph)
            break

@simoncozens like this?

@simoncozens
Copy link
Collaborator

That's what I would do. I presume you're posting it because you've tested it and it works. :-) You could also check HVAR.

@madig
Copy link
Contributor

madig commented Mar 5, 2021

Cosimo says:

a value of None means it's an inferred delta
it doesn't mean it going to be inferred as 0,0
that depends on the varLib.iup algorithm
tuplevariations have a calcInferredDeltas() method, check the instancer to see how that works

Also: kerning deltas need to be checked.

@anthrotype
Copy link
Member

i checked varLib.iup code, and the last four phantom points are treated specially (see iup_delta and iup_contour functions), in the sense that if deltas are None, they will be always inferred as (0, 0). so I think you're safe to ignore IUP for those 4

@felipesanches felipesanches modified the milestones: 0.7.35, 0.7.36 May 12, 2021
@felipesanches felipesanches modified the milestones: 0.7.37, 0.7.x May 20, 2021
@felipesanches felipesanches removed this from the 0.7.x milestone Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New check proposal We expect new check proposals to include a detailed rationale description and a suggested check-id P2 Important
Projects
None yet
Development

No branches or pull requests

8 participants