-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add CBDT to maximum_color #400
Conversation
what if one only wants COLRv1 and OT-SVG but not CBDT, or only COLRv1 and CBDT but not OT-SVG? Maybe maximum_color should have an option to select the desired list of formats (defaulting to all possible ones given input). |
# the bitmap is vertically scaled to fill the space desc to asc | ||
# this gives us a ratio between pixels and upem | ||
funits = config.ascender - config.descender | ||
return (bitmap_pixel_height, funits) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the function is named _pixels_to_funits
, so i'd expect it to actually return the ratio pixels/funits
(flaot) instead of passing them through as a tuple. It's not doing much besides subtracting ascender-descender
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should make a type. I'm under the impression that by delaying the computation and always doing multiple (enbiggen) then divide we minimize float damage.
I liked that one could make a hybrid font with multiple tables directly from the same set of input SVGs. Now one would have to first make either a COLRv1 or OT-SVG font, and then run maximum_color on it to add the rest of the tables (and may also find oneself with additional tables that were not requested, e.g. if one is only interested in testing vector color formats and is not interested in the bitmaps). |
I think if we want that back we should add a flag to nanoemoji to directly list the color table(s) desired and then it can share code with maximum color. The method of provisioning flags for every combination of formats just didn't scale, many valid combinations were left out. EDIT: bitmaps are now off by default so if you just run the tool it adds whichever vector color table is missing |
@@ -296,10 +296,13 @@ def glyph_region(ttfont: ttLib.TTFont, glyph_name: str) -> Rect: | |||
"""The area occupied by the glyph, NOT factoring in that Y flips. | |||
|
|||
map_font_space_to_viewbox handles font +y goes up => svg +y goes down.""" | |||
width = ttfont["hmtx"][glyph_name][0] | |||
if width == 0: | |||
width = ttfont["glyf"][glyph_name].xMax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm I'm not sure about using the xMax as replacement for advance width when this is 0... Are you doing this because of the clipping of bitmaps that overflow the viewbox #402? It feels like you are special-casing the case when a glyph bounds exceed horizontally.. Ideally some more global fix would be desirable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's a bug in the input but it seemed like it happened for glyf components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking resolution of this in #404
cpal_color = ttfont["CPAL"].palettes[0][palette_index] | ||
palette = ttfont["CPAL"].palettes[0] | ||
if palette_index == _FOREGROUND_COLOR_INDEX: | ||
return colors.Color.fromstring("black") # as good a guess as any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why black? Should we not use the special "currentColor" here?
#381
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking resolution of this in #405
|
||
_generate_cbdt(nw, input_font, font, color_font, picosvg_files) | ||
# until now we held glyph names stable, nuke 'em | ||
_strip_glyph_names(nw, wip_file, final_output) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glyph names may be useful for debugging so ideally maximum_color should respect the --keep_glyph_names flag (flase by default) like the main nanoemoji compiler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I forgot we had a flag for that. Good catch!
1913a3d
to
5533f7d
Compare
5533f7d
to
335f056
Compare
See https://github.com/googlefonts/nanoemoji/tree/maximum_bitmap#adding-color-tables-to-existing-fonts for outline of what/why.
Remove formats with multiple color tables from --color_formats, user should use maximum_color instead.
Add support for variable width bitmaps. Fixes #401.