Excise pixman, use z2d for sprite fonts, refactor box drawing, add new box characters #2439
Conversation
…or regression test Use a single unified function for intersection-style line drawing chars, and one for fractional block characters. Add a ground truth image based on this commit for the regression test (differences from before changes validated visually, 1 incorrect rendering actually fixed)
Move away from C-style bit sets, calculate sextants procedurally rather than hard coding.
More complete coverage of the Symbols For Legacy Computing block, including characters from Unicode 16.0. Pixman and the web canvas impl for Canvas have been removed in favor of z2d for drawing, since it has a nicer API with more powerful methods, and is in Zig with no specific platform optimizations so should compile to wasm no problem.
+ un-`comptime` the line spec and make it a packed struct, to reduce codegen size.
|
Alright, one issue that has been pointed out is that the rounded corner glyphs don't align with the line characters at odd widths. This would've been caught if the regression test also rendered each glyph at an odd size to check for parity/rounding differences. I'll update the regression test and fix the rounded corner glyphs. |
Smaller, odd cell size, odd (1) thickness, catches more edge cases with rounding etc.
+ update Box.ppm ground truth image
|
(I've updated the comparison images in the PR description) |
|
Looking good now! |
src/font/sprite/canvas.zig
Outdated
| const x0: usize = @intFromFloat(v.x); | ||
| const x1: usize = @intFromFloat(v.x + v.width); | ||
| const y0: usize = @intFromFloat(v.y); | ||
| const y1: usize = @intFromFloat(v.y + v.height); |
There was a problem hiding this comment.
We've hit a lot of subtle bugs in the past when we relied on "implicit" rounding with @intFromFloat. Should there be an explicit round/truncate operation here?
| const y0: usize = @intFromFloat(v.y); | ||
| const y1: usize = @intFromFloat(v.y + v.height); | ||
|
|
||
| for (y0..y1) |y| { |
There was a problem hiding this comment.
I think safety checks catch this but we should probably add assertions to be explicit that y0 <= y1 and same for x0.
There was a problem hiding this comment.
You make a good point here that there could be weird cases where width or height are negative somehow. It made me realize also that this function, which can only plot discrete pixels, should really not be able to take floats. I'll make the geometry primitives generic and use a Rect(u32) for this.
src/font/sprite/canvas.zig
Outdated
| x: i32, | ||
| y: i32, | ||
| x: f64, | ||
| y: f64, |
There was a problem hiding this comment.
Why does z2d operate in floating point when pixels are discrete? Or is there some sort of scaling happening somewhere I don't see?
There was a problem hiding this comment.
All path/vector API in z2d operates in floating point and are rasterized to their discrete values based on the polygon ultimately rendered based on the points in the path. This is pretty par for the course with vector graphics APIs (e.g., Cairo or tiny-skia as other examples).
As an aside, I'm tracking longer-term feedback for unit numerics in vancluever/z2d#26 and if you have any opinions I'd love to hear them 🙂
There was a problem hiding this comment.
Ah yes, that makes sense. Thanks!
|
Overall looks very excellent. I think my feedback is very simple, just some questions. Thanks! |
- Make canvas geometry primitives generic, use `Rect(u32)` for `rect` function, so that we don't have to worry about negatives or rounding. - Make `Quads` struct packed just in case it gets non-comptime use in the future. - Clarify comment on why we're discarding out of range pixels + runtime unreachable for any other type of error which we shouldn't ever see. - Move z2d import above in-tree imports.
| error.InvalidHeight, | ||
| error.InvalidWidth, | ||
| error.InvalidPixelFormat, | ||
| => { |
There was a problem hiding this comment.
I made these explicit instead of else so we can know if @vancluever or anyone else adds any new errors.
This PR entails a pretty substantial change in the sprite font code. In order to refactor multiple parts of the box drawing code without fear of regressions or errors I added a regression test that draws all of the box drawing characters to an atlas and compares it to a ground truth. If there's a difference, the atlas and the diff from the ground truth are automatically dumped as ppm files.
I added support for a decent handful of additional characters from the Symbols For Legacy Computing block, including new ones added in Unicode 16.0; in a follow-up PR I plan to add support for the Symbols For Legacy Computing Supplement block as well as a handful of semigraphics characters that live in other blocks such as Miscellaneous Technical.
I refactored multiple parts of the code to programmatically generate characters of a certain type based on descriptive structs, rather than having individual functions for each and every character which is just a lot of code to have and is very error prone and can make it difficult to make sweeping changes to an entire group of characters.
You'll notice in the diff (below) that my refactor actually fixes an improperly rendered box drawing character, specifically
U+2537 ┷ BOX DRAWINGS UP LIGHT AND HORIZONTAL HEAVY, which previously was erroneously rendered with a light horizontal.The addition of z2d allowed me to change the way diagonal lines are drawn such that they are the correct thickness (previously they were thinned by their slope), and use a more pleasant curve for the rounded corner box drawing characters (providing a smoother transition between straight line and corner, reducing the 2nd order discontinuity).
The removal of the pixman dependency and refactored box drawing yields a smaller binary size (
ReleaseFastwent from19.1kbto18.7kbfor me).I fully expect feedback that I need to address on this PR in terms of style and the like. I know I definitely did some questionable stuff in some parts but I'm blind to my own mistakes and honestly just don't have the mental capacity to do a self code review on this code rn, but I wanna get this PR in so I'm making it now.
Comparison