googlefonts / nanoemoji

A wee tool to build color fonts.
Apache License 2.0
239 stars 19 forks source link

Fix hybrid vector+bitmap color font #384

Closed anthrotype closed 2 years ago

anthrotype commented 2 years ago

Fixes #382

Before this PR, the assumption in many parts of the codebase was that there were either vector-only or bitmap-only color formats; and for the former, the ColorGlyph.filename pointed to an .svg file, whereas for the latter, it pointed to a .png file. Then #370 introduced the notion of hybrid glyf_colr_1_and_picosvg_and_cbdt font that contains CBDT, SVG and COLR tables. However, the CBDT table was still reading the PNG bytes from the same ColorGlyph.filename, but the latter still pointed to an .svg file, hence it was invalid.

After this PR, ColorGlyph object gets an additional optional bitmap attribute containing the PNG bytes, in addition to the existing optional svg attribute containing a picosvg.SVG object. Both are optional, though at least one is required; for hybird vector+bitmap formats, both are not None.

~~The ColorGlyph.bitmap bytes are read in nanoemoji.write_font._inputs function, from the .png file at the same location as the input ColorGlyph.filename; i.e. for bitmap-only formats, this is the ColorGlyph.filename itself; whereas for hybrid format, this is a file with the same name but different *.png suffix in the same directory as the (picosvg) *.svg. This effectively becomes an inplicit dependency, and I added them to the ninja write_font build rule so that they are taken into account when building the hybrid font. Defining a different build subdirectory where to place the png files for the hybrid build, would have required that nanoemoji.write_font be changed to explicitly take the additional input PNG files, or alternatively some flag to translate from the .svg to the respective .png. All in all, my current apprach is simpler and keeps the changes to a minimum, so I went with that.~~

EDIT: After Rod's review, I reverted to adding both the svg and png filenames to the glyphmap.csv as first two columns, one of the two can be omitted (left empty). The default write_glyphmap script takes all the .svg and .png input files and matches them by their path stem (basename excluding the suffix) and places them on the same row, generating codepoints and glyph_name from their shared stem. write_font parses the glyphmap.csv to obtain the list of svg and/or png input filenames, loads one or both depending on whether the desired config's color format has_svgs or has_bitmaps or both, and assigns them to a ColorGlyph.

anthrotype commented 2 years ago

I have something working locally which I want to clean up first, will push it tomorrow

anthrotype commented 2 years ago

Ok, I changed it so that

1) nanoemoji.write_font now (once again) reads its glyph inputs from argv list of positional arguments instead of from the glyphmap file; inputs will contain .svg and/or .png files and are grouped in pairs by matching them by their path stem (the part of the basename excluding the suffix, e.g. "bar" for either "foo/bar.svg" or "foo/bar.png"). 2) the glyphmap is produced from the original SVGs (before they undergo picosvg or rasterization to bitmap) the source_names.txt file (originally used for write_codepoints, later replaced by write_glyphmap and left unused); this is to avoid having a .csv with differing number of columns depending on whether it has only one .svg or .png or both an .svg and a .png; what's importat for the glyphmap is the file stem, not the extension suffix, and neither the directory where the file is located; 3) write_font takes both a --glyphmap_file and a list of svg/png file arguments, and matches one with the other by keying on their stem; this ensures that, e.g., when there's both a 0061.svg and 0061.png, they get the same codepoints and glyph name mappings, and are attached to a single InputGlyph which has both an svg and bitmap attributes.

rsheeter commented 2 years ago

this is to avoid having a .csv with differing number of columns depending on whether it has only one .svg or .png or both an .svg and a .png

The change to blend glyphmap and argv bugs me, glyphmap was specifically supposed to be self-contained. Why not just make the CSV always contain ..., svg_file, png_file,...? - some formats would leave one or the other blank but that's fine. If both are present and codepoints from the respective filenames don't match that's an error condition.

anthrotype commented 2 years ago

Ok, I restored glyphmap.csv as the single source of inputs for write_font, with first two columns (optionally empty) for svg and png filenames. Also updated top description to match the current state of the PR. I think this is good to merge now.

rsheeter commented 2 years ago

LGTM!