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.

Sign up for GitHub

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 COLR/CPAL support #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add COLR/CPAL support #41

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions Lib/extractor/formats/opentype.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ def extractFontFromOpenType(
doKerning=True,
customFunctions=[],
doInstructions=True,
doColors=True,
):
source = TTFont(pathOrFile)
if doInfo:
Expand All @@ -60,6 +61,8 @@ def extractFontFromOpenType(
function(source, destination)
if doInstructions:
extractInstructions(source, destination)
if doColors:
extractColorsFromOpenType(source, destination)
source.close()


Expand Down Expand Up @@ -1046,3 +1049,44 @@ def _extractOpenTypeKerningFromKern(source):
# there are no minimum values.
kerning.update(subtable.kernTable)
return kerning

# -----
# Color
# -----

COLOR_LAYERS_KEY = "com.github.googlei18n.ufo2ft.colorLayers"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but where should that option be passed in?

COLOR_PALETTES_KEY = "com.github.googlei18n.ufo2ft.colorPalettes"
COLOR_LAYER_MAPPING_KEY = "com.github.googlei18n.ufo2ft.colorLayerMapping"
UFO2FT_FILTER_KEY = "com.github.googlei18n.ufo2ft.filters"
def extractColorsFromOpenType(
source,
destination,
):
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.
Copy link
Member

@anthrotype anthrotype May 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

firstPalette = source["CPAL"].palettes[0]
colorIDs = list()
for i, color in enumerate(firstPalette):
colorIDs.append((color.red/255.0, color.green/255.0, color.blue/255.0, color.alpha/255.0))

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():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for i, layer_record in enumerate(layer_records):
(name, colorID) = (layer_record.name, layer_record.colorID)
layer_name = "color{}_layer{}".format(colorID, i)
if not layer_name in destination.layers:
layer = destination.newLayer(layer_name)
layer._set_color(colorIDs[colorID])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that a private method of defcon? consider some alternative way that doesn't require accessing a private member

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should just be layer.color

glyph = destination[name]
destination.layers[layer_name].insertGlyph(glyph, name=base_glyph)
layerGlyphs.append(name)

for (base_glyph, layer_records) in source["COLR"].ColorLayers.items():
destination[base_glyph].lib[COLOR_LAYER_MAPPING_KEY] = [("color{}_layer{}".format(lr.colorID, i), lr.colorID) for i, lr in enumerate(layer_records)]

for name in layerGlyphs:
if name in destination.layers.defaultLayer:
del destination.layers.defaultLayer[name]