Refactor font rendering pipeline, remove font-kit and pathfinder#736
Refactor font rendering pipeline, remove font-kit and pathfinder#736pbdeuchler wants to merge 23 commits intoplotters-rs:masterfrom
font-kit and pathfinder#736Conversation
…rifa/zeno The legacy native text path carried two parallel backends -- font-kit + pathfinder behind the `ttf` feature, ab_glyph behind the `ab_glyph` feature -- selected by a cfg cascade in style/font/mod.rs and resolved fonts eagerly at FontDesc construction. That coupling makes it hard to scope fonts to a chart, hard to ship in-memory fonts without a process-global registry, and hard to evolve the rasterizer without churning the FontDesc surface. Replace the native pipeline with a single linebender stack: fontique for selection, harfrust for shaping, skrifa for outlines, zeno for rasterization. The wasm text path is untouched because the browser still owns text rendering there. Hide the new stack behind a small two-trait abstraction (FontEngine + ParsedFont) in style/font/engine.rs so a future swap to swash or another shaper is a single-file change. The harfrust+skrifa+zeno crates only appear in style/font/harfrust_engine.rs. Move font resolution off FontDesc into a FontContext that lives per-DrawingArea, with a thread-local stack pushed and popped via an RAII guard around element draws so backends keep BackendTextStyle::draw and layout_box unchanged. Resolution becomes lazy: nothing happens until layout_box or draw runs against the active context. A process-global parsed-font intern keyed by (bytes-hash, len, index) deduplicates parse work across contexts. Add DrawingArea::with_fonts and FontContextBuilder so callers can attach in-memory fonts to a chart and optionally disable system lookup, while the legacy register_font surface is preserved as a compatibility shim under the ab_glyph feature. Ship a small OFL-licensed TTF fixture so the new layer can be tested without depending on host-installed fonts. Cover engine roundtrip, explicit-context resolution, the global parse intern, and TLS guard cleanup under panic.
The unit tests in style/font/* exercise the engine and context internals, but they do not prove the new pipeline survives the path the rest of the crate actually uses: DrawingArea -> backend -> BackendTextStyle::draw -> FontContext. Only an integration suite catches breakage in the TLS push/pop, sub-area context inheritance, or the legacy register_font interop. Add tests/font_migration.rs driving BitMapBackend with the bundled TTF fixture so the suite is host-font-independent. Cover with_fonts rendering, explicit-context isolation from a default DrawingArea, sub-area context inheritance and override behavior, and concurrent text drawing across threads with separate contexts. Under the ab_glyph compatibility feature, additionally verify that register_font called after DrawingArea construction still reaches the default context, that an explicit context can opt into the legacy registry via include_registered, and that with_fonts contexts stay isolated from the registry. These cases lock down the precedence rules called out in docs/font-pipeline-migration.md so the legacy contract cannot regress silently.
Now that style/font/mod.rs always routes native text through FontContext + the harfrust/skrifa/zeno engine, the parallel ttf, ab_glyph, and naive backend modules are unreachable. Leaving them in tree pulls font-kit, pathfinder_geometry, ttf-parser, lazy_static, and the ab_glyph crate into every native build for no runtime benefit, and slows compilation by keeping the cfg cascade in mod.rs alive. Delete the dead modules and remove the now-unused dependencies from plotters/Cargo.toml. The `ttf`, `system_fonts`, and `fontconfig-dlopen` features go away with them; `ab_glyph` is kept as an empty feature shim that gates register_font and the include_registered builder option. Re-export FontContext and FontContextBuilder from the prelude so the new per-area font configuration API is reachable through the same import as the rest of the crate. Refresh README.md, the readme template, and the crate-level rustdoc to drop the old ttf/font-kit/rusttype language and describe ab_glyph as a register_font compatibility shim rather than a separate rasterizer. Update the dev-dependencies in plotters-bitmap/Cargo.toml and plotters-svg/Cargo.toml that previously selected the deleted `ttf` feature. Tighten parse_cached so two threads racing the same uncached fingerprint converge on a single interned parsed instance instead of one of them inserting on top of the other's Weak.
When the legacy native backends were removed in the previous commit, register_font's return type InvalidFont stopped being publicly nameable -- the type is still defined in style/font/migration.rs but the path to it through style/font/mod.rs and style/mod.rs only carried `register_font`, not `InvalidFont`. Downstream callers that destructure or annotate the error (`.unwrap_or_else(|e: InvalidFont| ...)`, `Result<(), InvalidFont>`) would fail to compile against the migrated crate even though the function itself is still exported. Re-export InvalidFont in the same cfg-gated `pub use` block as register_font so the legacy public API surface is fully usable for source-compatible upgrades. The type itself is unchanged; this is purely a path fix.
… regressed tests, code quality nits, rework caching so that register_font always takes effect at slight performance penalty
|
@philocalyst let me know your thoughts on this one... completely open to feedback, i'm not super attached to how I implemented things here but I think it's the least worst option. I might still refine this a bit and have some more manual testing to do but let me know your initial thoughts. |
The new fontique-backed font pipeline links against fontconfig on Linux, so every CI job that compiles or runs plotters needs libfontconfig-dev available before the toolchain step. Without it, fontique fails to build the system fonts source on Ubuntu runners. Add the apt-get install step to plotters-backend.yml, plotters-bitmap.yml, plotters-core.yml, plotters-svg.yml, rust-clippy.yml, and wasm.yml. Gate it behind 'if: runner.os == "Linux"' on the multi-OS matrix jobs so Windows and macOS runners skip it. In plotters-core.yml, also bump the MSRV job from 1.56.0 to 1.88.0 (the new font crates require it) and normalize indentation across the file so the YAML structure is consistent. Carried over from pbd/remove-pathfinder-geometry, where these workflow tweaks were authored alongside an alternate fontique migration.
|
Hmmmm, I'll take a look at those CI failures in a bit. I think I know what's happening. |
After with_font_context was removed, FontContext::builder() built an Arc<FontContext> that had no public consumer -- there was no way to attach it to a DrawingArea, and the methods on FontContext (layout_box, draw, current_or_default) were already pub(crate). The type and builder were public dead weight that suggested a richer API than actually existed. Lower FontContext, FontContextBuilder, system_default(), builder(), with_font(), include_registered(), and build() to pub(crate). Drop the public re-exports from style/font/mod.rs, style/mod.rs, and the prelude in lib.rs. Gate disable_system_fonts behind #[cfg(test)] since it's only used by unit tests now. Add plotters/examples/dynamic_font.rs to demonstrate the remaining public path: download Roboto from the Google Fonts css2 endpoint at runtime, parse the @font-face blocks for the Regular and Bold .ttf URLs, fetch the bytes, and attach them to a chart via DrawingArea::with_fonts. The example uses a Wget user-agent to coax Google Fonts into serving raw TrueType -- modern user-agents receive WOFF2, which harfrust and skrifa cannot read directly. Add ureq as a non-wasm dev-dependency for the example's HTTP fetches. Update README.md, doc-template/readme.template.md, and the crate rustdoc preamble to drop the FontContext::builder() reference and link to the new example.
zeno does honest analytical anti-aliasing, but on its own it produces noticeably chunkier text than the previous font-kit path. Two structural reasons combined: 1. We passed `DrawSettings::unhinted` to skrifa, so glyph stems landed wherever the outline put them in floating-point pixel space. font-kit on master had the same flag but its underlying rasterizers (FreeType on Linux, Core Graphics on macOS) apply auto-hinting and stem-darkening regardless. We were missing both. 2. `context::draw` rounded each glyph's advance to integer pixel coordinates before placing the rasterized mask. harfrust's sub-pixel kerning was computed and then thrown away, so spacing collapsed to whole-pixel grid steps. Add a `HintingInstance` per (parsed font, size) inside `HarfrustFont`, cached behind a `Mutex<HashMap<u32, Arc<HintingInstance>>>`. `HintingInstance::new` traces the font's bytecode interpreter and is far too expensive to run per glyph; the cache is keyed by `size_px.to_bits()` so the typical chart hits it after the first label. Switch `glyph.draw` to `DrawSettings::hinted(&instance, false)` with the default `SmoothMode::Normal` target -- the same mode FreeType's smooth-rendering path uses, which is what the old pipeline effectively had. Extend the `ParsedFont::rasterize` trait signature to accept a sub-pixel offset `(sx, sy)` in `[0, 1)`. The HarfrustFont implementation folds the offset into the path coordinates inside `ZenoPen` (so zeno's analytical AA accounts for the fractional position rather than rounding it away), and then `Mask::new(&path).render()` returns placement data that's already integer-aligned for the caller. In `context::draw`, split each glyph's `f32` position into an integer pixel and a quantized sub-pixel index via a new `split_subpixel` helper. Quantize to 4 levels per axis (FreeType's default, the perceptual sweet spot) and route the quanta through `rasterize_cached`. The cache key gains `sx_quantum` and `sy_quantum`, growing the key space by ~16x; in practice the hot working set is still tiny (chart text reuses a small glyph repertoire). Cover the new behavior with two unit tests: that a half-pixel horizontal offset measurably changes the rasterized mask, and that `split_subpixel` rounds correctly across pixel boundaries including negative coordinates. Visually, this brings small text (10-14pt chart labels) much closer to the master branch's appearance -- stems are crisp, kerning preserves sub-pixel spacing, and the y-axis description label stops looking pixely.
…pulled in a later version of the image crate
| end-to-end example that downloads Roboto from Google Fonts at runtime and | ||
| attaches it to a chart through the new pipeline. | ||
|
|
||
| | Name | Description | Additional Dependency | Default? | |
There was a problem hiding this comment.
ab_glyph doesn't add the ab_glyph dep?
There was a problem hiding this comment.
Not anymore, it all goes through the same pipeline. ab_glyph just turns off system font discovery now.
| ?family=Kablammo\ | ||
| &family=Roboto:ital,wght@0,100..900;1,100..900\ | ||
| &display=swap"; | ||
|
|
There was a problem hiding this comment.
Would rather just host the fonts in-tree, to reduce this logic and avoid the issues when polling against an external service.
| fn rasterize( | ||
| &self, | ||
| glyph_id: u32, | ||
| size_px: f32, |
There was a problem hiding this comment.
What is size_px for? Feel like the name is washy
There was a problem hiding this comment.
Pixel size for the font, for calculating scale and passing to the harfrust shaper
There was a problem hiding this comment.
Non-blocking but I was hinting that it could be renamed for clarity -- font_size_px maybe?
| sy: subpixel.1, | ||
| }, | ||
| ) | ||
| .map(|_| ()) |
There was a problem hiding this comment.
Can remove the map (_) call and use ? at the end of the map_err
There was a problem hiding this comment.
Compiler complains about this, can't infer the types
| .map_err(|_| FontError::InvalidFontIndex(self.index)) | ||
| } | ||
|
|
||
| fn hinter_for(&self, size_px: f32) -> Result<Option<Arc<HintingInstance>>, FontError> { |
There was a problem hiding this comment.
In general find the result<option<>> pattern needless -- anyway we could compress to just a Result<>? Would appreciate a comment inline if you do feel it is needed.
There was a problem hiding this comment.
I flattened this to an Option<> instead since we want to throw away the error anyway and draw an unhinted glyph if this fails. Happy to make it a Result with an error log or something if we want though
|
Do you plan to fix the remaining CI errors? The tests and error variants feel solid to me, just some readability and semantic issues core to the logic that I would prefer to be untangled. I'd also like to mark somewhere that when Glifio stabilizes, to switch off of That should solidify this stack for a while yet. |
|
Not a blocker for a merge, but if you'd like bonus points, pioneering screenshot integration testing here would be a great way to see how (if at all) this changes text outputs, and ensure future changes don't ruin this. |
|
@philocalyst I appreciate the review! working on fixing those CI issues and will address these comments as well |
|
I do want to say that right now i'm not super happy with the kerning and rendering on this branch. Here's some screenshots for comparison, let me know your thoughts:
|
|
|
Yeah that's a little annoying -- that's something handled by Harfrust as well :( |
Co-authored-by: Miles Wirht <114884788+philocalyst@users.noreply.github.com>
Co-authored-by: Miles Wirht <114884788+philocalyst@users.noreply.github.com>
… font-kit. minor compilation/test fixes. fix up features so that we don't pull in ab_glyph in tests
@philocalyst I believe I addressed everything, ready for re-review |
`SeriesLabelStyle::draw` calls `MultiLineText::estimate_dimension` and `compute_line_layout` directly rather than through `backend_ops`, so fonts registered via `DrawingArea::with_fonts` were invisible to those layout passes and `--all-features` builds failed to resolve their own explicit fonts. Mirror what `backend_ops` already does and push the plotting area's font context onto the thread-local stack for the duration of the legend draw.
Builds a dark-background line plot that exercises a wider slice of the chart API in a single image: dashed extrapolation, dashed/dotted vertical reference lines, per-point value labels, an in-plot summary panel, and a styled `configure_series_labels` legend. Uses bundled Roboto via `with_fonts(...)` so the example works without depending on the host's installed fonts.
| fn rasterize( | ||
| &self, | ||
| glyph_id: u32, | ||
| size_px: f32, |
There was a problem hiding this comment.
Non-blocking but I was hinting that it could be renamed for clarity -- font_size_px maybe?
| HarfrustEngine | ||
| .parse(data.clone(), 0) | ||
| .map_err(|_| InvalidFont { _priv: () })?; | ||
|
|
||
| REGISTERED_FONTS | ||
| .lock() | ||
| .map_err(|_| InvalidFont { _priv: () })? | ||
| .push(registered_font(name, style, data)); |
There was a problem hiding this comment.
We're dropping the original errors here which feels unwise for something that interacts with foreign data, would prefer to be as helpful as possible (when) bad things happen.
| sx: f32, | ||
| sy: f32, |
There was a problem hiding this comment.
| sx: f32, | |
| sy: f32, | |
| subpixel_offset: Vector2F, |
|
|
||
| fn draw_unhinted_glyph( | ||
| glyph: &OutlineGlyph<'_>, | ||
| size_px: f32, |
| fn draw_unhinted_glyph( | ||
| glyph: &OutlineGlyph<'_>, | ||
| size_px: f32, | ||
| subpixel: (f32, f32), |
There was a problem hiding this comment.
Would like to mirror the x/y logic from below
| pub enum FontError { | ||
| /// The font bytes could not be parsed. | ||
| InvalidFontData(String), | ||
| /// The requested font collection index does not exist. | ||
| InvalidFontIndex(u32), | ||
| /// The requested font family and style are not available in the active context. | ||
| NotInContext { | ||
| /// The requested family name. | ||
| family: String, | ||
| /// The requested style name. | ||
| style: String, | ||
| }, | ||
| /// The request could only be satisfied by system fonts, but system lookup is disabled. | ||
| SystemFontsDisabled { | ||
| /// The requested family name. | ||
| family: String, | ||
| }, | ||
| /// A candidate font could not be loaded. | ||
| FontUnavailable { | ||
| /// The requested family name. | ||
| family: String, | ||
| /// The requested style name. | ||
| style: String, | ||
| }, | ||
| /// A glyph outline could not be converted into a coverage mask. | ||
| RasterizeError(String), | ||
| /// Internal font state could not be locked. | ||
| LockError, |
There was a problem hiding this comment.
lol once I started enumerating all of them I figured I might as well be exhaustive once I got 80% of the way there
| text: &str, | ||
| (base_x, base_y): (i32, i32), | ||
| mut draw: DrawFunc, | ||
| ) -> FontResult<Result<(), E>> { |
There was a problem hiding this comment.
Could we not simplify this to just FontResult or Result? This is another anti-pattern imo.
There was a problem hiding this comment.
wups sorry I think this is LLM slop that I didn't catch
| &self, | ||
| glyph_id: u32, | ||
| size_px: f32, | ||
| subpixel: (f32, f32), |
|
A lot cleaner this time around, feeling good about this PR! Have no complaints after these (small) issues are handled. You over-comment in some places but it's not a point to press. Just wish that some of that explaining was moved to the complex or external stuff (Like the Zeno impls with the heavily abbreviated params). Testing looks green and good. Does make me think we need some kind of reporting, the lack of a warn in some of the fallbacks could lead to some annoying debugging for users. Would love keep seeing contributions from you :) |
Co-authored-by: Miles Wirht <114884788+philocalyst@users.noreply.github.com>
|
Ha, you're not the first person to say that about my comments. TBH a big reason the more complex stuff isn't commented is because I still don't have a great grasp on what a lot of that is doing, it was mostly trial and error/copy pasting/Claude that got the rasterization and calculation bits working. Slowly figuring it out though. I found another edge case with series labels last night, so I created a complex example chart to help test things and fixed the bug in the least hacky way I could. I'm still not happy with the kerning and rasterization, but I figure once we can get this merged I can start to experiment with other feature gated engines that might emphasize font quality. I'm a bit busy today but will try and address your comments soon. Happy to help keep improving plotters, I use it extensively so there's a couple other things i'd like to fix if I can find the time. |
pbdeuchler
changed the title
font-kit and pathfinderfont-kit and pathfinder
Co-authored-by: Miles Wirht <114884788+philocalyst@users.noreply.github.com>
2 participants
Alternative PR to #735
Font rendering pipeline changes
Per discussion here, with the removal of
font-kitandpathfinderwe are now able to unify the default font rendering pipeline into a single code path. Users can now determine whether or not system fonts are used programmatically as part of the API instead of using build flags. The biggest change outside of the API surface is that font resolution and rasterization is deferred until the chart'sdrawcall actually runs.The new API creates a
FontContextstruct that is created and scoped perDrawingArea. ThisFontContextstruct gates what fonts are available fordrawcalls. Users may manipulate theFontContextby calling the newDrawingArea::with_fontsmethod. This call differs only slightly from the existingregister_fontcall in that it takes multiple fonts and relaxes thestaticrequirement on the font bytes by allowingArc<[u8]>. This obviously creates some allocations but I'm not sure that's a concern here. Happy to change that back or into something different if we'd prefer a different performance profile.Deprecation of
ab_glyphfeature/functionalityWith this change we can deprecate the
ab_glyphfeature andregister_fontfunctionality. The only thing theab_glyphfeature enables now is:register_fontsymbol and its global registry exist.FontContextis built withenable_system: false.I've done my best to integrate the existing behavior into the new pipeline however due to the way font caching works now
register_fontis much more stateful than before. There is a potential footgun when using bothregister_fontandwith_fontswhereregister_fontcan override the global state and unintentionally change a specificDrawingArea'sFontContext, however this behavior works in an obvious imperative way so I'm hoping it's not a big issue.In order to make
register_fontwork nicely with the new caching there are some performance changes where on every text render call we now:ab_glyph)Parsing is still amortized and even for programs where users are using hundreds of fonts with hundreds of charts there should be a negligible difference, however anything rendering dynamically changing charts at scale will definitely notice something.
Removal of
font-kitandpathfinderThis PR also removes
font-kitandpathfinderin favor ofharfrust,skrifa, andzeno. It also creates aFontEngineabstraction so this trio of dependencies can easily be swapped out in the future as things change (or toswashif we want). This is probably the most straightforward change.There is a potential regression since
register_fonttakes byte blobs where existing code that is compatible withab_glyphfails withharfrust. However given thatharfrustis a Google sanctioned port ofharfbuzz, the industry gold standard of font parsing, if there are regressions they are probably edge cases that shouldn't exist in the first place.We also bump MSRV to 1.88.0.