Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial glyph loading/scaling crate #191

Merged
merged 29 commits into from
Jan 24, 2023
Merged

Initial glyph loading/scaling crate #191

merged 29 commits into from
Jan 24, 2023

Conversation

dfrg
Copy link
Member

@dfrg dfrg commented Dec 20, 2022

This adds a new crate called punchcut containing the initial TrueType glyph scaling code based on read-fonts. The caching and hinting code has been extracted to hopefully make this more reviewable.

A few notes:

  • There are fixed point math functions that have some overlap with code in font-types. FreeType provides outline points as either integral font units or fixed point (26.6) values depending on whether a scale has been applied. We copy that here and that makes some of the math and types less than ideal. I'm open to options for making this nicer (without loss in perf/memory) but I'd like to not block the initial cut on it.
  • There is a tiered architecture that exposes the individual sources (just glyf for now) as well as a higher level interface at the root of the crate. The layering feels pretty good to me right now, but comments for improvement are welcome.
  • I've tried to comment some of the bits in source/glyf/scaler.rs that might be tricky. Feel free to point out any that require further explanation.

This has nice integration with kurbo: the Outline type implements kurbo::Shape so these can be fed directly into vello (née piet-gpu). I plan on using this for glyph loading support in that crate soon™.

This originally had an abstract `Glyph` type that was intended to handle all possible outputs (outline, color outline, bitmaps). This was modeled after swash which took a prioritized array of source types and would select the appropriate format internally. We're providing explicit methods for the different "kinds" here, so have each method return a concrete type instead.
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.

A bunch of comments inline, but overall, (and skipping over the fact that I don't have a particularly firm grasp on all of the business logic) this seems quite clean!

I think there's likely some room for improvement around types, as you say, but I don't have a ton of concrete proposals, outside of 'more things should be types, and more things like conversion logic and operations should be methods on those types'.

so lgtm, but I am not a domain expert.

punchcut/README.md Outdated Show resolved Hide resolved
punchcut/docs/capi.md Outdated Show resolved Hide resolved
punchcut/src/lib.rs Outdated Show resolved Hide resolved
punchcut/src/lib.rs Outdated Show resolved Hide resolved
punchcut/src/outline.rs Outdated Show resolved Hide resolved
punchcut/src/source/glyf/scaler.rs Outdated Show resolved Hide resolved
punchcut/src/source/glyf/scaler.rs Show resolved Hide resolved
punchcut/src/source/glyf/scaler.rs Outdated Show resolved Hide resolved
punchcut/src/source/glyf/scaler.rs Outdated Show resolved Hide resolved
punchcut/src/source/glyf/scaler.rs Outdated Show resolved Hide resolved
@madig
Copy link

madig commented Dec 26, 2022

So, this crate is for loading glyphs from various OT formats and turning them into kurbo datatypes, potentially applying hinting?

@dfrg
Copy link
Member Author

dfrg commented Dec 26, 2022

Yes, that’s the intent. There are working hinters for both TrueType and CFF but I left the former out to keep the PR size more manageable and the latter is waiting on CFF parsing in fontations.

@madig
Copy link

madig commented Dec 26, 2022

Interesting. Did you port FreeType's TTF hinting engine?

Copy link
Contributor

@drott drott left a comment

Choose a reason for hiding this comment

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

Hope that's okay if I leave a couple of drive-by comments after I was familiarizing myself with the punchcut API and the suggestions for the C FFI.

punchcut/docs/capi.md Outdated Show resolved Hide resolved
punchcut/src/lib.rs Outdated Show resolved Hide resolved
punchcut/Cargo.toml Outdated Show resolved Hide resolved
punchcut/src/source/glyf/math.rs Outdated Show resolved Hide resolved
punchcut/src/lib.rs Outdated Show resolved Hide resolved
dfrg and others added 2 commits January 11, 2023 12:31
Co-authored-by: Colin Rofls <colin@cmyr.net>
Co-authored-by: Colin Rofls <colin@cmyr.net>
@dfrg
Copy link
Member Author

dfrg commented Jan 11, 2023

Interesting. Did you port FreeType's TTF hinting engine?

Yes, I ported it a while back for the swash crate.

dfrg and others added 10 commits January 11, 2023 13:01
punchcut/README.md Outdated Show resolved Hide resolved
punchcut/README.md Outdated Show resolved Hide resolved
punchcut/README.md Outdated Show resolved Hide resolved
surfaces.

The primary purpose of this crate is the generation of outlines from font data, so the name seemed
appropriate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Provide (a link could suffice) a basic example of use, say loading a font from a file and getting the outline of something.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add some example code. Inline is fairly common in Rust READMEs

/// Set of points that define the shape of the outline.
pub points: Vec<Point>,
/// Set of tags (one per point).
pub tags: Vec<u8>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not Vec(Point, PointTag) then? - would make the tag-per-point relation explicit and unscrewupable.

Copy link
Member Author

Choose a reason for hiding this comment

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

A lot of context is lost with hinting and variations being missing. The tags array is also used during delta computation and hinting and is temporarily associated with other point arrays (original, unscaled, deltas) during those processes.


/// Converts the outline to a sequence of path commands and invokes the callback for
/// each on the given sink.
pub fn to_path(&self, sink: &mut impl PathSink) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

more comments, imagine a coder new to fonts is trying to follow.

context: &'a mut Context,
/// Current font data.
font: Font<'a>,
/// Font identifier for the hinting cache.
Copy link
Collaborator

Choose a reason for hiding this comment

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

help the reader understand how such ids are created, preferably by having a type, at the very least a newtype not u64, and some lovely comments on said type

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to newtype, but it needs to be flexible. We don't want to be responsible for generation or management of unique font ids. I mentioned above that I tried that with swash and it ended up being a huge source of friction for just about everybody trying to work with it (including me :)

}

/// State for loading a glyph.
struct GlyphScaler<'a, 'b> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

just 'a I can handle but if you are going to have more than 1 I feel strongly names and/or commentary makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will make this more clear.

Copy link
Collaborator

@rsheeter rsheeter left a comment

Choose a reason for hiding this comment

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

LGTM overall.

I think three key things need to be addressed, either here or in follow-ons:

  1. Testing is very light, perhaps it could become data driven with instructions on how to regenerate or extend the data?
  2. Commenting is very light; imagine you want a competent coder who isn't a font expert to be able to have a clue
    • Also please remove pointless comments. For example, hints: bool doesn't benefit from a comment saying True if hints enabled.
    • That is, I'm not looking for pointless comment every field stuff, I want the core logic and references to be captured where not obvious
  3. Everything related to hinting needs to be either feature gated or deleted

If you will do follow-on PRs please file issues so we don't forget.

Edit: also apologies for taking so long to get to reviewing this.

@dfrg
Copy link
Member Author

dfrg commented Jan 13, 2023

Thanks for the review.

I'd rather address (at least the bulk of) the issues here and ping for follow ups rather than merging and letting them linger.

@dfrg
Copy link
Member Author

dfrg commented Jan 13, 2023

Addressed most of the low-hanging fruit, plus removed the peniko/kurbo dependencies and feature gated all of the hinting code. I'll look at the rest tomorrow (today?)

@dfrg
Copy link
Member Author

dfrg commented Jan 13, 2023

Opened issue #203 to discuss better testing wrt comparison with FreeType.

* add F26Dot6 type to font-types
* remove the now unnecessary math module
* change TrueType scaler to always return results in 26.6 even when unscaled. This differs from FreeType but gives us a consistent API where the interpretation of values is not dependent on a bool flag (Outline::is_scaled which has also been removed).
* adds a python script to extract glyph outlines using freetype-py and ties it into other infra
* change testing to use the new extracted glyph outlines
@dfrg
Copy link
Member Author

dfrg commented Jan 15, 2023

  1. Testing is very light, perhaps it could become data driven with instructions on how to regenerate or extend the data?
  2. Commenting is very light; imagine you want a competent coder who isn't a font expert to be able to have a clue
  3. Everything related to hinting needs to be either feature gated or deleted

Status now:

  1. Partially done in that new testing infra is in place including automatic extraction of the outline data. Test cases are still sparse and I've decided to defer that with Add additional interesting test ttx files for glyph loading #204 opened to track.
  2. Still pending. I'll spend some time on more complete documentation in the next few days.
  3. Done

@dfrg
Copy link
Member Author

dfrg commented Jan 24, 2023

We've agreed that now is a good time to merge this. Thanks to everyone for both detailed reviews and patience. The code has been substantially improved as a result.

There are some minor remaining adjustments needed which I've written up in #209 to track.

@dfrg dfrg merged commit 9eadacd into googlefonts:main Jan 24, 2023
@dfrg dfrg deleted the load-glyphs branch January 24, 2023 19:35
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.

5 participants