From 699e1000e144e2c423396821d00547bd5a3f97ae Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Fri, 7 Jul 2023 16:09:03 +0100 Subject: [PATCH 1/6] GlyphPathBuilder: support contours starting with offcurves, or without oncurves Fixes https://github.com/googlefonts/fontc/issues/349 --- fontir/src/error.rs | 2 + fontir/src/ir.rs | 382 +++++++++++++++++++++++++++++++------- glyphs2fontir/src/toir.rs | 39 ++-- ufo2fontir/src/toir.rs | 10 +- 4 files changed, 331 insertions(+), 102 deletions(-) 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..1761d842 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,58 @@ 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 https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point-types + 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 https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point-types 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 from the last non-offcufve point to Point `p`, using 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 https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point-types 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 +1130,13 @@ 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 +1147,73 @@ impl GlyphPathBuilder { } self.offcurve.clear(); } else { - self.move_to(p)?; + self.begin_path(OnCurve::Cubic(p.into()))?; } Ok(()) } + /// Append off-curve point `p` to curve segment identified by the following on-curve + /// point, which can be either a (cubic) "curve" or (quadratic) "qcurve". + /// If this is the first point in a contour, the contour is assumed to be closed. + /// Cf. "offcurve" in https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point-types 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 +1226,7 @@ mod tests { use crate::{ coords::{CoordConverter, UserCoord}, + error::PathConversionError, ir::Axis, }; @@ -1182,46 +1265,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().to_svg()); + 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 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().to_svg()); + 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 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 +1359,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.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(); + 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.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.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.close_path().unwrap(); - assert_eq!("M2,2 C3,0 0,3 4,2 L2,2 Z", builder.build().to_svg()); + 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/glyphs2fontir/src/toir.rs b/glyphs2fontir/src/toir.rs index e7275962..1da5d37a 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.iter().collect(); // 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 @@ -95,11 +88,7 @@ fn to_ir_path(glyph_name: GlyphName, src_path: &Path) -> Result 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 Date: Mon, 7 Aug 2023 12:54:05 +0100 Subject: [PATCH 2/6] glyphs2fontir: support translating qcurves to IR paths --- glyphs-reader/src/font.rs | 6 ++ glyphs2fontir/src/source.rs | 17 ++++ glyphs2fontir/src/toir.rs | 3 + resources/testdata/glyphs2/QCurve.glyphs | 87 ++++++++++++++++ resources/testdata/glyphs3/QCurve.glyphs | 121 +++++++++++++++++++++++ 5 files changed, 234 insertions(+) create mode 100644 resources/testdata/glyphs2/QCurve.glyphs create mode 100644 resources/testdata/glyphs3/QCurve.glyphs 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 1da5d37a..ffe4b6e3 100644 --- a/glyphs2fontir/src/toir.rs +++ b/glyphs2fontir/src/toir.rs @@ -85,6 +85,9 @@ 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)?, } } 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; +} From bcdb65219b9b3dc410536ad0ea9d98c5e7ab9837 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Tue, 8 Aug 2023 17:38:29 +0100 Subject: [PATCH 3/6] docstrings: let first line be short summary --- fontir/src/ir.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/fontir/src/ir.rs b/fontir/src/ir.rs index 1761d842..fc6e2d3b 100644 --- a/fontir/src/ir.rs +++ b/fontir/src/ir.rs @@ -1070,6 +1070,7 @@ impl GlyphPathBuilder { } /// 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 https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point-types pub fn move_to(&mut self, p: impl Into) -> Result<(), PathConversionError> { @@ -1083,6 +1084,7 @@ impl GlyphPathBuilder { } /// 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. @@ -1097,8 +1099,9 @@ impl GlyphPathBuilder { Ok(()) } - /// Draws a quadratic curve from the last non-offcufve point to Point `p`, using the - /// TrueType "implied on-curve point" principle. + /// 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 @@ -1131,6 +1134,7 @@ impl GlyphPathBuilder { } /// 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. @@ -1152,9 +1156,11 @@ impl GlyphPathBuilder { Ok(()) } - /// Append off-curve point `p` to curve segment identified by the following on-curve - /// point, which can be either a (cubic) "curve" or (quadratic) "qcurve". - /// If this is the first point in a contour, the contour is assumed to be closed. + /// 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 https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point-types pub fn offcurve(&mut self, p: impl Into) -> Result<(), PathConversionError> { if self.first_oncurve.is_some() { @@ -1165,7 +1171,9 @@ impl GlyphPathBuilder { Ok(()) } - /// Ends the current sub-path. It's called automatically by `build()` thus can be + /// 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> { From 960d5b00b6ba6d14f5855ba8256cb9825842cec3 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Tue, 8 Aug 2023 17:40:22 +0100 Subject: [PATCH 4/6] wrap url in <> inside docstrings --- fontir/src/ir.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fontir/src/ir.rs b/fontir/src/ir.rs index fc6e2d3b..33a02862 100644 --- a/fontir/src/ir.rs +++ b/fontir/src/ir.rs @@ -1072,7 +1072,7 @@ impl GlyphPathBuilder { /// 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 https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point-types + /// Cf. "move" in pub fn move_to(&mut self, p: impl Into) -> Result<(), PathConversionError> { if !self.is_empty() { return Err(PathConversionError::MoveAfterFirstPoint { @@ -1088,7 +1088,7 @@ impl GlyphPathBuilder { /// 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 https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point-types + /// Cf. "line" in pub fn line_to(&mut self, p: impl Into) -> Result<(), PathConversionError> { self.check_num_offcurve(|v| v == 0)?; if self.first_oncurve.is_none() { @@ -1107,7 +1107,7 @@ impl GlyphPathBuilder { /// 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 https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point-types + /// 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 @@ -1161,7 +1161,7 @@ impl GlyphPathBuilder { /// 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 https://unifiedfontobject.org/versions/ufo3/glyphs/glif/#point-types + /// Cf. "offcurve" in pub fn offcurve(&mut self, p: impl Into) -> Result<(), PathConversionError> { if self.first_oncurve.is_some() { self.offcurve.push(p.into()); From 9f832525b0f6d0976dbcc3bf09d22d9f5dc809f4 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Tue, 8 Aug 2023 17:40:40 +0100 Subject: [PATCH 5/6] iter().collect() => .clone() for Vec --- glyphs2fontir/src/toir.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glyphs2fontir/src/toir.rs b/glyphs2fontir/src/toir.rs index ffe4b6e3..e356ef66 100644 --- a/glyphs2fontir/src/toir.rs +++ b/glyphs2fontir/src/toir.rs @@ -52,7 +52,7 @@ fn to_ir_path(glyph_name: GlyphName, src_path: &Path) -> Result = src_path.nodes.iter().collect(); + let mut nodes: Vec<_> = src_path.nodes.clone(); // First is a delicate butterfly if !src_path.closed { From 504d4a3638e55c6b5205a034eb4ad69a56e089b5 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Tue, 8 Aug 2023 17:42:51 +0100 Subject: [PATCH 6/6] wrap code with backticks to avoid it being confused with HTML tag --- fontir/src/ir.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fontir/src/ir.rs b/fontir/src/ir.rs index 33a02862..5553f9e3 100644 --- a/fontir/src/ir.rs +++ b/fontir/src/ir.rs @@ -1021,7 +1021,7 @@ impl OnCurve { /// 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 +/// our GlyphInstance.contours is defined as a `Vec`, so frontends should /// convert one contour at a time. #[derive(Debug)] pub struct GlyphPathBuilder {