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

Issue with skew attribute in component transform in Glyphs3-merge branch #1047

Open
roberto-arista opened this issue Nov 13, 2024 · 18 comments

Comments

@roberto-arista
Copy link

Consider the following code

import pprint

from fontTools.pens.recordingPen import RecordingPen
from glyphsLib import GSFont

if __name__ == "__main__":
    gs_font = GSFont("example.glyphs")
    layer = gs_font.glyphs["bitcoin"].layers[0]
    recording_pen = RecordingPen()
    layer.draw(recording_pen)
    pprint.pp(recording_pen.value)

When running the code from the main branch, the output is:

[('moveTo', ((249, 650),)),
 ('lineTo', ((297, 650),)),
 ('lineTo', ((324, 800),)),
 ('lineTo', ((276, 800),)),
 ('closePath', ()),
 ('moveTo', ((364, 650),)),
 ('lineTo', ((412, 650),)),
 ('lineTo', ((439, 800),)),
 ('lineTo', ((391, 800),)),
 ('closePath', ()),
 ('moveTo', ((115, -110),)),
 ('lineTo', ((163, -110),)),
 ('lineTo', ((191, 50),)),
 ('lineTo', ((143, 50),)),
 ('closePath', ()),
 ('moveTo', ((230, -110),)),
 ('lineTo', ((278, -110),)),
 ('lineTo', ((307, 50),)),
 ('lineTo', ((259, 50),)),
 ('closePath', ()),
 ('addComponent', ('B', <affine transformation 1 0 0.0 0.94 5 21>))]

Instead, when running the code from Glyphs3-merge, the output is:

[('moveTo', ((249, 650),)),
 ('lineTo', ((297, 650),)),
 ('lineTo', ((324, 800),)),
 ('lineTo', ((276, 800),)),
 ('closePath', ()),
 ('moveTo', ((364, 650),)),
 ('lineTo', ((412, 650),)),
 ('lineTo', ((439, 800),)),
 ('lineTo', ((391, 800),)),
 ('closePath', ()),
 ('moveTo', ((115, -110),)),
 ('lineTo', ((163, -110),)),
 ('lineTo', ((191, 50),)),
 ('lineTo', ((143, 50),)),
 ('closePath', ()),
 ('moveTo', ((230, -110),)),
 ('lineTo', ((278, -110),)),
 ('lineTo', ((307, 50),)),
 ('lineTo', ((259, 50),)),
 ('closePath', ()),
 ('addComponent',
  ('B', <affine transformation 1.0 0.0 -0.8422883804630793 0.94 5.0 21.0>))]

Attached you can find the .glyphs file.

example.glyphs.zip

cc @schriftgestalt

@schriftgestalt
Copy link
Collaborator

As fare as I can see, both transforms are not correct. In the .glyphs source is this:

{
pos = (5,21);
ref = B;
scale = (1,0.94);
slant = (-0.7,0);
}

transfrom from main:

1 0 0.0 0.94 5 21

and from Glyphs3 branch:

1 0 -0.8422883804630793 0.94 5 21

and from Glyphs 3.app

1 0 -0.011484837902484654 0.94 5 21

from what I can tell, the version from Glyphs.app seem to be correct. The calculation of the transform from scale, rotation and slant was done in a different order than in Glyphs.app (and the skew needed to be converted to radians). I fixed that. Thanks for the test case.

@roberto-arista
Copy link
Author

Hey Georg, thanks for the fix. I cannot see the code on remote, let me know when you'll push it (and on which branch) so I can test it on my end.

@schriftgestalt
Copy link
Collaborator

the type cast is still relevant. Remove the change with the active property.

@roberto-arista
Copy link
Author

done!

@roberto-arista
Copy link
Author

hey @schriftgestalt, could you push your fix online if it's ready? thanks!

@schriftgestalt
Copy link
Collaborator

my changes are all pushed

@roberto-arista
Copy link
Author

my changes are all pushed

thanks!

@anthrotype
Copy link
Member

If I understand this correctly the main branch still has the issue, as this was only fixed in Georg's Glyphs3-merge branch. I suggest we keep this open until main is also fixed.

from what I can tell, the version from Glyphs.app seem to be correct. The calculation of the transform from scale, rotation and slant was done in a different order than in Glyphs.app (and the skew needed to be converted to radians)

It's important that this order is well documented, remember that glyphsLib is not the only external implementation now, there's fontc as well. So far we have been trying to match glyphsLib's main branch.

@anthrotype
Copy link
Member

For reference, this commit on Glyphs3-branch changed the order in which skew is applied:

ccdf20b

before the combined transform order was:

translate * rotate * scale * skew

after that patch, it becomes like this:

translate * skew * sotate * scale

@anthrotype
Copy link
Member

hm glyphsLib main branch does not even handle the slant, I can't find a reference to it anywhere in glyphsLib/classes.py

Is this a relatively new thing in Glyphs, or was it never implemented in glyphsLib?

@schriftgestalt
Copy link
Collaborator

Glyphs 2 only had plain transform structs, so no special handling of slant needed. In Glyphs 3, I changed it to store the components separately as it makes it easier to show them in the UI (it very easy to get a transform from the components, but not the other way around). So I added support for rotate, scale and slant in the Glyphs3 branch.
a component in Glyphs 2:

{
name = A;
transform = "{0.8, 0.37927, -0.12343, 0.62414, 153, -74}";
}

and in Glyphs 3

{
angle = 20;
pos = (153,-74);
ref = A;
scale = (0.8,0.7);
slant = (10,8);
}

(if you have code that can decompose the above transform into the same values as shown in the G3 example (or at least something meaningful), I would be very grateful)

@anthrotype
Copy link
Member

I'm not sure.. The code we have right now can only decompose a 2x2 transformation matrix into scale and rotation components:

def transformStructToScaleAndRotation(transform):

By the way, why does the Python Scripting API only mentions position, scale and rotation attributes but not any "slant" or "skew"?

https://docu.glyphsapp.com/#gscomponent

@schriftgestalt
Copy link
Collaborator

By the way, why does the Python Scripting API only mentions position, scale and rotation attributes but not any "slant" or "skew"?

I just saw that and fixed it. I didn’t upload it, yet.

@anthrotype
Copy link
Member

anthrotype commented Nov 18, 2024

Ah I had forgotten we so have something to decompose an affine transform into its components:

https://github.com/fonttools/fonttools/blob/b90ac3c29f6030ec7309b044c41a72d9f586163f/Lib/fontTools/misc/transform.py#L410

Just Van Rossum added this to support the proposed VARC table

@schriftgestalt does this work for your use case?

@schriftgestalt
Copy link
Collaborator

print(DecomposedTransform.fromTransform((0.8, 0.37927, -0.12343, 0.62414, 153, -74)))

produces this (rounded a bit to make it more readable):

DecomposedTransform(translateX=153, translateY=-74, rotation=25.365, scaleX=0.885, scaleY=0.616, skewX=9.983, skewY=0.0, tCenterX=0, tCenterY=0)

Original:
angle = 20;
pos = (153,-74);
scale = (0.8,0.7);
slant = (10,8);

I think it is not even possible. A certain amount of skew is a rotation. And it gets very complicated with uneven scale. But my algorithm is not doing much better with this ;(

@anthrotype
Copy link
Member

the fonttools DecomposedTransform is derived from this
https://web.archive.org/web/20191221045335/https://frederic-wang.fr/decomposition-of-2d-transform-matrices.html

I believe the difference with yours may be down to the order of the operations?

In DecomposedTransform.toTransform() (which does the reverse and re-combines the components into the original matrix), ignoring for a moment the tCenterX and tCenterY (which we don't need here), notice how first does: skew, then scale, then rotate and finally translate (chaining operations works backwards, the last one is going to be applied first since the Transform.transform method multiplies matrices self @ other, which can be thought of mapping by other before applying self):

https://github.com/fonttools/fonttools/blob/b90ac3c29f6030ec7309b044c41a72d9f586163f/Lib/fontTools/misc/transform.py#L484-L500

Whereas in your case you're doing first scale, then rotate, then skew and finally translate.

@anthrotype
Copy link
Member

@schriftgestalt by the way, can you even insert a vertical skew (i.e. y-coordinates displaced vertically up/down proportionally to the x-coordinates) in the Glyphs.app UI?

I see an icon only for the horizontal skew (the parallelogram tilted to the right):

Image

@schriftgestalt
Copy link
Collaborator

The vertical skew can only be added by script. We’ll add it eventually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants