glifo: Add font embolden#1628
Conversation
|
I've also added support in vello classic |
|
@jrmoulton We want to make sure we're happy with this ... have some screenshots again? and then we'd do the publish of |
|
I'll get some screenshots posted |
|
I've exposed the join type, miter limit, and tolerance. |
| pub embolden_x_bits: u64, | ||
| /// Synthetic embolden amount. Only non-zero for outline glyphs. | ||
| pub embolden_y_bits: u64, | ||
| /// Join style for synthetic embolden. Only meaningful for outline glyphs. | ||
| pub embolden_join_bits: u8, | ||
| /// Miter limit for synthetic embolden. Only meaningful for outline glyphs. | ||
| pub embolden_miter_limit_bits: u64, | ||
| /// Tolerance for synthetic embolden. Only meaningful for outline glyphs. | ||
| pub embolden_tolerance_bits: u64, |
There was a problem hiding this comment.
Could we not cast to f32 first and then store as u32 instead of u64? This would help with keeping the memory footprint of the glyph cache key smaller.
| let packed = pack_color(BLACK); | ||
| let key1 = GlyphCacheKey::new(1, 0, 42, 16.0, true, 0.3, BLACK, packed, &[]); | ||
| let key2 = GlyphCacheKey::new(1, 0, 42, 16.0, true, 0.3, BLACK, packed, &[]); | ||
| let key1 = GlyphCacheKey::new( |
There was a problem hiding this comment.
I think this constructor is even more unwieldy now than it already was before now, would it make sense to group all emboldening-related keys into a custom struct?
There was a problem hiding this comment.
I tried it but it seemed to be an awkward fit to construct the a struct just as a grouping for the cache key. It might be better to remove the new function and directly construct the key struct. That would be less unwieldy and ambiguous but since the new function was already there I opted to use it.
There was a problem hiding this comment.
actually I think I can just move the handling into the new function. that should work fine
There was a problem hiding this comment.
Yeah I was overthinking that
| /// Set the synthetic embolden amount. | ||
| pub fn font_embolden(mut self, embolden: Diagonal2) -> Self { | ||
| self.run.font_embolden = embolden; | ||
| self | ||
| } | ||
|
|
||
| /// Set the join style for synthetic embolden. | ||
| pub fn font_embolden_join(mut self, join: Join) -> Self { | ||
| self.run.font_embolden_join = join; | ||
| self | ||
| } | ||
|
|
||
| /// Set the miter limit for synthetic embolden. | ||
| pub fn font_embolden_miter_limit(mut self, miter_limit: f64) -> Self { | ||
| self.run.font_embolden_miter_limit = miter_limit; | ||
| self | ||
| } | ||
|
|
||
| /// Set the tolerance for synthetic embolden. | ||
| pub fn font_embolden_tolerance(mut self, tolerance: f64) -> Self { | ||
| self.run.font_embolden_tolerance = tolerance; | ||
| self | ||
| } | ||
|
|
There was a problem hiding this comment.
Similarly, would it make sense to just create a FontEmboldenSettings struct and just a single setter for it?
There was a problem hiding this comment.
I don't think so. This already has a fluent api for setting different properties. The naming is a bit unfortunate but I don't think adding a new struct with probably another fluent api would be better.
There was a problem hiding this comment.
But I can do that if we decide to
There was a problem hiding this comment.
I think it would be my preference, but yeah let's see what other's say.
This already has a fluent api for setting different properties.
Yes but they are all different properties, while the font embolden ones all refer to the same feature basically. Just like we also have a single Stroke struct instead of methods like set_stroke_width, set_stroke_miter_limit, etc.
There was a problem hiding this comment.
If you're looking for another opinion, I think I prefer the separate struct version rather than inclining because it feels like how our other API is designed (collect into structs rather than inlining and flattening fields)
|
Also, I think it would be great to have one or two tests for this in |
taj-p
left a comment
There was a problem hiding this comment.
So good!!! Left some comments 🙏
Would be nice to have some tests as Laurenz suggested above
| outline.draw(draw_settings, &mut path).ok()?; | ||
| if path.finish(false) == 0 { | ||
| let n_path_segments = if self.embolden != Diagonal2::new(0.0, 0.0) { | ||
| let mut path = BezPathOutline(BezPath::new()); |
There was a problem hiding this comment.
Should we maintain a reusable BezPath on the session so that we don't pay this allocation for every glyph?
| outline_glyph.draw(draw_settings, &mut drawing_buf).unwrap(); | ||
| if embolden != Diagonal2::new(0.0, 0.0) { | ||
| drawing_buf.path = kurbo::expand_path( | ||
| &drawing_buf.path, |
There was a problem hiding this comment.
It's unfortunate that the new method doesn't have a version that doesn't allocate.
It looks like expand_path always allocates a new BezPath internally - so we're paying an allocation per glyph when zero is possible.
There was a problem hiding this comment.
I had a go of adding a re-useable context here linebender/kurbo#582
| /// Set the synthetic embolden amount. | ||
| pub fn font_embolden(mut self, embolden: Diagonal2) -> Self { | ||
| self.run.font_embolden = embolden; | ||
| self | ||
| } | ||
|
|
||
| /// Set the join style for synthetic embolden. | ||
| pub fn font_embolden_join(mut self, join: Join) -> Self { | ||
| self.run.font_embolden_join = join; | ||
| self | ||
| } | ||
|
|
||
| /// Set the miter limit for synthetic embolden. | ||
| pub fn font_embolden_miter_limit(mut self, miter_limit: f64) -> Self { | ||
| self.run.font_embolden_miter_limit = miter_limit; | ||
| self | ||
| } | ||
|
|
||
| /// Set the tolerance for synthetic embolden. | ||
| pub fn font_embolden_tolerance(mut self, tolerance: f64) -> Self { | ||
| self.run.font_embolden_tolerance = tolerance; | ||
| self | ||
| } | ||
|
|
There was a problem hiding this comment.
If you're looking for another opinion, I think I prefer the separate struct version rather than inclining because it feels like how our other API is designed (collect into structs rather than inlining and flattening fields)
|
I added a snapshot test |
|
@jrmoulton Git is complaining about this branch for me because the snapshot is not in git-lfs. I think you need to install Git LFS and then recommit the snapshot. |
|
This is looking decent to me: Left: Firefox on macOS. Right: Blitz on macOS with Vello Classic + this PR. Embolden amount is computed as: let embolden = kurbo::Vec2::new((0.015125 * font_size).min(0.3), (0.0121 * font_size).min(0.3));where
Unless I'm missing something, this PR doesn't seem to wire through embolden into the Blitz PR for anyone who wants to try this out: DioxusLabs/blitz#428 |
The glyph run builder is provided by glifo. You can look at the snapshot test that uses vello cpu.
I had thought it was logical pixels but when I was testing I found it to be device pixels. I could have been wrong though |
To be clear: I'm applying the formula to the unscaled font size, and applying the result as an embolden amount in physical pixels. |
|
@nicoburns I think I've fixed the git lfs issue |
nicoburns
left a comment
There was a problem hiding this comment.
@jrmoulton This needs the kurbo crates.io patch removing, and an explicit dependency on kurbo v0.13.1 in both vello_encoding and glifo but has otherwise been judged to be good to land.
|
CI issues seems to be Swatinem/rust-cache#341 |

This PR bumps kurbo to latest main although it is currently a patch since peniko depends on the released kurbo 0.13. I can open a PR to peniko if we need to bump kurbo there first.
This PR adds support for font embolden by getting the value through to the new kurbo
expand_path. The default embolden value is (0, 0). The chosen miter limit is 4.0 and the tolerance is 0.1. I'm not sure if these are the best values to use or if we should maybe make them configurable but these seem to be reasonable defaults.@nicoburns