Merge pull request #475 from googlefonts/reverse-contours

Reverse TrueType glyph contour direction by default to match fontmake

fontbe/src/glyphs.rs

@@ -15,7 +15,7 @@ use fontdrasil::{
 use fontir::{
     coords::{Location, NormalizedCoord, NormalizedLocation},
     ir,
-    orchestration::WorkId as FeWorkId,
+    orchestration::{Flags, WorkId as FeWorkId},
     variations::{VariationModel, VariationRegion},
 };
 use kurbo::{cubics_to_quadratic_splines, Affine, BezPath, CubicBez, PathEl, Point, Rect, Vec2};
@@ -346,7 +346,12 @@ impl Work<Context, AnyWorkId, Error> for GlyphWork {
         let glyph = CheckedGlyph::new(ir_glyph)?;
 
         // Hopefully in time https://github.com/harfbuzz/boring-expansion-spec means we can drop this
-        let glyph = cubics_to_quadratics(glyph);
+        let mut glyph = cubics_to_quadratics(glyph);
+
+        if !context.flags.contains(Flags::KEEP_DIRECTION) {
+            glyph.reverse_contour_direction();
+        }
+
         let should_iup = glyph.should_iup(); // we partially borrow it later
 
         let (name, point_seqs, contour_ends) = match glyph {
@@ -686,6 +691,21 @@ impl CheckedGlyph {
             CheckedGlyph::Contour { .. } => true,
         }
     }
+
+    /// Flip the glyph contours' direction, or do nothing if the glyph is a composite.
+    ///
+    /// The source contours are normally drawn with cubic curves thus are expected to be
+    /// in counter-clockwise winding direction as recommended for PostScript outlines.
+    /// When converting to TrueType quadratic splines, we reverse them so that they
+    /// follow the clockwise direction as recommeded for TrueType outlines.
+    fn reverse_contour_direction(&mut self) {
+        if let CheckedGlyph::Contour { name, paths } = self {
+            trace!("Reverse '{name}' contour direction");
+            for contour in paths.values_mut() {
+                *contour = contour.reverse_subpaths();
+            }
+        }
+    }
 }
 
 fn path_el_type(el: &PathEl) -> &'static str {

fontc/src/args.rs

@@ -53,6 +53,16 @@ pub struct Args {
     /// Set to skip compilation of OpenType Layout features
     #[arg(long, default_value = "false")]
     pub skip_features: bool,
+
+    /// Whether to keep the original glyph contour direction (TTF only).
+    ///
+    /// TrueType contours are recommended to follow clockwise orientation;
+    /// By default, we assume the source were drawn in counter-clockwise direction
+    /// as in PostScript outlines, and we flip the direction.
+    // Named to match fontmake's homonymous flag:
+    // https://github.com/googlefonts/fontmake/blob/6a8b2907/Lib/fontmake/__main__.py#L443
+    #[arg(long, default_value = "false")]
+    pub keep_direction: bool,
 }
 
 impl Args {
@@ -69,6 +79,7 @@ impl Args {
             self.decompose_transformed_components,
         );
         flags.set(Flags::EMIT_TIMING, self.emit_timing);
+        flags.set(Flags::KEEP_DIRECTION, self.keep_direction);
 
         flags
     }
@@ -92,6 +103,7 @@ impl Args {
             decompose_transformed_components: Flags::default()
                 .contains(Flags::DECOMPOSE_TRANSFORMED_COMPONENTS),
             skip_features: false,
+            keep_direction: false,
         }
     }
 }

fontc/src/lib.rs

@@ -817,6 +817,11 @@ mod tests {
         buf
     }
 
+    fn read_ir_glyph(build_dir: &Path, name: &str) -> ir::Glyph {
+        let raw_glyph = read_file(&build_dir.join(format!("glyph_ir/{name}.yml")));
+        ir::Glyph::read(&mut raw_glyph.as_slice())
+    }
+
     fn read_be_glyph(build_dir: &Path, name: &str) -> RawGlyph {
         let raw_glyph = read_file(&build_dir.join(format!("glyphs/{name}.glyf")));
         let read: &mut dyn Read = &mut raw_glyph.as_slice();
@@ -930,10 +935,10 @@ mod tests {
         assert_eq!(
             vec![
                 CurvePoint::on_curve(270, -9),
-                CurvePoint::off_curve(352, -9),
-                CurvePoint::off_curve(448, 56),
-                CurvePoint::off_curve(491, 174),
-                CurvePoint::on_curve(491, 253),
+                CurvePoint::off_curve(188, -9),
+                CurvePoint::off_curve(90, 55),
+                CurvePoint::off_curve(48, 174),
+                CurvePoint::on_curve(48, 254),
             ],
             glyph.points().take(5).collect::<Vec<_>>()
         );
@@ -1956,4 +1961,65 @@ mod tests {
         );
         assert_no_compile_features("static.designspace", &[b"GPOS", b"GSUB"]);
     }
+
+    #[test]
+    fn compile_simple_glyph_keep_direction() {
+        let temp_dir = tempdir().unwrap();
+        let build_dir = temp_dir.path();
+        let mut args = Args::for_test(build_dir, "glyphs3/WghtVar.glyphs");
+        // first compile with default args (keep_direction=false)
+        compile(args.clone());
+
+        let ir_glyph = read_ir_glyph(build_dir, "hyphen");
+        let ir_default_instance = ir_glyph.default_instance();
+        assert_eq!(
+            &ir_default_instance.contours[0].to_svg(),
+            "M131,330 L131,250 L470,250 L470,330 L131,330 Z"
+        );
+
+        let RawGlyph::Simple(glyph) = read_be_glyph(build_dir, "hyphen") else {
+            panic!("Expected a simple glyph");
+        };
+
+        // point order in the compiled glyph is reversed compared to the IR glyph
+        // while the starting point is the same
+        assert_eq!(
+            vec![
+                CurvePoint::on_curve(131, 330),
+                CurvePoint::on_curve(470, 330),
+                CurvePoint::on_curve(470, 250),
+                CurvePoint::on_curve(131, 250),
+            ],
+            glyph
+                .contours()
+                .iter()
+                .flat_map(|c| c.iter())
+                .copied()
+                .collect::<Vec<_>>()
+        );
+
+        // recompile with --keep-direction
+        args.keep_direction = true;
+        compile(args);
+
+        let RawGlyph::Simple(glyph) = read_be_glyph(build_dir, "hyphen") else {
+            panic!("Expected a simple glyph");
+        };
+
+        // order/starting point in the compiled glyph are the same as in the IR glyph
+        assert_eq!(
+            vec![
+                CurvePoint::on_curve(131, 330),
+                CurvePoint::on_curve(131, 250),
+                CurvePoint::on_curve(470, 250),
+                CurvePoint::on_curve(470, 330),
+            ],
+            glyph
+                .contours()
+                .iter()
+                .flat_map(|c| c.iter())
+                .copied()
+                .collect::<Vec<_>>()
+        );
+    }
 }

fontir/src/orchestration.rs

@@ -32,6 +32,8 @@ bitflags! {
         const DECOMPOSE_TRANSFORMED_COMPONENTS = 0b00010000;
         // If set a files reporting on timing will be emitted to disk
         const EMIT_TIMING = 0b00100000;
+        // If set, the direction of contours will NOT be reversed
+        const KEEP_DIRECTION = 0b01000000;
     }
 }
 

resources/scripts/ttx_diff.py

@@ -97,6 +97,8 @@ def build_fontc(source: Path, build_dir: Path, compare: str):
         "-p",
         "fontc",
         "--",
+        # uncomment this to compare output w/ fontmake --keep-direction
+        # "--keep-direction",
         "--source",
         str(source),
         "--build-dir",
@@ -118,7 +120,7 @@ def build_fontmake(source: Path, build_dir: Path, compare: str):
         "--drop-implied-oncurves",
         # TODO we shouldn't need these flags
         "--no-production-names",
-        "--keep-direction",
+        # "--keep-direction",
     ]
     if compare == _COMPARE_GFTOOLS:
         cmd += [