Add COLR/CPAL support #41
Conversation
| ): | ||
| if not "CPAL" in source or not "COLR" in source: return | ||
| destination.lib[UFO2FT_FILTER_KEY] = [{"name": "Explode Color Layer Glyphs", "pre": True}] | ||
| # UFO format only supports one palette. Others ignored. |
There was a problem hiding this comment.
that's not true, colorPalettes key can contain multiple palettes
it's not "UFO format", btw. This is currently a private set of keys only defined by ufo2ft, not made public in the UFO spec as of yet
There was a problem hiding this comment.
Uh, actually it's both. I added certain keys to support building in fontmake, but those keys aren't strictly necessary. If you go just by layercontents.plist and into each glyphs directory's layerinfo.plist you can easily build a color font from that. I assume that @justvanrossum didn't implement it this way, but instead did it by preprocessing the glyphs, to save time. Once @simoncozens works more on fonttools-rs I'll probably add COLR/CPAL support without these keys.
There was a problem hiding this comment.
But there's the rub: there's only a single layercontents.plist. You can't have more than one for different palettes (maybe something to add to UFO4?). Sure, these hacky keys support more than one palette, but I added the keys as an afterthought after I already made sure that the UFO generated correctly.
There was a problem hiding this comment.
we deliberately chose not to use the layerinfo.plist color for this, since that one is traditionally used for GUI annotation purposes, in fact it's called markColor in defcon API for that reason.
these hacky keys
I can accept that you say they are not documented properly, but they are not "hacky" keys. They are the ufo2ft's way to build COLR/CPAL fonts.
There was a problem hiding this comment.
I have no idea why it'd be expected that the layer color stops at the GUI, and I don't see that in the spec either?
There was a problem hiding this comment.
It makes sense to say it because a lot of work went into getting it to its current state based on the current reading, and more work would be needed to change it to a different reading. Also, the project is one year old in September, though the color layers are only a few months old, true. And only a few days old if you are only going by releases and not development branch activity.
I put more stock in how much effort something would take to change than how long it's been a certain way, though.
There was a problem hiding this comment.
@benkiel This probably isn't the place, but what about instead of promoting the keys, we add a disambiguating key? Something like layer_colors_exported, receiving a boolean? I can easily support that.
I just see it as duplicated data when most of the time users are going to want the GUI color and exported color to equal one another, in a world of color fonts.
As far as the multiple palettes go, this is going to have to go into UFO4. I'd naively solve it by making the layer color an array of strings instead of just a string. If it's missing from one layer and not another, then the color is equal across both palettes, and the layer with the most colors dictates how many palettes the font has. What do you think?
There was a problem hiding this comment.
I use colors a lot for grouping glyphs in the GUI, even in COLR/CPAL fonts, so making layer colors work also as CPAL colors would be the wrong thing for my fonts, since the two have different uses. It is better to keep the current UFO layer color as they have been understood by almost all implementations so far, and use a different structure for color font layers. Glyphs also uses distinct structures for GUI colors and CPAL palettes.
There was a problem hiding this comment.
This isn't a well-tread part of the spec, but since UFO4 will likely support COLRv1, this GUI color separation concern makes me think of the fact that COLRv1 has a flags array with masks for USABLE_WITH_LIGHT_BACKGROUND (0b1) and USABLE_WITH_DARK_BACKGROUND (0b10). (Cf. Palette Type Array, CPAL, OT Spec.) Since flags equal to 0b0 are meaningless (not usable at all?), that could be interpreted as, well, not usable at all; only a GUI coloring; not exported.
So if we're already saying that the color should take an array of strings, well, there'd also be a need to mark flags for each palette. Maybe, rather than an array of strings, it should be a structure similar to [(flags, color), ...]. That makes the most sense to me, and has the benefit of making the default the current behavior, 0b0, not usable for any export.
There was a problem hiding this comment.
Or not? Do we think that the flags are useless and not worth supporting? I think they'll end up used eventually, emoji for use with dark themes seems to be the intent.
| layer_name = "color{}_layer{}".format(colorID, i) | ||
| if not layer_name in destination.layers: | ||
| layer = destination.newLayer(layer_name) | ||
| layer._set_color(colorIDs[colorID]) |
There was a problem hiding this comment.
is that a private method of defcon? consider some alternative way that doesn't require accessing a private member
| # Color | ||
| # ----- | ||
|
|
||
| COLOR_LAYERS_KEY = "com.github.googlei18n.ufo2ft.colorLayers" |
There was a problem hiding this comment.
this COLOR_LAYERS_KEY is unused, because you're using the more high-level alternative approach of COLOR_LAYER_MAPPING_KEY combined with "Explode Color Layer Glyphs" filter.
Maybe you could it made optional whether to create multiple layers containing glyphs with the same colorID (i.e. what you're currently doing) vs. creating a single default layer that contains all the glyphs, both the base color glyphs and their "glyphlets", with colorLayers key defining the mapping with the respective palette index.
There was a problem hiding this comment.
Sure, but where should that option be passed in?
| destination.lib[COLOR_PALETTES_KEY] = [[(color.red/255.0, color.green/255.0, color.blue/255.0, color.alpha/255.0) for color in firstPalette]] | ||
|
|
||
| layerGlyphs = list() | ||
| for (base_glyph, layer_records) in source["COLR"].ColorLayers.items(): |
There was a problem hiding this comment.
this is assuming COLR v0 table (flat colors only); maybe you should check the table.version and error out nicely if it's == 1 (instead of AttributeError).
COLRv1 defines a tree of paint tables with support for gradients and much more. It's not officially in OpenType yet so it's ok if extractor doesn't support this until finalized.
There was a problem hiding this comment.
Thanks @ctrlcctrlv for working on this! I left some comments in my (generally positive) review.
Some tests would be nicer too.
|
Coming back to this; now that color has been more settled, should this PR be updated? |
This allows
ufo-extractorto output UFO fonts which can be built as COLR/CPAL color fonts byfontmake, and also be understood as color UFO fonts byMFEKglif.An example of this working is https://github.com/MFEK/TwemojiMozilla.ufo.
Based on my Twitter thread https://twitter.com/MFEKglif/status/1396819553041125378.
This patch was under the purview of the MFEK project because I needed a complicated example UFO font with many layers.