Merge pull request #385 from anthrotype/fix-leading-offcurves

GlyphPathBuilder: support contours starting with offcurves or without oncurves

fontir/src/error.rs

@@ -145,6 +145,8 @@ pub enum PathConversionError {
         num_offcurve: usize,
         points: Vec<Point>,
     },
+    #[error("{glyph_name} contour contains a 'move' that is not the first point: {point:?}")]
+    MoveAfterFirstPoint { glyph_name: GlyphName, point: Point },
 }
 
 #[derive(Debug, Error)]

glyphs-reader/src/font.rs

@@ -269,6 +269,8 @@ pub enum NodeType {
     OffCurve,
     Curve,
     CurveSmooth,
+    QCurve,
+    QCurveSmooth,
 }
 
 #[derive(Clone, Debug, FromPlist, PartialEq)]
@@ -440,12 +442,16 @@ impl std::str::FromStr for NodeType {
             "OFFCURVE" => Ok(NodeType::OffCurve),
             "CURVE" => Ok(NodeType::Curve),
             "CURVE SMOOTH" => Ok(NodeType::CurveSmooth),
+            "QCURVE" => Ok(NodeType::QCurve),
+            "QCURVE SMOOTH" => Ok(NodeType::QCurveSmooth),
             // Glyphs 3 style
             "l" => Ok(NodeType::Line),
             "ls" => Ok(NodeType::LineSmooth),
             "o" => Ok(NodeType::OffCurve),
             "c" => Ok(NodeType::Curve),
             "cs" => Ok(NodeType::CurveSmooth),
+            "q" => Ok(NodeType::QCurve),
+            "qs" => Ok(NodeType::QCurveSmooth),
             _ => Err(format!("unknown node type {s}")),
         }
     }

glyphs2fontir/src/source.rs

@@ -1348,4 +1348,21 @@ mod tests {
             )
         );
     }
+
+    #[test]
+    fn build_glyph_contour_ir_containing_qcurves() {
+        let glyph_name = "i";
+        let expected = "M302,584 Q328,584 346,602 Q364,620 364,645 Q364,670 346,687.5 Q328,705 302,705 Q276,705 257.5,687.5 Q239,670 239,645 Q239,620 257.5,602 Q276,584 302,584 Z";
+        for test_dir in &[glyphs2_dir(), glyphs3_dir()] {
+            let (source, context) = build_static_metadata(test_dir.join("QCurve.glyphs"));
+            build_glyphs(&source, &context, &[&glyph_name.into()]).unwrap();
+            let glyph = context.glyphs.get(&WorkId::Glyph(glyph_name.into()));
+            let default_instance = glyph
+                .sources()
+                .get(context.static_metadata.get().default_location())
+                .unwrap();
+            let first_contour = default_instance.contours.first().unwrap();
+            assert_eq!(first_contour.to_svg(), expected);
+        }
+    }
 }

glyphs2fontir/src/toir.rs

@@ -49,34 +49,27 @@ fn to_ir_path(glyph_name: GlyphName, src_path: &Path) -> Result<BezPath, WorkErr
     // See also https://github.com/fonttools/ufoLib2/blob/4d8a9600148b670b0840120658d9aab0b38a9465/src/ufoLib2/pointPens/glyphPointPen.py#L16
     let mut path_builder = GlyphPathBuilder::new(glyph_name.clone());
     if src_path.nodes.is_empty() {
-        return Ok(path_builder.build());
+        return Ok(path_builder.build()?);
     }
 
-    let mut nodes = &src_path.nodes[..];
+    let mut nodes: Vec<_> = src_path.nodes.clone();
 
     // First is a delicate butterfly
-    let first = if !src_path.closed {
-        let (first, elements) = nodes
-            .split_first()
-            .expect("Not empty and no first is a good trick");
-        nodes = elements;
-        first
+    if !src_path.closed {
+        let first = nodes.remove(0);
+        if first.node_type == NodeType::OffCurve {
+            return Err(WorkError::InvalidSourceGlyph {
+                glyph_name,
+                message: String::from("Open path starts with off-curve points"),
+            });
+        }
+        path_builder.move_to((first.pt.x, first.pt.y))?;
     } else {
         // In Glyphs.app, the starting node of a closed contour is always
         // stored at the end of the nodes list.
-        let (last, elements) = nodes
-            .split_last()
-            .expect("Not empty and no last is a good trick");
-        nodes = elements;
-        last
+        // Rotate so that it is at the beginning.
+        nodes.rotate_right(1);
     };
-    if first.node_type == NodeType::OffCurve {
-        return Err(WorkError::InvalidSourceGlyph {
-            glyph_name,
-            message: String::from("Open path starts with off-curve points"),
-        });
-    }
-    path_builder.move_to((first.pt.x, first.pt.y))?;
 
     // Walk through the remaining points, accumulating off-curve points until we see an on-curve
     // https://github.com/googlefonts/glyphsLib/blob/24b4d340e4c82948ba121dcfe563c1450a8e69c9/Lib/glyphsLib/pens.py#L92
@@ -92,14 +85,13 @@ fn to_ir_path(glyph_name: GlyphName, src_path: &Path) -> Result<BezPath, WorkErr
             NodeType::OffCurve => path_builder
                 .offcurve((node.pt.x, node.pt.y))
                 .map_err(WorkError::PathConversionError)?,
+            NodeType::QCurve | NodeType::QCurveSmooth => path_builder
+                .qcurve_to((node.pt.x, node.pt.y))
+                .map_err(WorkError::PathConversionError)?,
         }
     }
 
-    if src_path.closed {
-        path_builder.close_path()?;
-    }
-
-    let path = path_builder.build();
+    let path = path_builder.build()?;
     trace!(
         "Built a {} entry path for {}",
         path.elements().len(),

resources/testdata/glyphs2/QCurve.glyphs

@@ -0,0 +1,87 @@
+{
+.appVersion = "3208";
+DisplayStrings = (
+"",
+i
+);
+customParameters = (
+{
+name = "Disable Last Change";
+value = 1;
+}
+);
+date = "2023-07-20 13:49:39 +0000";
+familyName = "New Font";
+fontMaster = (
+{
+alignmentZones = (
+"{800, 16}",
+"{700, 16}",
+"{500, 16}",
+"{0, -16}",
+"{-200, -16}"
+);
+ascender = 800;
+capHeight = 700;
+descender = -200;
+id = m01;
+xHeight = 500;
+}
+);
+glyphs = (
+{
+glyphname = i;
+layers = (
+{
+layerId = m01;
+paths = (
+{
+closed = 1;
+nodes = (
+"328 584 OFFCURVE",
+"364 620 OFFCURVE",
+"364 645 QCURVE SMOOTH",
+"364 670 OFFCURVE",
+"328 705 OFFCURVE",
+"302 705 QCURVE SMOOTH",
+"276 705 OFFCURVE",
+"239 670 OFFCURVE",
+"239 645 QCURVE SMOOTH",
+"239 620 OFFCURVE",
+"276 584 OFFCURVE",
+"302 584 QCURVE SMOOTH"
+);
+},
+{
+closed = 1;
+nodes = (
+"241 0 LINE",
+"354 0 LINE",
+"354 500 LINE",
+"241 500 LINE"
+);
+}
+);
+width = 600;
+}
+);
+unicode = 0069;
+},
+{
+glyphname = space;
+layers = (
+{
+layerId = m01;
+width = 200;
+}
+);
+unicode = 0020;
+}
+);
+unitsPerEm = 1000;
+userData = {
+GSDontShowVersionAlert = 1;
+};
+versionMajor = 1;
+versionMinor = 0;
+}

resources/testdata/glyphs3/QCurve.glyphs

@@ -0,0 +1,121 @@
+{
+.appVersion = "3208";
+.formatVersion = 3;
+DisplayStrings = (
+"",
+i
+);
+customParameters = (
+{
+name = "Write lastChange";
+value = 0;
+}
+);
+date = "2023-07-20 13:49:39 +0000";
+familyName = "New Font";
+fontMaster = (
+{
+id = m01;
+metricValues = (
+{
+over = 16;
+pos = 800;
+},
+{
+over = 16;
+pos = 700;
+},
+{
+over = 16;
+pos = 500;
+},
+{
+over = -16;
+},
+{
+over = -16;
+pos = -200;
+},
+{
+}
+);
+name = Regular;
+}
+);
+glyphs = (
+{
+glyphname = i;
+layers = (
+{
+layerId = m01;
+shapes = (
+{
+closed = 1;
+nodes = (
+(328,584,o),
+(364,620,o),
+(364,645,qs),
+(364,670,o),
+(328,705,o),
+(302,705,qs),
+(276,705,o),
+(239,670,o),
+(239,645,qs),
+(239,620,o),
+(276,584,o),
+(302,584,qs)
+);
+},
+{
+closed = 1;
+nodes = (
+(241,0,l),
+(354,0,l),
+(354,500,l),
+(241,500,l)
+);
+}
+);
+width = 600;
+}
+);
+unicode = 105;
+},
+{
+glyphname = space;
+layers = (
+{
+layerId = m01;
+width = 200;
+}
+);
+unicode = 32;
+}
+);
+metrics = (
+{
+type = ascender;
+},
+{
+type = "cap height";
+},
+{
+type = "x-height";
+},
+{
+type = baseline;
+},
+{
+type = descender;
+},
+{
+type = "italic angle";
+}
+);
+unitsPerEm = 1000;
+userData = {
+GSDontShowVersionAlert = 1;
+};
+versionMajor = 1;
+versionMinor = 0;
+}

ufo2fontir/src/toir.rs

@@ -30,7 +30,7 @@ pub(crate) fn to_design_location(
 fn to_ir_contour(glyph_name: GlyphName, contour: &norad::Contour) -> Result<BezPath, WorkError> {
     let mut path_builder = GlyphPathBuilder::new(glyph_name.clone());
     if contour.points.is_empty() {
-        return Ok(path_builder.build());
+        return Ok(path_builder.build()?);
     }
 
     // Walk through the remaining points, accumulating off-curve points until we see an on-curve
@@ -44,13 +44,7 @@ fn to_ir_contour(glyph_name: GlyphName, contour: &norad::Contour) -> Result<BezP
         }
     }
 
-    // "A closed contour does not start with a move"
-    // https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point-types
-    if norad::PointType::Move != contour.points[0].typ {
-        path_builder.close_path()?;
-    }
-
-    let path = path_builder.build();
+    let path = path_builder.build()?;
     trace!(
         "Built a {} entry path for {}",
         path.elements().len(),