[skrifa] new hinting options type #1087
Conversation
Exposes the glyph style map as a public type and wrapped in an `Arc` to make it shareable among multiple autohinting instances.
Per IM, changes skrifa to use `FontRef` directly rather than `TableProvider`. Extracts some shared functionality into a `BaseScaler` type for outlines in preparation for more autohinting work but no real functional changes.
There was a problem hiding this comment.
Thank you for continuing to upstream autohinting, great to see this.
No concerns for HintingOptions and the configuration API. This looks as discussed earlier and seems good to be used in Skia (with a reservation that I have not tried it yet).
But the is_italic argument to Scale feels odd to me and seems a surprising configuration side effect to hinting later. I don't know how to set this argument correctly from the client side. (Besides the general concern with boolean API args that are hard to read at the call site.)
| /// | ||
| /// This matches the logic used in FreeType when neither of the | ||
| /// `FT_LOAD_FORCE_AUTOHINT` or `FT_LOAD_NO_AUTOHINT` flags are specified. | ||
| pub fn prefer_interpreter(&self) -> bool { |
There was a problem hiding this comment.
Perhaps explain in this comment from where this is used and how it relates to the Engine configuration option?
- Engine::AutoFallback docs: mention the relevant load flags wrt to FreeType's behavior; be more specific about PostScript/TrueType specific selection - OutlineGlyphCollection::prefer_interpreter() docs: explain how this relates to HintingOptions and Engine::AutoFallback - rework Scale constructor to take Style enum for determining whether the font is italic rather than using a boolean
Co-authored-by: Dominik Röttsches <[email protected]>
| @@ -28,6 +198,7 @@ use crate::alloc::{boxed::Box, vec::Vec}; | |||
| /// FreeType 2.7. | |||
| /// | |||
| /// The default value of this type is equivalent to `FT_LOAD_TARGET_NORMAL`. | |||
| #[doc(hidden)] | |||
There was a problem hiding this comment.
Yes, but we require transitional states for rolling updates in Chrome and I specifically avoided the deprecated attribute to avoid raising warnings at compile time.
There was a problem hiding this comment.
Thanks for the explanation. I wonder if whatever uses this in chrome should use allow(deprecated) instead. I think that should be entirely equivalent for that use case.
I suppose that could cause problems if a non-skrifa library were updated?
There was a problem hiding this comment.
Yeah, it seems like that could affect other things in undesirable ways. My plan was to mark these deprecated for the next cycle and then remove them which adds a bit of latency but should be fine.
There was a problem hiding this comment.
I've done some more digging, and this is exactly the use case for #[deprecated(since = "TBD")], which will only trigger the deprecated_in_future lint. I can't find exactly when this was stabilised, but experimentally, it has worked since rustc 1.75.
Replaces the current
HintingModewith a newHintingOptionstype to support autohinting. Specifically, it allows for selecting the hinting engine (interpreter, auto or auto fallback). Also reintroduces a "light" mode to matchFT_RENDER_MODE_LIGHTsince this actually represents a different mode in the autohinter (but didn't for TrueType or CFF).Adds the
symmetric_renderingflag as well (closes #1080).Based on #1086 since it depends on the new public
GlyphStylestype.@drott this is the same as the reference code in the preview branch