Skip to content

Conversation

@rsheeter
Copy link
Contributor

@rsheeter rsheeter commented Mar 10, 2023

Split out from #120 to try to make review more tractable.

Compile simple static glyphs. cu2qu in a manner that should work for variations.

Blocked on googlefonts/fontations#258, but I don't expect that to appreciably change the logic so I believe this is reviewable. Edit: fixed.

@rsheeter rsheeter force-pushed the simple_glyphs branch 3 times, most recently from 4b10dcd to b50438a Compare March 10, 2023 07:13
@rsheeter rsheeter marked this pull request as ready for review March 10, 2023 07:13
@rsheeter rsheeter changed the title Build simple glyphs Build simple glyphs (static glyf, 2 of 3) Mar 10, 2023
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel my brain has largely departed me for the day, but nothing here jumps out as egregious 👍

Base automatically changed from orch to main March 10, 2023 22:11
@rsheeter rsheeter merged commit 6bf9564 into main Mar 10, 2023
@rsheeter rsheeter deleted the simple_glyphs branch March 10, 2023 22:24
@rsheeter
Copy link
Contributor Author

@anthrotype although now merged I would still love your feedback on this

Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

.collect();
new_glyphs.sort();
trace!(
"Added {} additional glyphs: {new_glyphs:?}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why just "added {} additional glyphs"? It is not conceivable that glyphs are also removed when current != new glyph_order?

fn add_glyph_be_job(workload: &mut Workload, fe_root: &FeContext, glyph_name: GlyphName) {
let glyph_ir = fe_root.get_glyph_ir(&glyph_name);

// To build a glyph we need it's components, plus static metadata
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

+ we need it's components
- we need its components

let successes = self.read_completions(&recv, RecvType::Blocking)?;
successes
.into_iter()
.for_each(|s| self.handle_success(&fe_root, s));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noob question: what's the advantage of using iterator+for_each closure over an explicit for loop here?
Is it just taste? I should probably get used to it ..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually agree with you, I think this is confusing, and maybe doing too much.

.collect();

// At long last, actually convert something to quadratic
// TODO what should we pass for accuracy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python cu2qu defaults to a relative max_error of 1/1000th of the font's UPEM; e.g. for a usual font with head.unitsPerEm=1000, the absolute max_error is 1 font unit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants