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

contours of zero width can't be exported into a variable font #572

Closed
jansindl3r opened this issue Aug 19, 2019 · 15 comments · Fixed by googlefonts/cu2qu#185
Closed

contours of zero width can't be exported into a variable font #572

jansindl3r opened this issue Aug 19, 2019 · 15 comments · Fixed by googlefonts/cu2qu#185

Comments

@jansindl3r
Copy link

jansindl3r commented Aug 19, 2019

Hello, I have a master that has all points at the same x position. It cannot be exported as variable font with fontmake -o variable -m font.designspace. Source is in ufo. I am attaching demo that reproduces this. Can I turn off compatibility check and force the export? See letter A Thanks, Jan
bug.zip

--keep-direction and --keep-overlaps didn't help
CLI:
fontmake -o variable -m bug.designspace --verbose WARNING
Error:
WARNING:fontTools.varLib:glyph A has incompatible masters; skipping

@jansindl3r
Copy link
Author

jansindl3r commented Aug 23, 2019

What is uncompatible in this?

  <outline>
    <contour>
      <point type="line" x="72" y="651" />
      <point type="line" x="72" y="101" />
      <point type="line" x="476" y="101" />
      <point type="line" x="476" y="651" />
    </contour>
  </outline>

vs

  <outline>
    <contour>
      <point type="line" x="0" y="651" />
      <point type="line" x="0" y="101" />
      <point type="line" x="0" y="101" />
      <point type="line" x="0" y="651" />
    </contour>
  </outline>

change the second contour to this, makes it compatible... >>>

  <outline>
    <contour>
      <point type="line" x="1" y="651" />
      <point type="line" x="2" y="101" />
      <point type="line" x="3" y="101" />
      <point type="line" x="4" y="651" />
    </contour>
  </outline>

@madig
Copy link
Collaborator

madig commented Aug 23, 2019

Cursory look: Maybe the points

      <point type="line" x="0" y="101" />
      <point type="line" x="0" y="101" />

get merged into one point?

@jansindl3r
Copy link
Author

Most likely, I dont want them to get merged, what could be done, please?
I also believe that only overlapped starting point gets merged - will check tomorrow.

@jansindl3r
Copy link
Author

this is what Glyphs are able to export into a variable font

master 1
x:83.0 y:129.0 type:line
x:518.0 y:129.0 type:line
x:518.0 y:607.0 type:line
x:83.0 y:607.0 type:line
Master 2 (everything at the same position)
x:300.0 y:350.0 type:line
x:300.0 y:350.0 type:line
x:300.0 y:350.0 type:line
x:300.0 y:350.0 type:line

@jansindl3r
Copy link
Author

jansindl3r commented Aug 26, 2019

same problem with arg -o ttf-interpolatable ...overlapped startpoint gets merged

UFO:

     <point type="line" x="0" y="651" />
      <point type="line" x="0" y="101" />
      <point type="line" x="0" y="101" />
      <point type="line" x="0" y="651" />

opening this master in GLYPHS, after exporting 'compatible ttfs':

x:0.0 y:101.0 type:line
x:0.0 y:101.0 type:line
x:0.0 y:651.0 type:line

This works as expected though..

-o variable-cff2

@madig
Copy link
Collaborator

madig commented Aug 26, 2019

Hm. It werks if I do fontmake.exe -m bug.designspace -o ttf --keep-overlaps but fails with ttf-interpolatable.

@madig
Copy link
Collaborator

madig commented Aug 26, 2019

Tracking this down to cu2qu.ufo.fonts_to_quadratic...

TTFPreProcessor seems to use Cu2QuPointPen per individual glyph, whereas TTFInterpolatablePreProcessor uses cu2qu.ufo.fonts_to_quadratic on the same glyphs from all masters.

@madig
Copy link
Collaborator

madig commented Aug 26, 2019

This seems to be a feature (?) of fontTools' ReverseContourPen, which qu2cu uses for the conversion to change the winding order. Making the https://github.com/fonttools/fonttools/blob/63fb3fb881440b636267dc43082aab134e40f8f9/Lib/fontTools/pens/reverseContourPen.py#L64 unconditionally true and commenting out https://github.com/fonttools/fonttools/blob/63fb3fb881440b636267dc43082aab134e40f8f9/Lib/fontTools/pens/reverseContourPen.py#L79-L83 turns the input points

[
    (0, 651),
    (0, 101),
    (0, 101),
    (0, 651),
]

into

[
    (0, 651),
    (0, 651),
    (0, 101),
    (0, 101),
]

The associated cu2qu test case would be

    def test_overlapping_points(self):
        glyph = Glyph()
        pen = glyph.getPointPen()
        pen.beginPath()
        pen.addPoint((0, 651), segmentType="line")
        pen.addPoint((0, 101), segmentType="line")
        pen.addPoint((0, 101), segmentType="line")
        pen.addPoint((0, 651), segmentType="line")
        pen.endPath()
        assert glyph_to_quadratic(glyph, reverse_direction=True)
        assert [(p.x, p.y) for p in glyph[0]] == [
            (0, 651),
            (0, 651),  # is this reordering to be expected?
            (0, 101),
            (0, 101),
        ]

which passes with the modification mentioned above. Not sure what to do about this one. I'd expect the contour to not change? @anthrotype

@jansindl3r
Copy link
Author

@madig thanks for having look on that! I tried to track it, but I wasn't able to go that deep. The reordering is wrong as long as it happens per master, none of the apps out there is able to export my variable font correctly, only FontLab VI for Mac can do it. I don't know, probably they use their own version of FontTools. They have option to trun on and off these "smart features" that shift or remove points randomly

@jansindl3r
Copy link
Author

jansindl3r commented Sep 4, 2019

@madig I have just checked and FontLab VI exports it correctly and is using the same ReverseContourPen.py as you pointed to. It must be somewhere else, then?

anthrotype added a commit to anthrotype/fonttools that referenced this issue Sep 10, 2019
The PointToSegmentPen translates between PointPen and (Segment)Pen
protocol.

In the SegmentPen protocol, closed contours always imply a final 'lineTo'
segment from the last oncurve point to the starting point.
So the PointToSegmentPen omits the final 'lineTo' segment for closed
contours -- unless the option 'outputImpliedClosingLine' is True
(it is False by default, and defcon.Glyph.draw method initializes the
converter pen without this option).

However, if the last oncurve point is on a "line" segment and has same
coordinates as the starting point of a closed contour, the converter pen must
always output the closing 'lineTo' explicitly (regardless of the value of the
'outputImpliedClosingLine' option) in order to disambiguate this case from
the implied closing 'lineTo'.

If it doesn't do that, a duplicate 'line' point at the end of a closed
contour gets lost in the conversion.

See googlefonts/fontmake#572.
@anthrotype
Copy link
Member

@jansindl3r I fixed the issue with duplicate points being dropped in fonttools PointToSegmentPen. The fix is available in the latest fontmake v2.0.2 (which requires fonttools v4.0.1).
Please let us know if you find other issues, thanks.

@jansindl3r
Copy link
Author

jansindl3r commented Sep 11, 2019

@anthrotype Thanks, I have just tested it on my font

using fontmake of version 2.0.2 in virtual enviroment is raising this error.
ERROR:cu2qu.ufo:Glyphs named 'A' have different number of segments Some of the glyphs went though and are displayed correctly, so it is on good way! About half of the glyphs returned error, I can't really discover the pattern. I am using this arguments command fontmake -o variable --keep-overlaps -m bug.designspace --keep-direction also tried with the same result
I have attached source causing it, you find there one glyph that gets exported and one that gets not.

ERROR:cu2qu.ufo:Glyphs named 'A' have different number of segments

bug.zip

@anthrotype
Copy link
Member

@jansindl3r thanks for the files. I think I found the root of the issue. I will push a fix to cu2qu soon.
Nice rotation effect, btw 👍

anthrotype added a commit to anthrotype/cu2qu that referenced this issue Sep 13, 2019
When collecting a glyph's segments, we can't simply call the glyphs' draw
method with the GetSegmentsPen, but we must initialize the
PointToSegmentPen explicitly with outputImpliedClosingLine=True.

By default PointToSegmentPen does not outputImpliedClosingLine -- unless
last and first point on closed contour are duplicated.

Because we are converting multiple glyphs at the same time, we want to
make sure the _get_segments function returns the same number of segments,
whether or not the last and first point overlap.

Fixes googlefonts/fontmake#572

Also see: fonttools/fonttools#1720
@anthrotype
Copy link
Member

@jansindl3r I have released cu2qu 1.6.6 with the fix and verified that your test font builds successfully with latest fontmake and updated cu2qu.

@jansindl3r
Copy link
Author

@anthrotype thanks! It works in full version of the font as well.. I am very happy that I can start implementing fontmake into my workflow..
rotor_better_op

simoncozens pushed a commit to simoncozens/fonttools that referenced this issue Mar 10, 2020
The PointToSegmentPen translates between PointPen and (Segment)Pen
protocol.

In the SegmentPen protocol, closed contours always imply a final 'lineTo'
segment from the last oncurve point to the starting point.
So the PointToSegmentPen omits the final 'lineTo' segment for closed
contours -- unless the option 'outputImpliedClosingLine' is True
(it is False by default, and defcon.Glyph.draw method initializes the
converter pen without this option).

However, if the last oncurve point is on a "line" segment and has same
coordinates as the starting point of a closed contour, the converter pen must
always output the closing 'lineTo' explicitly (regardless of the value of the
'outputImpliedClosingLine' option) in order to disambiguate this case from
the implied closing 'lineTo'.

If it doesn't do that, a duplicate 'line' point at the end of a closed
contour gets lost in the conversion.

See googlefonts/fontmake#572.
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

Successfully merging a pull request may close this issue.

3 participants