[designspace] Add avar2 mapping support #3123

behdad · 2023-05-24T16:22:54Z

This just uses list of list of dictionaries. Should use proper objects.

#3049

behdad · 2023-05-24T16:23:17Z

cc @justvanrossum Opening this such that we can work on it together.

behdad · 2023-05-25T20:55:39Z

I updated this to actually build an avar2 table, and successfully built one.

Lorp · 2023-05-25T21:03:09Z

Is this based on my proposed <map> syntax?

behdad · 2023-05-25T21:05:38Z

Is this based on my proposed <map> syntax?

Yes. Just using <mapping> instead of <map>.

Lorp · 2023-05-25T21:08:59Z

Are you dealing with supporting masters, thus only calculating duovar deltas after calculating supporting monovar deltas etc?

behdad · 2023-05-25T21:10:03Z

Are you dealing with supporting masters, thus only calculating duovar deltas after calculating supporting monovar deltas etc?

It's a full VariationModel solver.

behdad · 2023-05-25T21:11:36Z

Based on this experiment, I suggest this way forward:

If an output axis is specified with xvalue, then it must be also specified as an input axis,
Otherwise, an output axis can have a delta attribute, that has a value in normalized (-1..+1) range to be added to the axis value.

I think this makes most sense. I've willing to hear alternatives.

Lorp · 2023-05-25T21:18:05Z

That sounds like an excellent refinement.

Can you confirm your avar2 builder is happy crossing 0? (If specified absolutely, such a delta must be worked out in two steps.)

behdad · 2023-05-25T21:21:53Z

That sounds like an excellent refinement.

Can you confirm your avar2 builder is happy crossing 0? (If specified absolutely, such a delta must be worked out in two steps.)

It looks like it's happy to. In fact, the varLib modeler requires a region at 0.

davelab6 · 2023-05-25T22:20:09Z

If an output axis is specified with xvalue, then it must be also specified as an input axis

I am not so familiar with the implemention, and was curious about the name of this variable; I pinged Behdad to ask, what is the x in xvalue, and he kindly explained,

it comes from mutatorMath, which supports non-uniform interpolation, so there's xvalue and yvalue; and if yvalue is omitted, it's copied from xvalue. But in varfonts we only support uniform interpolation, so you always only see xvalue

behdad · 2023-05-25T23:29:13Z

cc @LettError @justvanrossum

Lorp · 2023-05-26T10:02:21Z

Recall that there are valid concerns from @anthrotype @justvanrossum @belluzj about the terminology in the spec, so we should not regard it as firm. In particular I’m happy to avoid use of "xvalue". See #3049.

LettError · 2023-05-26T10:04:29Z

"xvalue" is there to record anisotropic locations. This is perhaps not very useful for variable font but it is where this specification started and it is still used during design. As long as anisotropy remains possible I'm fine with any other name.

Lib/fontTools/designspaceLib/__init__.py

behdad · 2023-05-26T16:57:26Z

Based on this experiment, I suggest this way forward:

If an output axis is specified with xvalue, then it must be also specified as an input axis,

Otherwise, an output axis can have a delta attribute, that has a value in normalized (-1..+1) range to be added to the axis value.

This makes it ugly to represent in the code. Currently how it works, is that if an axis is only specified in output, then it will work as a delta from the axis's default value. Maybe we just document it that way and go home? @Lorp

behdad · 2023-05-26T19:12:41Z

@Lorp Can you work on a patch to the designspace spec?

behdad · 2023-05-26T19:18:33Z

@Lorp Instead of <map><region> what do you think of <mappings><mapping>?

behdad · 2023-05-26T19:18:57Z

@Lorp Instead of <map><region> what do you think of <mappings><mapping>?

What's what I'm using in the designspaceLib code.

Lorp · 2023-05-27T11:11:33Z

Sounds good. We don’t talk about regions anywhere else in designspace. I’m mostly away for the weekend, but will do it soon.

behdad · 2023-05-28T20:22:55Z

Should the dimension element have axis name instead of tag?

Tests/designspaceLib/designspace_test.py

Tests/designspaceLib/split_test.py

behdad · 2023-06-01T18:26:18Z

@anthrotype Thanks for the review. I think I addressed them all.

behdad · 2023-06-01T18:31:55Z

@khaledhosny Is this something you can use in your font to build the avar2 table?

khaledhosny · 2023-06-01T19:05:56Z

@khaledhosny Is this something you can use in your font to build the avar2 table?

Probably. I create a DS in code and pass it to varLib, so I can add the mapping there.

khaledhosny · 2023-06-01T21:03:09Z

@khaledhosny Is this something you can use in your font to build the avar2 table?

Probably. I create a DS in code and pass it to varLib, so I can add the mapping there.

aliftype/raqq@d5c7fd6

But I don’t understand these warnings, what am I doing wrong?

No input location specified for non-hidden axis 'SPAC' in axis mapping {'jstf': 0.0}; axis default value assumed.
No input location specified for non-hidden axis 'SPAC' in axis mapping {'jstf': 0.9}; axis default value assumed.
No input location specified for non-hidden axis 'SPAC' in axis mapping {'jstf': 1.0}; axis default value assumed.
No input location specified for non-hidden axis 'SPAC' in axis mapping {'jstf': -0.5}; axis default value assumed.
No input location specified for non-hidden axis 'SPAC' in axis mapping {'jstf': -1.0}; axis default value assumed.
No input location specified for non-hidden axis 'MSHQ' in axis mapping {'jstf': 0.0}; axis default value assumed.
No input location specified for non-hidden axis 'MSHQ' in axis mapping {'jstf': 0.9}; axis default value assumed.
No input location specified for non-hidden axis 'MSHQ' in axis mapping {'jstf': 1.0}; axis default value assumed.
No input location specified for non-hidden axis 'MSHQ' in axis mapping {'jstf': -0.5}; axis default value assumed.
No input location specified for non-hidden axis 'MSHQ' in axis mapping {'jstf': -1.0}; axis default value assumed.

khaledhosny · 2023-06-01T21:06:16Z

This fixes the warnings:

diff --git a/scripts/build.py b/scripts/build.py
index 8d19a4a..63d4531 100644
--- a/scripts/build.py
+++ b/scripts/build.py
@@ -726,11 +726,11 @@ def build(font, instance, args):
         ds.addSource(source)

     inputs = [
-        {"Justification": 0},
-        {"Justification": 90},
-        {"Justification": 100},
-        {"Justification": -50},
-        {"Justification": -100},
+        {"Justification": 0, "Spacing": 0, "Mashq": 10},
+        {"Justification": 90, "Spacing": 0, "Mashq": 10},
+        {"Justification": 100, "Spacing": 0, "Mashq": 10},
+        {"Justification": -50, "Spacing": 0, "Mashq": 10},
+        {"Justification": -100, "Spacing": 0, "Mashq": 10},
     ]

     outputs = [

But it feels redundant. Now I need to remember to update these if I ever change the default location of any of the axes.

khaledhosny · 2023-06-01T21:13:15Z

Also I’m seeing this ttx diff compared to my code:

   <avar>
     <version major="2" minor="0"/>
     <segment axis="SPAC">
+      <mapping from="-1.0" to="-1.0"/>
+      <mapping from="0.0" to="0.0"/>
+      <mapping from="1.0" to="1.0"/>
     </segment>
     <segment axis="MSHQ">
+      <mapping from="-1.0" to="-1.0"/>
+      <mapping from="0.0" to="0.0"/>
+      <mapping from="1.0" to="1.0"/>
     </segment>
     <segment axis="jstf">
+      <mapping from="-1.0" to="-1.0"/>
+      <mapping from="0.0" to="0.0"/>
+      <mapping from="1.0" to="1.0"/>
     </segment>

Probably harmless but also redundant.

behdad · 2023-06-01T21:27:41Z

But I don’t understand these warnings, what am I doing wrong?

Make the SPAC axis hidden and they should go away.

behdad · 2023-06-01T21:28:35Z

This fixes the warnings:

diff --git a/scripts/build.py b/scripts/build.py
index 8d19a4a..63d4531 100644
--- a/scripts/build.py
+++ b/scripts/build.py
@@ -726,11 +726,11 @@ def build(font, instance, args):
         ds.addSource(source)

     inputs = [
-        {"Justification": 0},
-        {"Justification": 90},
-        {"Justification": 100},
-        {"Justification": -50},
-        {"Justification": -100},
+        {"Justification": 0, "Spacing": 0, "Mashq": 10},
+        {"Justification": 90, "Spacing": 0, "Mashq": 10},
+        {"Justification": 100, "Spacing": 0, "Mashq": 10},
+        {"Justification": -50, "Spacing": 0, "Mashq": 10},
+        {"Justification": -100, "Spacing": 0, "Mashq": 10},
     ]

     outputs = [

But it feels redundant. Now I need to remember to update these if I ever change the default location of any of the axes.

Yeah it's one of the issues we've been discussing. Currently we warn if a non-hidden output axis is not specified.

khaledhosny · 2023-06-01T21:29:48Z

But I don’t understand these warnings, what am I doing wrong?

Make the SPAC axis hidden and they should go away.

But I want it the other way around, SPAC and MSHQ visible and jstf hidden since it should be used by justification algorithms and not directly by the user, unlike the other two.

behdad · 2023-06-01T21:43:28Z

But I don’t understand these warnings, what am I doing wrong?

Make the SPAC axis hidden and they should go away.

But I want it the other way around, SPAC and MSHQ visible and jstf hidden since it should be used by justification algorithms and not directly by the user, unlike the other two.

Interesting. Yeah I think we might want to nuke that warning then.

justvanrossum · 2023-06-02T06:02:28Z

Yes, let's get rid of the warning. But since leaving out the input value and putting it at the default value should behave the same way, I am also surprised at the ttx diff.

khaledhosny · 2023-06-02T07:38:51Z

But since leaving out the input value and putting it at the default value should behave the same way, I am also surprised at the ttx diff.

Sorry of I didn't make this clear, the ttx diff is not related to this issue but when generating avar with varLib in general (I’m comparing with the code I was using earlier which left the segments empty).

#3123 (comment)

behdad · 2023-06-02T18:52:01Z

Removed warning.

Is this good to merge then?

anthrotype · 2023-06-02T19:48:03Z

@behdad what about that diff that Khaled noticed?

behdad · 2023-06-02T20:02:07Z

@behdad what about that diff that Khaled noticed?

That's because of this pre-existing code block:

fonttools/Lib/fontTools/varLib/__init__.py

Lines 149 to 156 in 84cebca

    
           # Currently, some rasterizers require that the default value maps 
        
           # (-1 to -1, 0 to 0, and 1 to 1) be present for all the segment 
        
           # maps, even when the default normalization mapping for the axis 
        
           # was not modified. 
        
           # https://github.com/googlei18n/fontmake/issues/295 
        
           # https://github.com/fonttools/fonttools/issues/1011 
        
           # TODO(anthrotype) revert this (and 19c4b37) when issue is fixed 
        
           curve = avar.segments[axis.tag] = {-1.0: -1.0, 0.0: 0.0, 1.0: 1.0}

anthrotype · 2023-06-08T11:07:43Z

Are we all happy to merge this? We can start making avar2 fonts and come back fix things as needed

anthrotype · 2023-06-08T11:23:43Z

@behdad hm that commit you quoted is quite old, from 2017
04eacf1

I’m comparing with the code I was using earlier which left the segments empty

@khaledhosny does that mean you had not rebuilt your fonts since 2017?

khaledhosny · 2023-06-08T11:57:25Z

@behdad hm that commit you quoted is quite old, from 2017 04eacf1

I’m comparing with the code I was using earlier which left the segments empty

@khaledhosny does that mean you had not rebuilt your fonts since 2017?

I wasn’t using DS to generate avar table.

behdad marked this pull request as draft May 24, 2023 16:22

behdad force-pushed the avar2-designspace branch 4 times, most recently from 044054a to 522b22d Compare May 25, 2023 20:55

behdad force-pushed the avar2-designspace branch from 522b22d to c1a881b Compare May 25, 2023 21:40

madig reviewed May 26, 2023

View reviewed changes

Lib/fontTools/designspaceLib/__init__.py Outdated Show resolved Hide resolved

behdad force-pushed the avar2-designspace branch from 39f4c80 to cb4a2f4 Compare May 26, 2023 19:12

behdad changed the title ~~[designspace] Hack to read avar2 mapping~~ [designspace] Add avar2 mapping support May 26, 2023

behdad marked this pull request as ready for review May 28, 2023 20:00

behdad requested a review from justvanrossum May 28, 2023 20:00

anthrotype reviewed Jun 1, 2023

View reviewed changes

Tests/designspaceLib/designspace_test.py Show resolved Hide resolved

anthrotype reviewed Jun 1, 2023

View reviewed changes

Tests/designspaceLib/split_test.py Outdated Show resolved Hide resolved

behdad force-pushed the avar2-designspace branch from c4393bc to 176959c Compare June 1, 2023 17:26

[designspaceLib/avar2] Apply review comments

ddf16c9

behdad force-pushed the avar2-designspace branch from 176959c to ddf16c9 Compare June 1, 2023 17:54

behdad added 2 commits June 1, 2023 12:12

[designspaceLib/avar2] Remove Windows hack in tests

65bc6b7

[designspaceLib/avar2] Err if mappings axes ranges are being subset

54cfb5f

This comment was marked as outdated.

Sign in to view

[varLib/avar2] Remove warning

d601646

#3123 (comment)

khaledhosny approved these changes Jun 8, 2023

View reviewed changes

anthrotype merged commit b3ce097 into main Jun 8, 2023
10 checks passed

anthrotype deleted the avar2-designspace branch June 8, 2023 11:58

[designspace] Add avar2 mapping support #3123

[designspace] Add avar2 mapping support #3123

Conversation

behdad commented May 24, 2023 • edited

behdad commented May 24, 2023

behdad commented May 25, 2023

Lorp commented May 25, 2023 • edited

behdad commented May 25, 2023

Lorp commented May 25, 2023

behdad commented May 25, 2023

behdad commented May 25, 2023 • edited

Lorp commented May 25, 2023 • edited

behdad commented May 25, 2023

davelab6 commented May 25, 2023 • edited by behdad

behdad commented May 25, 2023

Lorp commented May 26, 2023

LettError commented May 26, 2023 • edited

behdad commented May 26, 2023

behdad commented May 26, 2023

behdad commented May 26, 2023

behdad commented May 26, 2023

Lorp commented May 27, 2023

behdad commented May 28, 2023

behdad commented Jun 1, 2023

behdad commented Jun 1, 2023

This comment was marked as outdated.

khaledhosny commented Jun 1, 2023

khaledhosny commented Jun 1, 2023

khaledhosny commented Jun 1, 2023

khaledhosny commented Jun 1, 2023

behdad commented Jun 1, 2023

behdad commented Jun 1, 2023

khaledhosny commented Jun 1, 2023 • edited

behdad commented Jun 1, 2023

justvanrossum commented Jun 2, 2023

khaledhosny commented Jun 2, 2023

behdad commented Jun 2, 2023

anthrotype commented Jun 2, 2023

behdad commented Jun 2, 2023

anthrotype commented Jun 8, 2023

anthrotype commented Jun 8, 2023

khaledhosny commented Jun 8, 2023

behdad commented May 24, 2023 •

edited

Lorp commented May 25, 2023 •

edited

behdad commented May 25, 2023 •

edited

Lorp commented May 25, 2023 •

edited

davelab6 commented May 25, 2023 •

edited by behdad

LettError commented May 26, 2023 •

edited

khaledhosny commented Jun 1, 2023 •

edited