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

fix maximum_color'ing 0-width glyphs #424

Merged
merged 7 commits into from
Aug 18, 2022
Merged

Conversation

anthrotype
Copy link
Member

trying to reproduce issues converting COLRv1 font to OT-SVG when the former contains glyphs with 0 advance width (#421)

trying to reproduce issues converting COLRv1 font to OT-SVG when the former contains glyphs with 0 advance width
that's how it is.. We could alternatively do glyph.recalcBounds(ttfont['glyf']) and the empty glyph would gain xMin, yMin, etc. all set to 0 anyway.
@anthrotype
Copy link
Member Author

oh right, empty glyf glyphs (numContours == 0) have no attribute xMax in fonttools. Currently glyph_region expects xMax to be there when a glyph's width == 0 (so that it can make up some advance width to prevent bitmaps disappear..)

https://github.com/googlefonts/nanoemoji/runs/7863175783?check_suite_focus=true#step:5:210

glyph.recalcBounds(glyfTable) would set bbox attributes to 0 for empty glyphs anyway, so we can simply assume if not hasattr(glyph, "xMax") then xMax is going to be 0 (provided that glyf glyph has been "expanded" [fully decompiled], which is the case as soon as you __getitem__ it from glyf).

why was this test not failing before? I guess because we don't keep dependencies pinned for our testing and something happened upstream since the last time we run our tests..
@anthrotype
Copy link
Member Author

ok, i'm now hitting the first assert width > 0

  File "/home/runner/work/nanoemoji/nanoemoji/.tox/py/lib/python3.7/site-packages/nanoemoji/generate_svgs_from_colr.py", line 41, in _view_box
    assert region.w > 0, f"0-width region for {glyph_name}"

https://github.com/googlefonts/nanoemoji/runs/7863539114?check_suite_focus=true#step:5:207

@anthrotype
Copy link
Member Author

anthrotype commented Aug 16, 2022

I do wonder if the 0 width check shouldn't still exist somewhere for bitmaps only.

I am not sure where to place that check though.. On the one hand, when going COLR=>SVG, I want to be able in generate_svgs_from_colr.py to write svg files that have viewbox.w=0, so that they get placed correctly in font space and there's no viewbox clipping for vector color formats; on the other hand, when coing COLR=>SVG=CBDT I want to prevent resvg from outputting empty PNGs when the viewbox.w = 0 (and use the current hack that takes as width the glyph.xMax), but then I can't reuse the same svgs generated from COLR as we currently do, but i'd have to generate a different sets of svgs where viewbox widths have not been zeroed. But even so, the xMax we take comes from the fallback b/w glyf glyph, which nanoemoji itself keeps empty when building COLRv1 from .svg input.. and empty glyphs implicitly have xMax=0 so we're back to invisible PNGs :,(

I think we should remove all the hacks that tamper with the glyphs advance width and that assert it's > 0 and live with the fact that 0-width bitmap glyphs will disappear until #402 is properly dealt with

@rsheeter
Copy link
Collaborator

I think we should remove all the hacks that tamper with the glyphs advance width and that assert it's > 0 and live with the fact that 0-width bitmap glyphs will disappear until #402 is properly dealt with

SGTM, it's moving us forward

this will make zero-width glyphs disappear when rasterized via resvg to PNG, because anything outside viewbox gets clipped (#402). Thus, running maximum_color --bitmaps with a font that has such zero-width glyphs will produce invisible empty CBDT/sbix glyphs until that issue is resolved.
However, removing this assertions allows us to support zero-width glyphs in vector color formats like OT-SVG/COLR (cf. #421)
this messes up horizontal positioning of glyphs with zero advance width, was added to prevent clipping of bitmap glyphs exceeding their viewbox (#402). The latter issue will be dealt with separately.
@anthrotype anthrotype marked this pull request as ready for review August 18, 2022 09:45
@anthrotype
Copy link
Member Author

@rsheeter I think this is ready now

@anthrotype anthrotype merged commit bf36fa0 into main Aug 18, 2022
@anthrotype anthrotype deleted the zero-width-colr-to-svg branch August 18, 2022 16:03
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