glifo: Fix interaction with complex paints#1668
Conversation
|
I will leave the actual review for someone else, but you should probably include at least one test using an image brush as well. |
|
Also, when I implemented brush transforms for glyphs for Vello Classic recently, I had some interesting tests that might be worth taking a peek at. |
|
Sure, can do that! |
| pub(crate) glyph_type: GlyphType<'a>, | ||
| /// The global transform of the glyph. | ||
| pub(crate) transform: Affine, | ||
| /// The transform of the paint, relative to [`PreparedGlyph::transform`]. |
There was a problem hiding this comment.
I think this is subtly wrong, since it’s actually relative to transform.pre_scale(scale).
| @@ -150,6 +151,8 @@ pub(crate) struct PreparedGlyph<'a> { | |||
| pub(crate) glyph_type: GlyphType<'a>, | |||
| /// The global transform of the glyph. | |||
| pub(crate) transform: Affine, | |||
There was a problem hiding this comment.
I think transform is a bit overloaded across this file. Would it be better to rename it to something like the following?
/// Per-glyph outline transform: maps the draw-unit glyph outline
/// (after font-size absorption) to scene coordinates.
pub(crate) outline_transform: Affine,| /// Global transform. | ||
| transform: Affine, | ||
| /// Absolute paint transform for the glyph run. | ||
| absolute_paint_transform: Affine, |
There was a problem hiding this comment.
Nit: "absolute" here doesn't refer to a world frame, just "the composition in scene space, before glifo's per-glyph rewrite". Renaming to scene_paint_transform would pair naturally with relative_paint_transform (which is in glyph-local space) and make the invariant readable: scene_paint_transform == outline_draw_transform * relative_paint_transform.
| let outline_draw_transform = outline_transform.pre_scale(scale_props.draw_scale); | ||
|
|
||
| // We assume that the backend calculates the absolute paint transform | ||
| // by concatenating scene transform and (relative) paint transform. | ||
| // (This is currently the case for Vello CPU / Vello Hybrid, but will | ||
| // also be assumed to be the case for any other potential backend.) | ||
| // Therefore, we can calculate the relative paint transform for | ||
| // the glyph by pre-concatenating it with the inverted outline transform. | ||
| let relative_paint_transform = | ||
| outline_draw_transform.inverse() * absolute_paint_transform; |
There was a problem hiding this comment.
When the outline cache hits (the common case for repeated characters), the code has already paid for an Affine::inverse() and Affine * Affine multiply that gets discarded. What about putting the computation into the outline-cache-miss branch, since that's the only place a relative_paint_transform produced here is consumed?
There was a problem hiding this comment.
I’m worried we’re introducing quite a lot of math here on the hot path, especially for a general solid fill, since this math isn’t even needed in that case, can this be avoided at least for solid colors? And more generally, are there ways to avoid doing this math per glyph? One idea would be to factor out the run-constant linear part so that we end up with significantly fewer computations overall, although that would increase the complexity here.
Another idea, which seems preferable, would be to push this responsibility into the renderer, perhaps by introducing set_paint_transform_absolute(t: Affine) on GlyphRenderer which would flip the responsibility: glifo calls it once per run with absolute_paint_transform, the renderer stores it as an optional Option<Affine> on its RenderState, and every paint-encoding site uses that override directly when set instead of composing transform · paint_transform. Glifo can then freely mutate state.transform per glyph without any compensation, because the paint basis is pinned. The per-glyph math drops to zero, the inverse disappears entirely.
3 participants
As part of our migration of glifo, we've basically completely broken the interaction between glyphs and complex paint transforms. This PR makes two changes to restore the expected behavior:
I added some more new tests to prevent future regressions. Thanks to @waywardmonkeys for finding this.