Add font embolden #823
Add font embolden #823
Conversation
f317cd7 to
d4eddc4
Compare
|
Need to investigate:
|
3e7d17e to
420d99d
Compare
|
This is now working. It has path expansion/stem darkening working on the GPU and properly works with Bevel, Miter, and Round joins. These are all configurable. The changes are implemented in both the cpu and gpu flatten shader. |
|
A quick update, this was discussed in Renderer office hours, also see transcript of the discussion. This would be great to have in some form, but the big question is review bandwidth as it's a fairly major change and has a risk of breaking things. We haven't forgotten about it, but need to decide what to do. |
420d99d to
fa91a62
Compare
|
I have updated this PR to allow the embolden to take a vec2 for separate emboldening of x and y. I will add some comments to the PR to highlight parts that need particular review and open questions. |
switch to gpu embolden
fa91a62 to
4131589
Compare
bf15178 to
dba1143
Compare
raphlinus
left a comment
There was a problem hiding this comment.
Generally this looks good and I'm inclined to approve. I haven't gone through with a fine toothed comb.
One question I have. I'm not understanding the motivation for adding an additional winding stream when it's one bit of information per path. It seems like you could add a bit to the flags u16 in Style. Was there a reason you didn't do this?
Another possibility to consider is negating the embolden vec, so it's a signed offset to apply to the path.
I'm not yet convinced the logic for separate x and y emboldening is correct; if I'm reading the code correctly, what you have is basically a variable width that's linearly interpolated from the "projected" boldening at the edges. This will incorrectly compute the cusp in extreme cases, which of course we don't expect for font rendering. The more principled way to do it would be to nonuniform scale the path, apply the stroke offset, then inverse nonuniform scale. That's very similar to the way stroke expansion works in the presence of an eccentric affine transformation (in this case, there's no pre-transform, just the application of a post-transform). However, I suspect that the difference between what you have and the "fully correct" version is subtle and is probably not worth additional complexity/rework, so I'm just mentioning it.
Overall, very cool, and deepest apologies for the delay in reviewing it.
| encoder: PathEncoder<'a>, | ||
| path_windings: &'a mut Vec<f32>, | ||
| /// Collects points from all subpaths for a single winding calculation | ||
| all_glyph_points: Vec<Point>, |
There was a problem hiding this comment.
It's not obvious to me why we're storing the points rather than accumulating the area on the fly.
| } | ||
| } | ||
|
|
||
| fn compute_winding(points: &[Point]) -> u8 { |
There was a problem hiding this comment.
Unless I'm missing something, this has the possibility of mis-computing area, as I believe it includes a component from the last point of a subpath to the moveto opening the next subpath, and this should not be included. I think this logic would be fairly straightforward to accumulate on the fly.
| var robust = ESPC_ROBUST_NORMAL; | ||
| if abs(k1) < K1_THRESH { | ||
| let k = es.params.k0; | ||
| let k = k0 + 0.5 * k1; |
There was a problem hiding this comment.
Reason for this change? They should be mathematically identical, and the old version is a touch less work.
Ok I'll fix the items from the feedback and take a look at this and report back.
❤️ |
This is basically just a direct port of the swash implementation of font embolden.It does seem to have about twice the "strength" of the swash implementation though and I haven't been able to figure out why.