diff --git a/fontir/src/error.rs b/fontir/src/error.rs index 338a2dd5..a1a8839f 100644 --- a/fontir/src/error.rs +++ b/fontir/src/error.rs @@ -145,6 +145,8 @@ pub enum PathConversionError { num_offcurve: usize, points: Vec, }, + #[error("{glyph_name} contour contains a 'move' that is not the first point: {point:?}")] + MoveAfterFirstPoint { glyph_name: GlyphName, point: Point }, } #[derive(Debug, Error)] diff --git a/fontir/src/ir.rs b/fontir/src/ir.rs index 8f0fa9d2..5553f9e3 100644 --- a/fontir/src/ir.rs +++ b/fontir/src/ir.rs @@ -15,7 +15,7 @@ use font_types::NameId; use font_types::Tag; use fontdrasil::types::{GlyphName, GroupName}; use indexmap::IndexSet; -use kurbo::{Affine, BezPath, PathEl, Point}; +use kurbo::{Affine, BezPath, Point}; use log::warn; use ordered_float::OrderedFloat; use serde::{Deserialize, Serialize}; @@ -992,25 +992,55 @@ pub struct Component { pub transform: Affine, } +#[derive(Debug, Copy, Clone, PartialEq)] +enum OnCurve { + Move(Point), + Line(Point), + Quad(Point), + Cubic(Point), +} + +impl OnCurve { + fn point(&self) -> &Point { + match self { + OnCurve::Move(p) => p, + OnCurve::Line(p) => p, + OnCurve::Quad(p) => p, + OnCurve::Cubic(p) => p, + } + } + + fn is_move(&self) -> bool { + matches!(self, OnCurve::Move(_)) + } +} + /// Helps convert points-of-type to a bezier path. /// /// Source formats tend to use streams of point-of-type. Curve manipulation is /// often easier on bezier path, so provide a mechanism to convert. +/// While kurbo::BezPath can contain multiple subpaths, and this builder could be +/// used to convert multiple contours (i.e. list of points) into a single BezPath, +/// our GlyphInstance.contours is defined as a `Vec`, so frontends should +/// convert one contour at a time. #[derive(Debug)] pub struct GlyphPathBuilder { glyph_name: GlyphName, offcurve: Vec, + leading_offcurve: Vec, path: BezPath, - last_move_to: Option, + first_oncurve: Option, } impl GlyphPathBuilder { + /// Create a new GlyphPathBuilder for a glyph with the given name. pub fn new(glyph_name: GlyphName) -> GlyphPathBuilder { GlyphPathBuilder { glyph_name, offcurve: Vec::new(), + leading_offcurve: Vec::new(), path: BezPath::new(), - last_move_to: None, + first_oncurve: None, } } @@ -1028,33 +1058,61 @@ impl GlyphPathBuilder { Ok(()) } - pub fn is_empty(&self) -> bool { - self.offcurve.is_empty() && self.path.elements().is_empty() + fn is_empty(&self) -> bool { + self.first_oncurve.is_none() && self.leading_offcurve.is_empty() } - pub fn move_to(&mut self, p: impl Into) -> Result<(), PathConversionError> { - self.check_num_offcurve(|v| v == 0)?; - let p = p.into(); - self.path.move_to(p); - self.last_move_to = Some(p); + fn begin_path(&mut self, oncurve: OnCurve) -> Result<(), PathConversionError> { + assert!(self.first_oncurve.is_none()); + self.path.move_to(*oncurve.point()); + self.first_oncurve = Some(oncurve); Ok(()) } + /// Lifts the "pen" to Point `p` and marks the beginning of an open contour. + /// + /// A point of this type can only be the first point in a contour. + /// Cf. "move" in + pub fn move_to(&mut self, p: impl Into) -> Result<(), PathConversionError> { + if !self.is_empty() { + return Err(PathConversionError::MoveAfterFirstPoint { + glyph_name: self.glyph_name.clone(), + point: p.into(), + }); + } + self.begin_path(OnCurve::Move(p.into())) + } + + /// Draws a line from the previous point to Point `p`. + /// + /// The previous point cannot be an off-curve point. + /// If this is the first point in a contour, the contour is assumed to be closed, + /// i.e. a cyclic list of points with no predominant start point. + /// Cf. "line" in pub fn line_to(&mut self, p: impl Into) -> Result<(), PathConversionError> { self.check_num_offcurve(|v| v == 0)?; - if self.is_empty() { - self.move_to(p)?; + if self.first_oncurve.is_none() { + self.begin_path(OnCurve::Line(p.into()))?; } else { self.path.line_to(p); } Ok(()) } + /// Draws a quadratic curve/spline from the last non-offcurve point to Point `p`. + /// + /// This uses the TrueType "implied on-curve point" principle. + /// The number of preceding off-curve points can be n >= 0. When n=0, a straight line is + /// implied. If n=1, a single quadratic Bezier curve is drawn. If n>=2, a sequence of + /// quadratic Bezier curves is drawn, with the implied on-curve points at the midpoints + /// between pairs of successive off-curve points. + /// If this is the first point in a contour, the contour is assumed to be closed. + /// Cf. "qcurve" in pub fn qcurve_to(&mut self, p: impl Into) -> Result<(), PathConversionError> { // https://github.com/googlefonts/fontmake-rs/issues/110 // Guard clauses: degenerate cases - if self.is_empty() { - return self.move_to(p); + if self.first_oncurve.is_none() { + return self.begin_path(OnCurve::Quad(p.into())); } if self.offcurve.is_empty() { return self.line_to(p); @@ -1075,11 +1133,14 @@ impl GlyphPathBuilder { Ok(()) } - /// Type of curve depends on accumulated off-curves + /// Draws a cubic curve from the previous non-offcurve point to Point `p`. /// - /// + /// Type of curve depends on the number of accumulated off-curves: 0 (straight line), + /// 1 (quadratic Bezier) or 2 (cubic Bezier). + /// If this is the first point in a contour, the contour is assumed to be closed. + /// Cf. "curve" in pub fn curve_to(&mut self, p: impl Into) -> Result<(), PathConversionError> { - if !self.is_empty() { + if self.first_oncurve.is_some() { match self.offcurve.len() { 0 => self.path.line_to(p), 1 => self.path.quad_to(self.offcurve[0], p.into()), @@ -1090,48 +1151,77 @@ impl GlyphPathBuilder { } self.offcurve.clear(); } else { - self.move_to(p)?; + self.begin_path(OnCurve::Cubic(p.into()))?; } Ok(()) } + /// Append off-curve point `p` to the following curve segment. + /// + /// The type of curve is defined by following on-curve point, which can be either a + /// (cubic) "curve" or (quadratic) "qcurve". + /// If offcurve is the first point in a contour, the contour is assumed to be closed. + /// Cf. "offcurve" in pub fn offcurve(&mut self, p: impl Into) -> Result<(), PathConversionError> { - self.offcurve.push(p.into()); + if self.first_oncurve.is_some() { + self.offcurve.push(p.into()); + } else { + self.leading_offcurve.push(p.into()); + } Ok(()) } - pub fn close_path(&mut self) -> Result<(), PathConversionError> { - // Take dangling off-curves to imply a curve back to sub-path start - if let Some(last_move) = self.last_move_to { - if !self.offcurve.is_empty() { - self.curve_to(last_move)?; - } - // explicitly output the implied closing line - // equivalent to fontTools' PointToSegmentPen(outputImpliedClosingLine=True) - match self.path.elements().last() { - // The last point of the closed contour is actually of type "line" - // Explicitly emit it so it doesn't get lost if we turn back into a pointstream - Some(PathEl::LineTo(_)) => { - self.path.line_to(last_move); - } + /// Ends the current sub-path. + /// + /// It's called automatically by `build()` thus can be + /// omitted when building one BezPath per contour, but can be called manually in + /// order to build multiple contours into a single BezPath. + pub fn end_path(&mut self) -> Result<(), PathConversionError> { + // a contour that does *not* start with a move is assumed to be closed + // https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point-types + if !self.first_oncurve.is_some_and(|on| on.is_move()) { + self.close_path()?; + } - // The source ends in something that isn't a line, and it's closed. - // Add an explicit line if it wouldn't be zero-length as this is how - // a closed point-stream is interpreted. - Some(PathEl::QuadTo(_, last_pt)) | Some(PathEl::CurveTo(_, _, last_pt)) => { - if *last_pt != last_move { - self.path.line_to(last_move) - } - } - _ => (), + self.check_num_offcurve(|v| v == 0)?; + self.first_oncurve = None; + Ok(()) + } + + fn close_path(&mut self) -> Result<(), PathConversionError> { + // Flush any leading off-curves to the end. This matches fontTools' PointToSegmentPen + // always starting/ending a closed contour on the first on-curve point: + // https://github.com/fonttools/fonttools/blob/57fb47/Lib/fontTools/pens/pointPen.py#L147-L155 + if !self.leading_offcurve.is_empty() { + self.offcurve.append(&mut self.leading_offcurve); + } + // Take dangling off-curves to imply a curve back to sub-path start. + // For closed paths we explicitly output the implied closing line + // equivalent to fontTools' PointToSegmentPen(outputImpliedClosingLine=True) + if let Some(first_oncurve) = self.first_oncurve { + match first_oncurve { + OnCurve::Line(pt) => self.line_to(pt)?, + OnCurve::Quad(pt) => self.qcurve_to(pt)?, + OnCurve::Cubic(pt) => self.curve_to(pt)?, + _ => unreachable!(), } self.path.close_path(); + } else if !self.offcurve.is_empty() { + // special TrueType oncurve-less quadratic contour, we assume the path + // starts at midpoint between the first and last offcurves + let first_offcurve = self.offcurve[0]; + let last_offcurve = *self.offcurve.last().unwrap(); + let implied_oncurve = first_offcurve.midpoint(last_offcurve); + self.begin_path(OnCurve::Quad(implied_oncurve))?; + self.close_path()?; } Ok(()) } - pub fn build(self) -> BezPath { - self.path + /// Builds the kurbo::BezPath from the accumulated points. + pub fn build(mut self) -> Result { + self.end_path()?; + Ok(self.path) } } @@ -1144,6 +1234,7 @@ mod tests { use crate::{ coords::{CoordConverter, UserCoord}, + error::PathConversionError, ir::Axis, }; @@ -1182,46 +1273,93 @@ mod tests { #[test] fn a_qcurve_with_no_offcurve_is_a_line() { let mut builder = GlyphPathBuilder::new("test".into()); - builder.move_to((2.0, 2.0)).unwrap(); + builder.move_to((2.0, 2.0)).unwrap(); // open contour + builder.qcurve_to((4.0, 2.0)).unwrap(); + assert_eq!("M2,2 L4,2", builder.build().unwrap().to_svg()); + + let mut builder = GlyphPathBuilder::new("test".into()); + builder.qcurve_to((2.0, 2.0)).unwrap(); // closed, ie not starting with 'move' builder.qcurve_to((4.0, 2.0)).unwrap(); - assert_eq!("M2,2 L4,2", builder.build().to_svg()); + assert_eq!("M2,2 L4,2 L2,2 Z", builder.build().unwrap().to_svg()); + } + + #[test] + fn a_curve_with_no_offcurve_is_a_line() { + let mut builder = GlyphPathBuilder::new("test".into()); + builder.move_to((2.0, 2.0)).unwrap(); // open + builder.curve_to((4.0, 2.0)).unwrap(); + assert_eq!("M2,2 L4,2", builder.build().unwrap().to_svg()); + + let mut builder = GlyphPathBuilder::new("test".into()); + builder.curve_to((2.0, 2.0)).unwrap(); // closed + builder.curve_to((4.0, 2.0)).unwrap(); + assert_eq!("M2,2 L4,2 L2,2 Z", builder.build().unwrap().to_svg()); + } + + #[test] + fn a_curve_with_one_offcurve_is_a_single_quad() { + let mut builder = GlyphPathBuilder::new("test".into()); + builder.move_to((2.0, 2.0)).unwrap(); // open + builder.offcurve((3.0, 0.0)).unwrap(); + builder.curve_to((4.0, 2.0)).unwrap(); + assert_eq!("M2,2 Q3,0 4,2", builder.build().unwrap().to_svg()); + + let mut builder = GlyphPathBuilder::new("test".into()); + builder.curve_to((2.0, 2.0)).unwrap(); // closed + builder.offcurve((3.0, 0.0)).unwrap(); + builder.curve_to((4.0, 2.0)).unwrap(); + assert_eq!("M2,2 Q3,0 4,2 L2,2 Z", builder.build().unwrap().to_svg()); } #[test] fn a_qcurve_with_one_offcurve_is_a_single_quad_to() { let mut builder = GlyphPathBuilder::new("test".into()); - builder.move_to((2.0, 2.0)).unwrap(); + builder.move_to((2.0, 2.0)).unwrap(); // open + builder.offcurve((3.0, 0.0)).unwrap(); + builder.qcurve_to((4.0, 2.0)).unwrap(); + assert_eq!("M2,2 Q3,0 4,2", builder.build().unwrap().to_svg()); + + let mut builder = GlyphPathBuilder::new("test".into()); + builder.qcurve_to((2.0, 2.0)).unwrap(); // closed builder.offcurve((3.0, 0.0)).unwrap(); builder.qcurve_to((4.0, 2.0)).unwrap(); - assert_eq!("M2,2 Q3,0 4,2", builder.build().to_svg()); + assert_eq!("M2,2 Q3,0 4,2 L2,2 Z", builder.build().unwrap().to_svg()); } #[test] fn a_qcurve_with_two_offcurve_is_two_quad_to() { let mut builder = GlyphPathBuilder::new("test".into()); - builder.move_to((2.0, 2.0)).unwrap(); + builder.move_to((2.0, 2.0)).unwrap(); // open + builder.offcurve((3.0, 0.0)).unwrap(); + builder.offcurve((5.0, 4.0)).unwrap(); + builder.qcurve_to((6.0, 2.0)).unwrap(); + assert_eq!("M2,2 Q3,0 4,2 Q5,4 6,2", builder.build().unwrap().to_svg()); + + let mut builder = GlyphPathBuilder::new("test".into()); + builder.qcurve_to((2.0, 2.0)).unwrap(); // closed builder.offcurve((3.0, 0.0)).unwrap(); builder.offcurve((5.0, 4.0)).unwrap(); builder.qcurve_to((6.0, 2.0)).unwrap(); - assert_eq!("M2,2 Q3,0 4,2 Q5,4 6,2", builder.build().to_svg()); + assert_eq!( + "M2,2 Q3,0 4,2 Q5,4 6,2 L2,2 Z", + builder.build().unwrap().to_svg() + ); } #[test] fn last_line_always_emit_implied_closing_line() { let mut builder = GlyphPathBuilder::new("test".into()); - builder.move_to((2.0, 2.0)).unwrap(); + builder.line_to((2.0, 2.0)).unwrap(); builder.line_to((4.0, 2.0)).unwrap(); - builder.close_path().unwrap(); // a closing line is implied by Z, but emit it nonetheless - assert_eq!("M2,2 L4,2 L2,2 Z", builder.build().to_svg()); + assert_eq!("M2,2 L4,2 L2,2 Z", builder.build().unwrap().to_svg()); let mut builder = GlyphPathBuilder::new("test".into()); - builder.move_to((2.0, 2.0)).unwrap(); + builder.line_to((2.0, 2.0)).unwrap(); builder.line_to((4.0, 2.0)).unwrap(); // duplicate last point, not to be confused with the closing line implied by Z builder.line_to((2.0, 2.0)).unwrap(); - builder.close_path().unwrap(); - assert_eq!("M2,2 L4,2 L2,2 L2,2 Z", builder.build().to_svg()); + assert_eq!("M2,2 L4,2 L2,2 L2,2 Z", builder.build().unwrap().to_svg()); } #[test] @@ -1229,37 +1367,151 @@ mod tests { // if last curve point is equal to move, there's no need to disambiguate it from // the implicit closing line, so we don't emit one let mut builder = GlyphPathBuilder::new("test".into()); - builder.move_to((2.0, 2.0)).unwrap(); builder.offcurve((3.0, 0.0)).unwrap(); builder.qcurve_to((2.0, 2.0)).unwrap(); - builder.close_path().unwrap(); - assert_eq!("M2,2 Q3,0 2,2 Z", builder.build().to_svg()); + assert_eq!("M2,2 Q3,0 2,2 Z", builder.build().unwrap().to_svg()); let mut builder = GlyphPathBuilder::new("test".into()); - builder.move_to((2.0, 2.0)).unwrap(); builder.offcurve((3.0, 0.0)).unwrap(); builder.offcurve((0.0, 3.0)).unwrap(); builder.curve_to((2.0, 2.0)).unwrap(); - builder.close_path().unwrap(); - assert_eq!("M2,2 C3,0 0,3 2,2 Z", builder.build().to_svg()); + assert_eq!("M2,2 C3,0 0,3 2,2 Z", builder.build().unwrap().to_svg()); } #[test] fn last_curve_not_equal_move_do_emit_closing_line() { // if last point is different from move, then emit the implied closing line let mut builder = GlyphPathBuilder::new("test".into()); - builder.move_to((2.0, 2.0)).unwrap(); + builder.line_to((2.0, 2.0)).unwrap(); builder.offcurve((3.0, 0.0)).unwrap(); builder.qcurve_to((4.0, 2.0)).unwrap(); - builder.close_path().unwrap(); - assert_eq!("M2,2 Q3,0 4,2 L2,2 Z", builder.build().to_svg()); + assert_eq!("M2,2 Q3,0 4,2 L2,2 Z", builder.build().unwrap().to_svg()); let mut builder = GlyphPathBuilder::new("test".into()); - builder.move_to((2.0, 2.0)).unwrap(); + builder.line_to((2.0, 2.0)).unwrap(); builder.offcurve((3.0, 0.0)).unwrap(); builder.offcurve((0.0, 3.0)).unwrap(); builder.curve_to((4.0, 2.0)).unwrap(); - builder.close_path().unwrap(); - assert_eq!("M2,2 C3,0 0,3 4,2 L2,2 Z", builder.build().to_svg()); + assert_eq!( + "M2,2 C3,0 0,3 4,2 L2,2 Z", + builder.build().unwrap().to_svg() + ); + } + + #[test] + fn start_on_first_oncurve_irrespective_of_offcurves() { + // the following three closed contours are all equivalent and get normalized + // to the same path, which begins/ends on the first on-curve point i.e. (2,2). + let expected = "M2,2 C6,0 0,6 4,2 C3,0 0,3 2,2 Z"; + + let mut builder = GlyphPathBuilder::new("test".into()); + builder.offcurve((3.0, 0.0)).unwrap(); + builder.offcurve((0.0, 3.0)).unwrap(); + builder.curve_to((2.0, 2.0)).unwrap(); + builder.offcurve((6.0, 0.0)).unwrap(); + builder.offcurve((0.0, 6.0)).unwrap(); + builder.curve_to((4.0, 2.0)).unwrap(); + assert_eq!(expected, builder.build().unwrap().to_svg()); + + let mut builder = GlyphPathBuilder::new("test".into()); + builder.offcurve((0.0, 3.0)).unwrap(); + builder.curve_to((2.0, 2.0)).unwrap(); + builder.offcurve((6.0, 0.0)).unwrap(); + builder.offcurve((0.0, 6.0)).unwrap(); + builder.curve_to((4.0, 2.0)).unwrap(); + builder.offcurve((3.0, 0.0)).unwrap(); + assert_eq!(expected, builder.build().unwrap().to_svg()); + + let mut builder = GlyphPathBuilder::new("test".into()); + builder.curve_to((2.0, 2.0)).unwrap(); + builder.offcurve((6.0, 0.0)).unwrap(); + builder.offcurve((0.0, 6.0)).unwrap(); + builder.curve_to((4.0, 2.0)).unwrap(); + builder.offcurve((3.0, 0.0)).unwrap(); + builder.offcurve((0.0, 3.0)).unwrap(); + assert_eq!(expected, builder.build().unwrap().to_svg()); + } + + #[test] + fn closed_quadratic_contour_without_oncurve_points() { + let mut builder = GlyphPathBuilder::new("test".into()); + // builder.qcurve_to((0.0, 1.0)).unwrap(); // implied + builder.offcurve((1.0, 1.0)).unwrap(); + builder.offcurve((1.0, -1.0)).unwrap(); + builder.offcurve((-1.0, -1.0)).unwrap(); + builder.offcurve((-1.0, 1.0)).unwrap(); + assert_eq!( + "M0,1 Q1,1 1,0 Q1,-1 0,-1 Q-1,-1 -1,0 Q-1,1 0,1 Z", + builder.build().unwrap().to_svg() + ); + } + + #[test] + fn invalid_move_after_first_point() { + // A point of type 'move' must be the first point in an (open) contour. + let mut builder = GlyphPathBuilder::new("test".into()); + builder.move_to((2.0, 2.0)).unwrap(); + builder.end_path().unwrap(); + // move_to after ending the current subpath is OK + builder.move_to((3.0, 3.0)).unwrap(); + // but it's an error if we try to do move_to again + let result = builder.move_to((4.0, 4.0)); + + assert!(result.is_err()); + let Err(PathConversionError::MoveAfterFirstPoint { glyph_name, point }) = result else { + panic!("unexpected error: {:?}", result); + }; + assert_eq!("test", glyph_name.as_str()); + assert_eq!((4.0, 4.0), (point.x, point.y)); + + builder.end_path().unwrap(); + builder.line_to((5.0, 5.0)).unwrap(); + // can't move_to in the middle of a closed (not starting with move_to) subpath + let result = builder.move_to((6.0, 6.0)); + + assert!(result.is_err()); + let Err(PathConversionError::MoveAfterFirstPoint { glyph_name, point }) = result else { + panic!("unexpected error: {:?}", result); + }; + assert_eq!("test", glyph_name.as_str()); + assert_eq!((6.0, 6.0), (point.x, point.y)); + + builder.end_path().unwrap(); + builder.offcurve((7.0, 7.0)).unwrap(); + // can't move_to after an offcurve point + let result = builder.move_to((8.0, 8.0)); + + assert!(result.is_err()); + let Err(PathConversionError::MoveAfterFirstPoint { glyph_name, point }) = result else { + panic!("unexpected error: {:?}", result); + }; + assert_eq!("test", glyph_name.as_str()); + assert_eq!((8.0, 8.0), (point.x, point.y)); + } + + #[test] + fn closed_path_with_trailing_cubic_offcurves() { + let mut builder = GlyphPathBuilder::new("test".into()); + builder.curve_to((10.0, 0.0)).unwrap(); + builder.line_to((0.0, 10.0)).unwrap(); + builder.offcurve((5.0, 10.0)).unwrap(); + builder.offcurve((10.0, 5.0)).unwrap(); + + let path = builder.build().unwrap(); + + assert_eq!("M10,0 L0,10 C5,10 10,5 10,0 Z", path.to_svg()); + } + + #[test] + fn closed_path_with_trailing_quadratic_offcurves() { + let mut builder = GlyphPathBuilder::new("test".into()); + builder.qcurve_to((10.0, 0.0)).unwrap(); + builder.line_to((0.0, 10.0)).unwrap(); + builder.offcurve((5.0, 10.0)).unwrap(); + builder.offcurve((10.0, 5.0)).unwrap(); + + let path = builder.build().unwrap(); + + assert_eq!("M10,0 L0,10 Q5,10 7.5,7.5 Q10,5 10,0 Z", path.to_svg()); } } diff --git a/glyphs-reader/src/font.rs b/glyphs-reader/src/font.rs index 2878420b..04ca7bff 100644 --- a/glyphs-reader/src/font.rs +++ b/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}")), } } diff --git a/glyphs2fontir/src/source.rs b/glyphs2fontir/src/source.rs index b31d60a2..aa96069c 100644 --- a/glyphs2fontir/src/source.rs +++ b/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); + } + } } diff --git a/glyphs2fontir/src/toir.rs b/glyphs2fontir/src/toir.rs index e7275962..e356ef66 100644 --- a/glyphs2fontir/src/toir.rs +++ b/glyphs2fontir/src/toir.rs @@ -49,34 +49,27 @@ fn to_ir_path(glyph_name: GlyphName, src_path: &Path) -> Result = 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 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(), diff --git a/resources/testdata/glyphs2/QCurve.glyphs b/resources/testdata/glyphs2/QCurve.glyphs new file mode 100644 index 00000000..eccb67f3 --- /dev/null +++ b/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; +} diff --git a/resources/testdata/glyphs3/QCurve.glyphs b/resources/testdata/glyphs3/QCurve.glyphs new file mode 100644 index 00000000..b4116a9b --- /dev/null +++ b/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; +} diff --git a/ufo2fontir/src/toir.rs b/ufo2fontir/src/toir.rs index 345dc25f..a3fc1652 100644 --- a/ufo2fontir/src/toir.rs +++ b/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 { 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