Repository navigation
feat(text): keep MultiFace font fallback on GPU - #513
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
kolkov
left a comment
There was a problem hiding this comment.
@besmpl, same apology here for the long silence, for the same reason.
Nine days after you opened this, I described the fix in #521 without linking your PR, which already implements it. That was a miss on my side. The direction matches what we planned: split by source face before shaping, keep the owning face on each glyph, and send single-font runs to the glyph-mask and MSDF paths. That replaces our own plan to build it.
I've read the diff, but I haven't run it yet. Its CI ran against the August main, and that run can't be restarted.
Needed before merge:
- A rebase onto current
main, so CI runs on the code this would merge into.text/andinternal/gpu/changed since August, and more lands with the next release. If you'd prefer to rebase once after that release, say so and I'll ping you when it's onmain. - This adds public API:
FontRun,MultiFace.Faces/FaceForRune/FontRuns/ShapeRuns,FilteredFace.FontRuns,text.ShapeRunsandShapedGlyph.Face. We're freezing the API for v1.0, so each exported name has to be one we'd keep forever. Two questions:- Does
FontRun.IsCJKneed to be exported? It's a policy flag for our atlas and hinting choice. An unexported field, or a script value rather than a bool, would age better. - Do we need both
MultiFace.ShapeRunsandtext.ShapeRuns? The function form already dispatches toFontRunswhen the face has no single source, so it covers the method's case.
- Does
Optional: ShapedGlyph gains an interface field, 16 bytes per glyph. If you have numbers for a long paragraph through scene before and after, they'd help. I'll measure it myself otherwise.
gogpu#513 added both MultiFace.ShapeRuns and text.ShapeRuns. The function already sends a face without a single source to its FontRuns, so it covers MultiFace; the method had no caller outside its tests. Public API is frozen at v1.0, so only the function stays. MultiFace.FontRuns now accepts a nil receiver, which the method used to guard, and the helper only the method used is gone. CHANGELOG entry for gogpu#513 credits besmpl. Refs gogpu#513
61d4d5e to
a21515c
Compare
|
@besmpl, same here: I've done the changes on your branch, your authorship kept. Your three commits are rebased onto On the two API questions:
Cost of The CHANGELOG credits you. Thanks for building this. |
gogpu#513 added both MultiFace.ShapeRuns and text.ShapeRuns. The function already sends a face without a single source to its FontRuns, so it covers MultiFace; the method had no caller outside its tests. Public API is frozen at v1.0, so only the function stays. MultiFace.FontRuns now accepts a nil receiver, which the method used to guard, and the helper only the method used is gone. CHANGELOG entry for gogpu#513 credits besmpl. Refs gogpu#513
a21515c to
4a8217a
Compare
kolkov
left a comment
There was a problem hiding this comment.
Rebased onto current main, CI green, merging for v0.52.7. Your commits stay yours; the one on top drops the duplicate ShapeRuns and the helper only it used.
Source-aware GPU font fallback (#451)
MultiFacenow exposes its fallback chain and contiguous source-font runs, and shaping retains the owning face on everyShapedGlyph. The glyph-mask and MSDF GPU paths rasterize those runs into ordered batches instead of returning a nil-source error; shaped CPU fallback also resolves each glyph's source. Mixed-font tests cover deterministic run offsets, source identity, glyph-mask batching, and MSDF batching.Fixes #521.
Verification:
go test ./...,go test -tags nogpu ./...,go vet ./....