Fix "Axis Mappings" interpretation #1040

simoncozens · 2024-10-31T09:02:13Z

This fixes #993. We look at the file format version, app version and instance mapping to determine the correct interpretation of the "Axis Mappings" custom parameter.

anthrotype · 2024-10-31T10:04:56Z

tests/builder/axes_test.py

+
+def test_axis_mappings_different_interpretations(ufo_module, datadir):
+    # https://github.com/googlefonts/glyphsLib/issues/859
+    font = GSFont(datadir.join("gf", "Rubik.glyphs"))  # Saved under Glyphs 3079


this Rubik.glyphs seems to already have a correct user->design mapping even if it was saved with an old version:

https://github.com/googlefonts/glyphsLib/blob/1cb4fc5ae2cf385df95d2b7768e7ab4eb60a5ac3/tests/data/gf/Rubik.glyphs#L49-L59

Right, because it was a format 2 file.

anthrotype · 2024-10-31T10:07:04Z

Lib/glyphsLib/builder/axes.py

+    # We want to return a userspace->designspace map for our purposes
+    inverted = {v: k for k, v in mapping.items()}
+    if int(self.font.appVersion) >= 3168:
+        # Mapping is *definitely* designspace->userspace, invert
+        return inverted


i'm confused, I thought older versions (< 3168?) would interpret it as design->user and thus require flip, whereas newer versions have aligned with fontmake's interpretation and already have user->design? This block seems to contradict my understanding, for it's using an inverted mapping if the version is more recent.

"Interpret as" is perhaps not helpful here. :-) I just opened Nunito source, converted to Glyphs 3 file format and saved under Glyphs 3310. The custom parameter is stored as:

wght = { 42 = 200; 61 = 300; 81 = 400; 91 = 500; 101 = 600; 125 = 700; 151 = 800; 178 = 900; 208 = 1000; };

i.e. designspace to userspace.

now I am extremely confused

Thankfully the code has comments and the tests are passing. :-) (And I can compile fonts that were not compiling before.)

well, I for one don't understand this whole mess. It would be nice to have a recap.

I can compile fonts that were not compiling before

good, what I'm worried about are the other fonts that were compiling before and may not after this.
Right now (tested with 3324) on a newly created glyphs3 file, the Axis Mappings is already stored in the source as user->design (or external->internal, as Glyphs calls them), so it is correct and matches fontmake. Wouldn't this PR invert it? I'm seeing

if int(self.font.appVersion) >= 3168: return inverted

You are going to love this.

Glyphs now stores user->design for new fonts, but it "upgrades" old fonts by simply inverting them. Take your new font, manually edit "appVersion" to (say) 3110, open it in Glyphs and save it again - the mappings will be flipped.

So I guess this means we can't trust the app version number, and we have to probe against the instance mapping.

We could have went with my suggestion (back in #745) and made glyphsLib match (old) GlyphsApp behavior and fixed the few fonts that would have been broken by this. Now it is even a bigger mess...

I don't deny that it's stupid and wrong and there may have been questionable choices on both sides, but in 95% of cases the interpretation is completely obvious. Do you have a weight uservalue of 7? You did something wrong. Did all the master locations suddenly fall outside of the design space and you have no valid masters any more? You got the axis mappings backwards. Compare against the instance locations, because we know which way round they are, and if the ranges are wildly different, switch it. It's a mess, but it's not a hard mess to clean up.

anthrotype · 2024-10-31T10:08:44Z

Lib/glyphsLib/builder/axes.py

+    if self.font.format_version != 3:
+        # Glyphs wasn't using this custom parameter, only we were
+        return mapping
+    # Let's get heuristical


hm i'm a bit skeptical we need to go "heuristical".. Can't we simply check the version and do accordingly? If the source file doesn't match the interpretation for the version it was saved with, we can say it's not our problem

I think there were some fonts that would set it "wrong" in Glyphs to make it work with fontMake.

how about fixing the actual sources instead?

Lib/glyphsLib/builder/axes.py

anthrotype · 2024-10-31T17:30:35Z

we do already check the instances when there's no axis mappings nor axis location parameters to infer the master's user-space coordinates based on the instances, and that inference has already proved a bit brittle when the user reports unexpected "base master not fund" or weird interpolations because of unexpected mappings.
Now we are saying that Axis Mappings, which was supposed to give a full explicit definitions of the mappings, not tied to the masters or instances, can't be trusted and we'll still try to fix it by looking at the instances.. but the instances in a VF at least do not need to be there, nor they need to be defined at all the axes corners. Is there a chance that this new heuristic to fix up a few sources at compile time may end up screwing perfectly valid fonts?

anthrotype · 2024-10-31T17:32:11Z

(i hate when I forget a not and the whole sentence stops making sense)

anthrotype · 2024-10-31T17:39:01Z

besides, the inference of user->design mappings from the instances is only meant to work for weight and width, IIRC. And it's unlikely that these instances also have Axis Location custom parameter if there's already an Axis Mappings, which in theory trumps Axis Location as it's more general

simoncozens · 2024-10-31T18:01:56Z

Ok. So how would you like to fix the issue?

schriftgestalt · 2024-10-31T20:48:20Z

I would not add any axis mapping if there is nothing explicitly in the file. Maybe issue a warning.

simoncozens · 2024-10-31T22:09:29Z

That doesn't fix the problem. The problem is we can't build fonts because the Axis Mappings that are in the file aren't interpreted correctly.

schriftgestalt · 2024-10-31T23:02:43Z

There are only very few files that have those faulty axis mappings. That has to be caught in QA and fixed in the files.

simoncozens · 2024-11-01T08:05:29Z

faulty axis mappings

OK, one last time. These are not faulty for Glyphs.app users. Glyphs.app compiles these things just fine, and produces VFs with correct avar mappings. glyphsLib does not. A good example is Cormorant.glyphs.

The whole purpose of glyphsLib is to compile Glyphs files so we don't have to use Glyphs.app in an open source toolchain. I don't think we should decide that we're just not going to bother to compile valid Glyphs files that work fine in Glyphs.

schriftgestalt · 2024-11-01T10:05:03Z

Then misunderstood the problem. Sorry.
But Glyphs doesn’t export a working avar for Cormorant. It needs "Axis Location" parameters in the master settings.

schriftgestalt · 2024-11-01T10:07:56Z

As I understand it (and how Glyphs handles it) is such:
Use the axis range from the masters (either the coordinates or the axis locations; those are called internal and external coordinates in the Glyphs 4 API) for fvar and such.
Then use the "Axis Location" in the instances or "Axis Mapping" in the font to build a avar table.

simoncozens · 2024-11-01T10:16:23Z

Urgh, you're right.

anthrotype · 2024-11-01T12:02:09Z

Cormorant.glyhps contains these incorrect mappings, with keys/values swapped:

{
name = "Axis Mappings";
value = {
wght = {
40 = 300;
48 = 400;
58 = 500;
69 = 600;
80 = 700;
};

Even Glyphs.app (version 3.3 build 3324) displays the first column (the keys) as "external" and the second column (the values) as "internal" (whereas it should be the other way around, but Glyphs.app is just reading what's in the source).

If I just look at the source, I could say with confidence that the source itself contains a bug. Maybe it was not set up like this by the font developer, and the source originally contained mappings in the order that fontmake expects but then got automatically "fixed" by Glyphs.app, I don't know. But it still remains buggy. And even Glyphs.app itself can't export a correct avar from this, not just fontmake.

The way I'd suggest fixing this is by modifying the custom parameter in the source, swapping the values with keys.

anthrotype · 2024-11-01T12:04:15Z

It needs "Axis Location" parameters in the master settings.

for fontmake at least, you don't need both Axis Mappings and Axis Locations because the former encompasses the latter and thus instead of risking duplicating the same information twice (or having them out of sync), we only look at Axis Mappings if present, otherwise we go find Axis Locations.

simoncozens added 2 commits October 31, 2024 08:59

Test case for axis mappings custom parameter

8f02119

Heuristically invert axis mappings

ef4a21f

simoncozens requested a review from anthrotype October 31, 2024 09:49

anthrotype reviewed Oct 31, 2024

View reviewed changes

Lib/glyphsLib/builder/axes.py Show resolved Hide resolved

simoncozens added 2 commits October 31, 2024 16:53

Trust no-one

473bfea

Ensure there is an instance mapping

39c3331

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "Axis Mappings" interpretation #1040

Fix "Axis Mappings" interpretation #1040

simoncozens commented Oct 31, 2024

anthrotype Oct 31, 2024

simoncozens Oct 31, 2024

anthrotype Oct 31, 2024 •

edited

Loading

simoncozens Oct 31, 2024

anthrotype Oct 31, 2024

simoncozens Oct 31, 2024

anthrotype Oct 31, 2024

anthrotype Oct 31, 2024 •

edited

Loading

simoncozens Oct 31, 2024

khaledhosny Oct 31, 2024

simoncozens Oct 31, 2024 •

edited

Loading

anthrotype Oct 31, 2024

schriftgestalt Oct 31, 2024

simoncozens Oct 31, 2024

anthrotype Oct 31, 2024

anthrotype commented Oct 31, 2024 •

edited

Loading

anthrotype commented Oct 31, 2024 •

edited

Loading

anthrotype commented Oct 31, 2024 •

edited

Loading

simoncozens commented Oct 31, 2024

schriftgestalt commented Oct 31, 2024

simoncozens commented Oct 31, 2024

schriftgestalt commented Oct 31, 2024

simoncozens commented Nov 1, 2024

schriftgestalt commented Nov 1, 2024

schriftgestalt commented Nov 1, 2024

simoncozens commented Nov 1, 2024

anthrotype commented Nov 1, 2024

anthrotype commented Nov 1, 2024 •

edited

Loading

Fix "Axis Mappings" interpretation #1040

Are you sure you want to change the base?

Fix "Axis Mappings" interpretation #1040

Conversation

simoncozens commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthrotype Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthrotype Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simoncozens Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthrotype commented Oct 31, 2024 • edited Loading

anthrotype commented Oct 31, 2024 • edited Loading

anthrotype commented Oct 31, 2024 • edited Loading

simoncozens commented Oct 31, 2024

schriftgestalt commented Oct 31, 2024

simoncozens commented Oct 31, 2024

schriftgestalt commented Oct 31, 2024

simoncozens commented Nov 1, 2024

schriftgestalt commented Nov 1, 2024

schriftgestalt commented Nov 1, 2024

simoncozens commented Nov 1, 2024

anthrotype commented Nov 1, 2024

anthrotype commented Nov 1, 2024 • edited Loading

anthrotype Oct 31, 2024 •

edited

Loading

anthrotype Oct 31, 2024 •

edited

Loading

simoncozens Oct 31, 2024 •

edited

Loading

anthrotype commented Oct 31, 2024 •

edited

Loading

anthrotype commented Oct 31, 2024 •

edited

Loading

anthrotype commented Oct 31, 2024 •

edited

Loading

anthrotype commented Nov 1, 2024 •

edited

Loading