Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve ab-glyph rendering use outline px_bounds to inform pixmap size #65

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

alexheretic
Copy link
Contributor

@alexheretic alexheretic commented Jul 3, 2024

I saw there were some recent issues with oob on the pixmap when rendering using ab_glyph.

The root cause is the use of "layout" positions (font.height(), h_advance etc) to inform the actual rendering pixmap size. The sizes for laying out glyphs are not always the same as the bounds required for actually rendering the glyphs. For example, a glyph at x=0 may have an outline/render that actually includes some pixels to the left of that layout origin.

ab-glyph provides OutlinedGlyph::px_bounds which accounts for this and will always have enough space to render each pixel returned by OutlinedGlyph::draw correctly. Continuing the example where the glyph (at x=0) outline reaches out to the left of the origin, the px_bounds.min.x of that glyph would be negative.

So outlining the glyphs and using px_bounds gives the correct pixmap size. It is then important to convert the px_bounds into "pixmap space" when calculating the pixel index, since px_bounds.min.x may be non-zero. I added comments to explain this in the code.

After all that we should end up with smaller pixmaps than before and p_idx should never go oob. (The pixmaps could be even smaller actually, I padding the vertical size with the full "ascent" of the font just to make positioning of the pixmap consistent e.g. so "Tg" & "gg" would have the same height pixmap).

I replaced the log with a debug_assert, since it "should never happen" but if a bug is still there or is introduced we'd probably prefer release code to still not panic.

Please feel free to ping me directly if you have problems/questions about ab_glyph code in future.

@PolyMeilex
Copy link
Owner

That's awesome, thanks!

Please feel free to ping me directly if you have problems/questions about ab_glyph code in future.

Cool, I appreciate that, I never took the time to learn ab-glyph properly, so that will be helpful for sure.

@PolyMeilex PolyMeilex merged commit c7a13f9 into PolyMeilex:master Jul 4, 2024
7 checks passed
@alexheretic alexheretic deleted the better-ab-glyph-rendering branch July 4, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants