Implements part of #754 via getter/setters for proxying the file format 3 attributes

[kontur]

This implements writing postscriptBlueValues, postscriptOtherBlues, postscriptStemSnapV and postscriptStemSnapH for glyphs > ufo.

I added ./tests/glyphs3_test.py and other tests are passing as well.

I'm not sure how elegant or clumsy it is to have those GSFontMaster._... underscore internals and accessing them via the getter/setter proxies, but it seemed the most practical way synthesize them from the GSFont attributes required for the offsets.

There are this, this & this rather blunt stop gaps in the getters which mainly function to return nothing for cases where the masters are constructed via code, e.g. in the tests (about 50 or so fail otherwise). In a way this could mask a real issue as well, but not sure how realistic it is to rewrite the tests, or add some kind of "defaulting" structures to keep the indexes for constructed objects in sync between GSFont and GSFontMaster.

Should I squash my commits into one? The first commit attempts were a bit hit and miss...

[kontur]

Nvm, squashed the commits into one anyway, seemed cleaner :)

[kontur]

Any chance to get this reviewed? :)

[anthrotype]

just left minor comments below. Ideally @simoncozens could take another look

To fix the CI erros you can try to rebase on main branch (or merge origin/main into yours) so that it pulls updated requirements

[kontur]

Thanks for those suggestions @anthrotype — I revised the tests and checking some. After rebasing I get 17 test failures related to instancing, but those appear present also in main.

[kontur]

Rebased on main again, but there appear to be a lot of unrelated tests failing. What's the deal with those, are those residue from merged in WIP and the respective features will still fix those, or is there an effort to just tackle them as separate task?

[anthrotype]

there appear to be a lot of unrelated tests failing.

the CI checks seem to be passing now, not sure what you're referring to: https://github.com/googlefonts/glyphsLib/actions/runs/3627890015

I'll take another review pass

[kontur]

I'm manually running e.g. tox -e py310 with a lot of failing tests in my branch (that originate from unrelated parts of the codebase, as far as I can tell) — would this differ from what the CI will run, e.g. are there some tests marked somehow to not be considered in the CI workflow?

[anthrotype]

the regression tests failing is expected when adding new features from a PR branch, but once that's merged to main, they fix themselves (the regression test CI job compares output from PR branch with that from main branch).

One needs to manually check that the changes are as expected:
https://github.com/googlefonts/glyphsLib/actions/runs/3627890014/jobs/6119377961

[anthrotype]

the CI also runs tox the same way as you do so that's unexpected.. let me try reproduce locally from a copy of your PR branch

[anthrotype]

try to also pass the -r flag to tox, to force recreating the tox virtual environment

[anthrotype]

I checked out your branch locally and run tox -e py310 and all the tests passed, same as on the CI, so I think there's some stuff in your local repo that's out of sync somehow.

[anthrotype]

the regression tests failures seem all benign, I can see that postscriptBlueValues are now being compiled which is what this PR is about

[kontur]

Ah, perfect, thanks for the -r hint, not very familiar with tox. Tests run locally without issues as well 🙇

[anthrotype]

Re: 'tox -r, sometimes tox isn't smart enough to understand that dependencies have changed in requirements.txt file, and doesn't automatically recreate the test venv, so you may have to force that

[schriftgestalt]

In general I would recommend to adapt the API of Glyphs 3 and make the builder aware of it instead of fitting the data into the Glyphs 2 API

[kontur]

@anthrotype yep, those type checks are better now, thanks for pointing that out.

regarding @schriftgestalt 's suggestion: I agree in general, but this would probably need a more orchestrated effort overall, no? it shouldn't just prioritise v3 handling for one attribute. I can rework this for the alignmentZones if that will otherwise prevent the PR from being merged. personally I just need to be able to work with v3 files, from releases preferably.

[anthrotype]

does glyphs.app python API still provide getters like alignmentZones returning a list of GSAlignmentZone objects for v3 as well as v2 fonts? If so, we could keep the old v2 API and file an issue to expose the new v3 API alongside it.

[schriftgestalt]

does glyphs.app python API still provide getters like alignmentZones returning a list of GSAlignmentZone objects

Yes, it does. But only as a compatibility measure. And the API doesn’t distinguish between version 2 and 3 files. In memory they are "upgraded" to version 3.

[simoncozens]

I said the other day to Cosimo that we should just support the G3 model and have a separate bit of code to “upgrade” G2 files on load. Trying to manipulate both systems is making the code atrocious, and G3 is much cleaner. But this would be a major overhaul.

[anthrotype]

where's this list of validZoneMetrics taken from? it seems constant, it should probably be defined at global scope, or in the constants module.

[anthrotype]

and why skip some of the font metric types as not "valid" when fetching a given master metric values? where is this logic encoded?

[kontur]

Basically I went by what's in the UI (mind you I missed bodyHeight):

Outputting the GSMetric.type of these objects of a file gives:

ascender
cap height
x-height
baseline
descender
italic angle
bodyHeight

The last empty line for type is what the UI has as "Custom Name" metric. This may or may not be an overshoot — presumably things with a two numeric value / position + overshoot we want to treat as an alignmentZone as well, no matter if it is not in what format 2 allowed as alignmentZones?

I think when I did a "has no overshoot" exclusion instead of explicitly checking for the type a lot of tests outright failed because the font metricValues does not match a given master's metricValues length (e.g. in all sorts of constructed test cases or programmatic access of those objects that don't adhere to the file format 3 referencing the font metricValues from the master by index). As far as I recall the "baseline": 0 default above also addressed this.

Note sure what to do here, but I'll ponder it.

[kontur]

Updated in code below in forced push. Disregarding the "italic angle" type of metrics early was key to avoiding all those issues in tests that construct just masters. Gets rid of the explicit magic list and does not require the baseline default either. Thanks for poking holes @anthrotype :)

[anthrotype]

... because the font metricValues does not match a given master's metricValues length (e.g. in all sorts of constructed test cases or programmatic access of those objects that don't adhere to the file format 3 referencing the font metricValues from the master by index

if it's invalid that len(master.metricValues) != len(font.metrics) then it is those tests that would need to be fixed...

[kontur]

The tests were correct, only when not filtering things that are now returned by a master'a metrics which are not alignmentZones in format 3 those test broke because without knowledge of the master my previous iteration of the code could not filter those properly. Different approach in the updated code, essentially discarding the italic angle from being an alignmentZone outright, and further removing metrics which do not have an overshoot, e.g. do not form a bluezone because they would be a "line" only.

[anthrotype]

can a valid "italic angle" metric ever have overshoot != 0? I suppose not. And if not, then simply filtering out metrics with overshoot=0 (without special-casing italic angle itself) should accomplish the same thing.

[kontur]

In here

 for index, fontMetric in enumerate(self.font.metrics):
            # Ignore the "italic angle" "metric", it is not an alignmentZone
            if fontMetric.type == "italic angle":
                continue
            metric = self.metrics[index]
            # Ignore metric without overshoot, it is not an alignmentZone
            if metric.overshoot == 0:
                continue
            zone = GSAlignmentZone(pos=metric.position, size=metric.overshoot)
            zones.append(zone)

self.font.metrics is a GSMetric value which has no overshoot. One has to use the index of this font metric values to get the master GSMetricValue value, and then discard things which have no overshoot. Filtering the font metric values by type italic angle is necessary because just accessing master.metrics[] by index will not have a value for a master metric that is an italic angle.

The format 3 handling of metric values referencing back up to the font metrics list is convoluted imo.

[anthrotype]

looks like alignmentZones is a read-only (https://docu.glyphsapp.com/#GSFontMaster.alignmentZones) so maybe we should not add a setter.
I agree with @simoncozens when he wrote

We should store things as v3 format internally to the object but provide accessors to get at information in a v2 way.

[kontur]

Some of the existing tests assign alignmentZones like this for constructing objects via code only (as opposed to actual conversion). Happy to drop the setter and tests.

[anthrotype]

If one wants to set these alignment zones using Glyphs.app v3 scripting API (where alignmentZones is read-only), how would one do that exactly?
Does glyphsLib already support this? If not, what do we need to support it?

[kontur]

Going by what I found in the code. The primary use case in the library seems to be here setting the alignmentZones when converting from UFO to glyphs.

Also tests here, here, here and here — my assumption being that a new feature or refactor should not break existing tests and thus a setter needs to be supported.

[anthrotype]

i'm ok to keep a setter for alignmentZones, but I wonder maybe it should store them internally as v3 metricValues.
Say you go from UFO => glyphs and want to save as format_version=3, those zones will not be written out in the generated .glyphs file.
I think we should have a method to convert from v2 (alignmentZones and stems) to v3 metricValues and call that when saving a font and format_version=3.

[anthrotype]

but that setter only sets self._alignmentZones, which as far as I understand is not written out to .glyphs source when format_version=3,

        if writer.format_version == 2:
            writer.writeObjectKeyValue(self, "alignmentZones", "if_true")

[anthrotype]

so I presume that roundtripped glyphs file of yours has format version 2.. but when Glyphs.app v3 opens it up it automatically populate the metric values.
That's what we are missing here, a way to upgrade these metrics from v2 to v3. I am not saying that we should do within this same PR, just that this is something we would like to support

[kontur]

Ah, I see, didn't understand that's what's happening. Yes, correct.

I'll have a look at that, either as part of this or separate. Follows the same obtuse logic of font metrics vs master metric 😄 Happy to adopt that if there is a more general v2 > v3 structure in place. That section of code you quoted, I suppose, is at the heart of what would need a more elegant and transparent file format version upgrade.

[kontur]

How does one actually write something out in format 3? Tried to find tests that verify those other self.format == 3 written things (e.g. here onwards), but could only find this one. Am I completely off track or is there pretty little actually for outputting/writing/testing v3? Would testing the output of the writer in writer_tests.py the right spot to implement checking for a yet-to-be-done implementation of writing those metrics back to v3 files?

[schriftgestalt]

but could only find this one.

This looks like the way to do this. While writing out, check the format and what data you have and act accordingly.

[kontur]

Rebased and refactored the alignmentZones getter code filtering that determined what Metric is or is not an alignmentZone.

[anthrotype]

@kontur can you fix the lint job failure? then I suppose we can merget this one, as at least allows one to psautohint a font (/cc @guidoferreyra). We can keep #754 open to address the things discussed in #825 (comment) and the general issue of "a more elegant and transparent file format version upgrade".

[kontur]

@anthrotype Ah sorry, must have missed the lint issues locally and didn't follow the updates of the test runner here. Fixed and rebased on main once more, gtg from my part.

[kontur]

patiently waiting

[anthrotype]

thanks for the patience