Skip to content

Commit

Permalink
Tidy up allocs in glyph ir work based on flamegraph
Browse files Browse the repository at this point in the history
  • Loading branch information
@rsheeter
rsheeter committed Aug 10, 2023
1 parent 51aa663 commit dfe24c3
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 55 deletions.
130 changes: 81 additions & 49 deletions fontir/src/ir.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use font_types::NameId;
use font_types::Tag;
use fontdrasil::types::{GlyphName, GroupName};
use indexmap::IndexSet;
use kurbo::{Affine, BezPath, Point};
use kurbo::{Affine, BezPath, PathEl, Point};
use log::warn;
use ordered_float::OrderedFloat;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -1028,18 +1028,21 @@ pub struct GlyphPathBuilder {
glyph_name: GlyphName,
offcurve: Vec<Point>,
leading_offcurve: Vec<Point>,
path: BezPath,
path: Vec<PathEl>,
first_oncurve: Option<OnCurve>,
}

impl GlyphPathBuilder {
/// Create a new GlyphPathBuilder for a glyph with the given name.
pub fn new(glyph_name: GlyphName) -> GlyphPathBuilder {
pub fn new(glyph_name: GlyphName, estimated_num_elements: usize) -> GlyphPathBuilder {
let mut capacity = estimated_num_elements.next_power_of_two();
if capacity == estimated_num_elements {
capacity += 4; // close path often adds a few
}
GlyphPathBuilder {
glyph_name,
offcurve: Vec::new(),
offcurve: Vec::with_capacity(2),
leading_offcurve: Vec::new(),
path: BezPath::new(),
path: Vec::with_capacity(capacity),
first_oncurve: None,
}
}
Expand All @@ -1058,13 +1061,13 @@ impl GlyphPathBuilder {
Ok(())
}

fn is_empty(&self) -> bool {
pub fn is_empty(&self) -> bool {
self.first_oncurve.is_none() && self.leading_offcurve.is_empty()
}

fn begin_path(&mut self, oncurve: OnCurve) -> Result<(), PathConversionError> {
assert!(self.first_oncurve.is_none());
self.path.move_to(*oncurve.point());
self.path.push(PathEl::MoveTo(*oncurve.point()));
self.first_oncurve = Some(oncurve);
Ok(())
}
Expand Down Expand Up @@ -1094,7 +1097,7 @@ impl GlyphPathBuilder {
if self.first_oncurve.is_none() {
self.begin_path(OnCurve::Line(p.into()))?;
} else {
self.path.line_to(p);
self.path.push(PathEl::LineTo(p.into()));
}
Ok(())
}
Expand Down Expand Up @@ -1124,10 +1127,11 @@ impl GlyphPathBuilder {
let next = window[1];
// current offcurve to halfway to the next one
let implied = Point::new((curr.x + next.x) / 2.0, (curr.y + next.y) / 2.0);
self.path.quad_to(curr, implied);
self.path.push(PathEl::QuadTo(curr, implied));
}
// last but not least, the last offcurve to the provided point
self.path.quad_to(*self.offcurve.last().unwrap(), p.into());
self.path
.push(PathEl::QuadTo(*self.offcurve.last().unwrap(), p.into()));
self.offcurve.clear();

Ok(())
Expand All @@ -1142,11 +1146,13 @@ impl GlyphPathBuilder {
pub fn curve_to(&mut self, p: impl Into<Point>) -> Result<(), PathConversionError> {
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()),
2 => self
.path
.curve_to(self.offcurve[0], self.offcurve[1], p.into()),
0 => self.path.push(PathEl::LineTo(p.into())),
1 => self.path.push(PathEl::QuadTo(self.offcurve[0], p.into())),
2 => self.path.push(PathEl::CurveTo(
self.offcurve[0],
self.offcurve[1],
p.into(),
)),
_ => self.check_num_offcurve(|v| v < 3)?,
}
self.offcurve.clear();
Expand Down Expand Up @@ -1205,7 +1211,7 @@ impl GlyphPathBuilder {
OnCurve::Cubic(pt) => self.curve_to(pt)?,
_ => unreachable!(),
}
self.path.close_path();
self.path.push(PathEl::ClosePath);
} 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
Expand All @@ -1221,7 +1227,7 @@ impl GlyphPathBuilder {
/// Builds the kurbo::BezPath from the accumulated points.
pub fn build(mut self) -> Result<BezPath, PathConversionError> {
self.end_path()?;
Ok(self.path)
Ok(BezPath::from_vec(self.path))
}
}

Expand Down Expand Up @@ -1271,71 +1277,86 @@ mod tests {
}

#[test]
fn a_qcurve_with_no_offcurve_is_a_line() {
let mut builder = GlyphPathBuilder::new("test".into());
fn a_qcurve_with_no_offcurve_is_a_line_open_contour() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
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());
#[test]
fn a_qcurve_with_no_offcurve_is_a_line_closed_contour() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
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
fn a_curve_with_no_offcurve_is_a_line_open_contour() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
builder.move_to((2.0, 2.0)).unwrap(); // open contour
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());
#[test]
fn a_curve_with_no_offcurve_is_a_line_closed_contour() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
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());
fn a_curve_with_one_offcurve_is_a_single_quad_open_contour() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
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());
#[test]
fn a_curve_with_one_offcurve_is_a_single_quad_closed_contour() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
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(); // open
fn a_qcurve_with_one_offcurve_is_a_single_quad_to_open_contour() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
builder.move_to((2.0, 2.0)).unwrap();
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());
#[test]
fn a_qcurve_with_one_offcurve_is_a_single_quad_to_closed_contour() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
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(); // open
fn a_qcurve_with_two_offcurve_is_two_quad_to_open_contour() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
builder.move_to((2.0, 2.0)).unwrap();
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());
#[test]
fn a_qcurve_with_two_offcurve_is_two_quad_to_closed_contour() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
builder.qcurve_to((2.0, 2.0)).unwrap(); // closed
builder.offcurve((3.0, 0.0)).unwrap();
builder.offcurve((5.0, 4.0)).unwrap();
Expand All @@ -1347,48 +1368,59 @@ mod tests {
}

#[test]
fn last_line_always_emit_implied_closing_line() {
let mut builder = GlyphPathBuilder::new("test".into());
builder.line_to((2.0, 2.0)).unwrap();
fn last_line_always_emits_implied_closing_line() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
builder.move_to((2.0, 2.0)).unwrap();
builder.line_to((4.0, 2.0)).unwrap();
// a closing line is implied by Z, but emit it nonetheless
assert_eq!("M2,2 L4,2 L2,2 Z", builder.build().unwrap().to_svg());
}

let mut builder = GlyphPathBuilder::new("test".into());
builder.line_to((2.0, 2.0)).unwrap();
#[test]
fn last_line_emits_nop_implied_closing_line() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
builder.move_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();
assert_eq!("M2,2 L4,2 L2,2 L2,2 Z", builder.build().unwrap().to_svg());
}

#[test]
fn last_curve_equals_move_no_closing_line() {
fn last_quad_equals_move_no_closing_line() {
// 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());
let mut builder = GlyphPathBuilder::new("test".into(), 0);
builder.move_to((2.0, 2.0)).unwrap();
builder.offcurve((3.0, 0.0)).unwrap();
builder.qcurve_to((2.0, 2.0)).unwrap();
assert_eq!("M2,2 Q3,0 2,2 Z", builder.build().unwrap().to_svg());
}

let mut builder = GlyphPathBuilder::new("test".into());
#[test]
fn last_cubic_equals_move_no_closing_line() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
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();
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() {
fn last_quad_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.line_to((2.0, 2.0)).unwrap();
let mut builder = GlyphPathBuilder::new("test".into(), 0);
builder.move_to((2.0, 2.0)).unwrap();
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());
}

let mut builder = GlyphPathBuilder::new("test".into());
builder.line_to((2.0, 2.0)).unwrap();
#[test]
fn last_cubic_not_equal_move_do_emit_closing_line() {
let mut builder = GlyphPathBuilder::new("test".into(), 0);
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((4.0, 2.0)).unwrap();
Expand Down
9 changes: 5 additions & 4 deletions glyphs2fontir/src/toir.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ pub(crate) fn to_ir_contours_and_components(
glyph_name: GlyphName,
shapes: &[Shape],
) -> Result<(Vec<BezPath>, Vec<ir::Component>), WorkError> {
let mut contours = Vec::new();
// For most glyphs in most fonts all the shapes are contours so it's a good guess
let mut contours = Vec::with_capacity(shapes.len());
let mut components = Vec::new();

for shape in shapes.iter() {
Expand Down Expand Up @@ -47,12 +48,12 @@ fn to_ir_component(glyph_name: GlyphName, component: &Component) -> ir::Componen
fn to_ir_path(glyph_name: GlyphName, src_path: &Path) -> Result<BezPath, WorkError> {
// Based on https://github.com/googlefonts/glyphsLib/blob/24b4d340e4c82948ba121dcfe563c1450a8e69c9/Lib/glyphsLib/builder/paths.py#L20
// 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(BezPath::new());
}

let mut nodes: Vec<_> = src_path.nodes.clone();
let mut nodes = &src_path.nodes[..];
let mut path_builder = GlyphPathBuilder::new(glyph_name.clone(), nodes.len());

// First is a delicate butterfly
if !src_path.closed {
Expand Down
5 changes: 3 additions & 2 deletions ufo2fontir/src/toir.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,12 @@ 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(BezPath::new());
}

let mut path_builder = GlyphPathBuilder::new(glyph_name.clone(), contour.points.len());

// Walk through the remaining points, accumulating off-curve points until we see an on-curve
for node in contour.points.iter() {
match node.typ {
Expand Down

0 comments on commit dfe24c3

Please sign in to comment.