fix(compiler): close open contours before filling glyph outlines - #271
Draft
qianiaoo wants to merge 1 commit into
Draft
fix(compiler): close open contours before filling glyph outlines#271qianiaoo wants to merge 1 commit into
qianiaoo wants to merge 1 commit into
Conversation
`flatten()` pushes each contour to the fill exactly as opentype.js reports it, and opentype.js emits no `Z` command for either outline format. For TrueType that is harmless: the contour already ends on its start point, so the closing edge is degenerate and the scanline fill sees a closed polygon. CFF closes a contour implicitly and leaves the command list open, so the edge from the last point back to the first is simply missing, the fill meets an unpaired crossing, and rows leak or drop. The effect on a CFF/OTF face is not subtle. Baking Noto Sans SC at 24 px, 151 of 218 glyphs come out different once contours are closed — strokes cut short, chunks of the glyph missing, fills bleeding sideways out of the outline. The fix is one compare per contour: if the last point is not the first point, append the first point. Measured against the faces this repository ships, so the blast radius is known: | face | glyphs baked | changed by this commit | | --- | ---: | ---: | | Noto Sans SC (CFF/OTF) | 218 | **151** | | HarmonyOS Sans (TrueType) | 76 | 0 | | Inter (TrueType, the default) | 76 | 0 | Inter bakes byte-identically, so no atlas this repository produces today changes, and no golden moves. The test builds the same square twice — once ending on its start point, once not — and asserts the two bake to identical bytes and that the bytes are a filled square rather than an empty cell. Without the fix both assertions fail, the open square baking to zero ink. `bun run test`: 385 pass. `tests/note.test.ts` fails in this environment because it needs a wasm build the toolchain here cannot produce (`can't find crate for core` for wasm32-unknown-unknown); it fails identically with this commit reverted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
flatten()inframework/compiler/bake-font.tshands each contour to the scanline fill exactly as opentype.js reports it, and opentype.js emits noZcommand for either outline format.For TrueType that has never mattered: the contour already ends on its start point, so the closing edge is degenerate and the fill sees a closed polygon. CFF closes a contour implicitly and leaves the command list open, so the edge from the last point back to the first is missing, the fill meets an unpaired crossing, and rows leak or drop.
So the baker works on every face this repository ships and silently mangles any OTF/CFF face — which is most CJK faces, and the reason I ran into it: baking Noto Sans SC produced glyphs with strokes cut short, chunks missing, and fills bleeding sideways out of the outline.
永at 24 px, before and after (coverage cell as ASCII):The fix
One compare per contour: if the last point is not the first point, append the first point. It costs nothing and makes the fill independent of which format the outline came from.
Blast radius, measured
Every face in the tree, baked at 24 px through both versions and compared byte for byte:
DEFAULT_REGULAR)Inter bakes byte-identically, so no atlas this repository produces today changes and no golden moves. The only outputs that move are the ones that were wrong.
The test
tests/font-bake.test.tsgains a case that needs no font asset: the same square described twice — once ending on its start point (TrueType's shape), once not (CFF's) — must bake to identical bytes, and those bytes must be a filled square rather than an empty cell.With this commit reverted both assertions fail, the open square baking to zero ink.
Checks
bun run test— 385 pass. One pre-existing failure in this environment:tests/note.test.tsneeds a wasm build that this machine's toolchain cannot produce (can't find crate for coreforwasm32-unknown-unknown). It fails identically with this commit reverted.Deliberately not included
While tracking this down I also looked at the fill rule itself: the crossings are paired off two at a time, which is even-odd, while both outline formats are drawn for non-zero winding. Changing it is a one-function patch, but on all three faces above it makes zero difference once contours are closed, so it is a spec-conformance change with no reproduction behind it and does not belong in a bug-fix PR. Happy to open it separately if you want it.