From f1e0180bc60e72c2d7ace172f2d33583e6c8802f Mon Sep 17 00:00:00 2001 From: Kostya Farber Date: Thu, 2 Jul 2026 08:19:30 +0100 Subject: [PATCH] fix: lift TrueType quadratics to cubics on binary import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The binary reader's pen emitted each quadratic as a single off-curve plus an on-curve, which the UFO writer then classified as a cubic curve segment with one control point — corrupting TTF imports on save. Each quad is now lifted to its exact cubic equivalent, keeping the IR cubic-only (the wire and renderer layers have no quadratic support). Also drop the duplicate start point skrifa emits when a contour's closing segment is a curve; closed contours already imply a return to the first point. --- crates/shift-backends/src/binary/reader.rs | 252 +++++++++++++++++- crates/shift-backends/tests/loading.rs | 71 +++++ crates/shift-backends/tests/round_trip/ufo.rs | 72 +++++ 3 files changed, 391 insertions(+), 4 deletions(-) diff --git a/crates/shift-backends/src/binary/reader.rs b/crates/shift-backends/src/binary/reader.rs index 6afc2d5b..13432ce1 100644 --- a/crates/shift-backends/src/binary/reader.rs +++ b/crates/shift-backends/src/binary/reader.rs @@ -47,11 +47,25 @@ impl OutlinePen for ShiftPen { .add_point(x as f64, y as f64, PointType::OnCurve, false); } + /// Binary imports produce cubic-only IR: the wire and renderer layers do + /// not support quadratic segments, so every TrueType quadratic is lifted + /// to its exact cubic equivalent (mathematically lossless — every + /// quadratic is a cubic with c1 = q0 + 2/3(q1 - q0), c2 = q2 + 2/3(q1 - q2)). fn quad_to(&mut self, cx0: f32, cy0: f32, x: f32, y: f32) { - self.current_contour() - .add_point(cx0 as f64, cy0 as f64, PointType::OffCurve, false); - self.current_contour() - .add_point(x as f64, y as f64, PointType::OnCurve, false); + let contour = self.current_contour(); + let q0 = contour + .last_point() + .expect("quad_to requires a current point"); + let (q0x, q0y) = (q0.x(), q0.y()); + let (q1x, q1y) = (cx0 as f64, cy0 as f64); + let (q2x, q2y) = (x as f64, y as f64); + let c1x = q0x + 2.0 / 3.0 * (q1x - q0x); + let c1y = q0y + 2.0 / 3.0 * (q1y - q0y); + let c2x = q2x + 2.0 / 3.0 * (q1x - q2x); + let c2y = q2y + 2.0 / 3.0 * (q1y - q2y); + contour.add_point(c1x, c1y, PointType::OffCurve, false); + contour.add_point(c2x, c2y, PointType::OffCurve, false); + contour.add_point(q2x, q2y, PointType::OnCurve, false); } fn curve_to(&mut self, cx0: f32, cy0: f32, cx1: f32, cy1: f32, x: f32, y: f32) { @@ -63,8 +77,19 @@ impl OutlinePen for ShiftPen { .add_point(x as f64, y as f64, PointType::OnCurve, false); } + /// Closes the current contour. skrifa ends contours whose final segment + /// is a curve with an explicit on-curve at the start point; closing a + /// contour in the IR already implies returning to the first point, so + /// that duplicate is dropped to avoid a degenerate zero-length segment. fn close(&mut self) { if let Some(contour) = self.contours.last_mut() { + if contour.len() > 1 { + let first = contour.first_point().expect("contour is non-empty"); + let last = contour.last_point().expect("contour is non-empty"); + if last.is_on_curve() && last.x() == first.x() && last.y() == first.y() { + contour.points_mut().pop(); + } + } contour.close(); } } @@ -197,6 +222,225 @@ fn localized_string(font: &FontRef<'_>, id: StringId) -> Option { #[cfg(test)] mod tests { use super::*; + use shift_font::Point; + use skrifa::outline::pen::PathElement; + use std::path::PathBuf; + + fn mutatorsans_ttf_path() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .parent() + .unwrap() + .parent() + .unwrap() + .join("fixtures/fonts/mutatorsans/MutatorSans.ttf") + } + + fn quad_at(q0: (f64, f64), q1: (f64, f64), q2: (f64, f64), t: f64) -> (f64, f64) { + let u = 1.0 - t; + ( + u * u * q0.0 + 2.0 * u * t * q1.0 + t * t * q2.0, + u * u * q0.1 + 2.0 * u * t * q1.1 + t * t * q2.1, + ) + } + + fn cubic_at( + p0: (f64, f64), + c1: (f64, f64), + c2: (f64, f64), + p3: (f64, f64), + t: f64, + ) -> (f64, f64) { + let u = 1.0 - t; + ( + u * u * u * p0.0 + 3.0 * u * u * t * c1.0 + 3.0 * u * t * t * c2.0 + t * t * t * p3.0, + u * u * u * p0.1 + 3.0 * u * u * t * c1.1 + 3.0 * u * t * t * c2.1 + t * t * t * p3.1, + ) + } + + fn point_types(contour: &Contour) -> Vec { + contour.points().iter().map(|p| p.point_type()).collect() + } + + #[test] + fn quad_lifted_to_geometrically_identical_cubic() { + let mut pen = ShiftPen::default(); + pen.move_to(10.0, 20.0); + pen.quad_to(50.0, 90.0, 100.0, 20.0); + + let contours = pen.contours(); + let points = contours[0].points(); + assert_eq!( + point_types(&contours[0]), + vec![ + PointType::OnCurve, + PointType::OffCurve, + PointType::OffCurve, + PointType::OnCurve + ] + ); + + let q0 = (10.0, 20.0); + let q1 = (50.0, 90.0); + let q2 = (100.0, 20.0); + let c1 = (points[1].x(), points[1].y()); + let c2 = (points[2].x(), points[2].y()); + for step in 0..=20 { + let t = step as f64 / 20.0; + let expected = quad_at(q0, q1, q2, t); + let actual = cubic_at(q0, c1, c2, q2, t); + assert!( + (expected.0 - actual.0).abs() < 1e-9 && (expected.1 - actual.1).abs() < 1e-9, + "lifted cubic diverges from quad at t={t}: {expected:?} vs {actual:?}" + ); + } + } + + #[test] + fn close_drops_duplicate_start_point_from_closing_curve() { + let mut pen = ShiftPen::default(); + pen.move_to(0.0, 0.0); + pen.line_to(100.0, 0.0); + pen.quad_to(100.0, 100.0, 0.0, 0.0); + pen.close(); + + let contours = pen.contours(); + let contour = &contours[0]; + assert!(contour.is_closed()); + assert_eq!( + point_types(contour), + vec![ + PointType::OnCurve, + PointType::OnCurve, + PointType::OffCurve, + PointType::OffCurve + ], + "closing curve's explicit end point should be dropped; the closed contour wraps to the start" + ); + } + + #[test] + fn close_keeps_distinct_last_point() { + let mut pen = ShiftPen::default(); + pen.move_to(0.0, 0.0); + pen.line_to(100.0, 0.0); + pen.line_to(100.0, 100.0); + pen.close(); + + let contours = pen.contours(); + assert!(contours[0].is_closed()); + assert_eq!(contours[0].len(), 3); + } + + #[test] + fn line_after_quad_composes() { + let mut pen = ShiftPen::default(); + pen.move_to(0.0, 0.0); + pen.quad_to(50.0, 100.0, 100.0, 0.0); + pen.line_to(200.0, 0.0); + pen.close(); + + let contours = pen.contours(); + assert_eq!( + point_types(&contours[0]), + vec![ + PointType::OnCurve, + PointType::OffCurve, + PointType::OffCurve, + PointType::OnCurve, + PointType::OnCurve + ] + ); + } + + #[test] + fn imported_cubics_match_source_quadratics() { + let path = mutatorsans_ttf_path(); + let bytes = std::fs::read(&path).expect("MutatorSans.ttf fixture should exist"); + let font_ref = FontRef::new(&bytes).unwrap(); + + let glyph_id = font_ref + .charmap() + .map('O') + .expect("MutatorSans should map 'O'"); + let mut elements: Vec = Vec::new(); + font_ref + .outline_glyphs() + .get(glyph_id) + .unwrap() + .draw( + DrawSettings::unhinted(Size::unscaled(), LocationRef::default()), + &mut elements, + ) + .unwrap(); + + let font = font_from_skrifa(&font_ref).unwrap(); + let glyph = font + .glyphs_by_unicode('O' as u32) + .next() + .expect("imported font should contain 'O'"); + let layer = glyph + .layers() + .values() + .next() + .expect("glyph should have a layer"); + + let mut cubic_segments: Vec<[(f64, f64); 4]> = Vec::new(); + for contour in layer.contours_iter() { + let points: Vec<&Point> = contour.points().iter().collect(); + let len = points.len(); + for i in 0..len { + let window: Vec<&Point> = (0..4).map(|offset| points[(i + offset) % len]).collect(); + if window[0].is_on_curve() + && window[1].point_type() == PointType::OffCurve + && window[2].point_type() == PointType::OffCurve + && window[3].is_on_curve() + { + cubic_segments.push([0, 1, 2, 3].map(|p| (window[p].x(), window[p].y()))); + } + } + } + + let mut current = (0.0, 0.0); + let mut quads_checked = 0; + for element in elements { + match element { + PathElement::MoveTo { x, y } | PathElement::LineTo { x, y } => { + current = (x as f64, y as f64); + } + PathElement::QuadTo { cx0, cy0, x, y } => { + let q0 = current; + let q1 = (cx0 as f64, cy0 as f64); + let q2 = (x as f64, y as f64); + let segment = cubic_segments + .iter() + .find(|[p0, _, _, p3]| *p0 == q0 && *p3 == q2) + .unwrap_or_else(|| { + panic!("no imported cubic segment from {q0:?} to {q2:?}") + }); + for step in 1..8 { + let t = step as f64 / 8.0; + let expected = quad_at(q0, q1, q2, t); + let actual = cubic_at(segment[0], segment[1], segment[2], segment[3], t); + assert!( + (expected.0 - actual.0).abs() < 1e-9 + && (expected.1 - actual.1).abs() < 1e-9, + "imported cubic diverges from source quad at t={t}" + ); + } + quads_checked += 1; + current = q2; + } + PathElement::CurveTo { x, y, .. } => { + current = (x as f64, y as f64); + } + PathElement::Close => {} + } + } + assert!( + quads_checked > 0, + "MutatorSans.ttf 'O' should contain quadratic segments" + ); + } fn make_closed_contour(points: Vec<(f64, f64, PointType)>) -> Contour { let mut contour = Contour::new(); diff --git a/crates/shift-backends/tests/loading.rs b/crates/shift-backends/tests/loading.rs index 39f08cf3..279167a5 100644 --- a/crates/shift-backends/tests/loading.rs +++ b/crates/shift-backends/tests/loading.rs @@ -146,6 +146,77 @@ fn loads_binary_fonts_with_contours() { } } +fn assert_cubic_point_runs(contour: &Contour, context: &str) { + let points = contour.points(); + assert!(!points.is_empty(), "empty contour in {context}"); + assert!( + points[0].is_on_curve(), + "contour should start with an on-curve point in {context}" + ); + + let mut off_run = 0; + for point in &points[1..] { + match point.point_type() { + PointType::OffCurve => off_run += 1, + PointType::OnCurve => { + assert!( + off_run == 0 || off_run == 2, + "on-curve point preceded by {off_run} off-curves in {context}" + ); + off_run = 0; + } + other => panic!("unexpected point type {other:?} in {context}"), + } + } + + if contour.is_closed() { + assert!( + off_run == 0 || off_run == 2, + "closing segment has {off_run} off-curves in {context}" + ); + let first = &points[0]; + let last = &points[points.len() - 1]; + assert!( + points.len() == 1 + || !(last.is_on_curve() && last.x() == first.x() && last.y() == first.y()), + "closed contour duplicates its start point in {context}" + ); + } else { + assert_eq!( + off_run, 0, + "open contour ends with {off_run} dangling off-curves in {context}" + ); + } +} + +#[test] +fn binary_import_produces_valid_cubic_point_runs() { + for path in [mutatorsans_ttf_path(), mutatorsans_otf_path()] { + let font = load_font(&path); + let mut curve_contours = 0; + for glyph in font.glyphs() { + for layer in glyph.layers().values() { + for contour in layer.contours_iter() { + let context = format!("glyph '{}' in {}", glyph.name(), path.display()); + assert_cubic_point_runs(contour, &context); + if contour + .points() + .iter() + .any(|point| point.point_type() == PointType::OffCurve) + { + curve_contours += 1; + } + } + } + } + assert!( + curve_contours > 0, + "{} should import contours with curve segments", + path.display() + ); + } +} + #[test] fn binary_font_missing_hmtx_returns_error_instead_of_panicking() { let mut bytes = std::fs::read(mutatorsans_ttf_path()).unwrap(); diff --git a/crates/shift-backends/tests/round_trip/ufo.rs b/crates/shift-backends/tests/round_trip/ufo.rs index 09d2c115..08f2a694 100644 --- a/crates/shift-backends/tests/round_trip/ufo.rs +++ b/crates/shift-backends/tests/round_trip/ufo.rs @@ -16,6 +16,10 @@ fn mutatorsans_ufo_path() -> PathBuf { fixtures_path().join("fonts/mutatorsans/MutatorSansLightCondensed.ufo") } +fn mutatorsans_ttf_path() -> PathBuf { + fixtures_path().join("fonts/mutatorsans/MutatorSans.ttf") +} + fn load_font(path: &Path) -> Font { assert!(path.exists(), "missing font fixture at {}", path.display()); FontLoader::new() @@ -195,3 +199,71 @@ fn preserves_components_anchors_layers_and_kerning() { assert_eq!(reloaded.kerning().get_kerning("T", "A"), Some(-75.0)); assert_eq!(reloaded.kerning().get_kerning("V", "A"), Some(-100.0)); } + +#[test] +fn ttf_import_saves_spec_valid_ufo_curves() { + let font = load_font(&mutatorsans_ttf_path()); + + let temp_dir = tempfile::tempdir().expect("tempdir should be created"); + let output_path = temp_dir.path().join("from-ttf.ufo"); + FontLoader::new() + .write_font(&font, output_path.to_str().unwrap()) + .expect("UFO writer should save the TTF-imported font"); + + let norad_font = norad::Font::load(&output_path) + .expect("norad should parse every glif written from a TTF import"); + + let mut curve_points = 0; + for layer in norad_font.layers.iter() { + for glyph in layer.iter() { + for contour in &glyph.contours { + let points = &contour.points; + for (index, point) in points.iter().enumerate() { + match point.typ { + norad::PointType::Curve => { + assert!( + points.len() >= 3, + "curve point without room for controls in glyph '{}'", + glyph.name() + ); + let len = points.len(); + let prev = &points[(index + len - 1) % len]; + let prev_prev = &points[(index + len - 2) % len]; + let before_controls = &points[(index + len - 3) % len]; + assert!( + prev.typ == norad::PointType::OffCurve + && prev_prev.typ == norad::PointType::OffCurve + && before_controls.typ != norad::PointType::OffCurve, + "curve point in glyph '{}' is not preceded by exactly two off-curves", + glyph.name() + ); + curve_points += 1; + } + norad::PointType::OffCurve => { + let next = &points[(index + 1) % points.len()]; + assert!( + matches!( + next.typ, + norad::PointType::OffCurve | norad::PointType::Curve + ), + "off-curve in glyph '{}' is not part of a cubic curve segment", + glyph.name() + ); + } + norad::PointType::QCurve => { + panic!( + "TTF import should not produce qcurve points (glyph '{}')", + glyph.name() + ); + } + norad::PointType::Line | norad::PointType::Move => {} + } + } + } + } + } + assert!( + curve_points > 0, + "TTF import round trip should contain curve segments" + ); +}