New problems with duplicate Unicode for color glyphs
Closed this issue · 13 comments
The duplicate Unicode issue still appears for Blaka Ink, a COLRv1 font with gradients.
Observations:
- The tests that were recently added are clearly insufficient in catching this.
- I'm not 100% sure yet, but I believe that the unicodes must be muted in two places: here in addition to here.
- This condition as triggered from here currently prevents the processing of color layers for my font (
minimal=False
).
I can't easily fix this because I don't understand the minimal architecture. But I'm happy to help with a PR if I'm told what to do.
I would start by adding
color0.attributes["color"] = 1
color1.attributes["color"] = 1
to the new test here.
It already starts throwing other errors before even checking for the unicodes.
clearly insufficient
well, not clearly enough to me by just reading this issue. Could you provide steps to reproduce the issue?
I don't understand the minimal architecture.
fontmake sets minimal=True because it's only interested in the minimal pieces of data required for compilation, not caring about other metadata desirable when roundtripping Glyphs<=>UFO
Take Blaka Ink (linked at the top), and try to generate a font from it.
It stalls with fontmake: Error: In 'instance_ufo/BlakaInk-Regular.ufo': Compiling UFO failed: cannot map 'A.color0' to U+0041; already mapped to 'A'
, which is the same issue of #937 still persisting.
I already pulled out all the relevant points in the code where something could go wrong.
#939 was supposed to fix an issue with COLRv0 ("color palettes" fonts as Glyphs.app calls them). So you're saying the duplicate unicode issue also affects COLRv1, good. Will take a look
Then the test should be duplicated, once for COLRv0 and once for COLRv1. But given the absence of processing of any type of color glyphs through the minimal switch, even the COLRv0 must fail in real world examples, I believe.
Then the test should be duplicated, once for COLRv0 and once for COLRv1
sounds good, can you work on a PR?
You're correct, we need to set unicodes to empty list right after we call to_ufo_glyph() to populate the COLRv1 layer glyph
I don't know what you mean with your references to minimal={True,False}, fontmake sets that correctly to True so the color layers are processed. Are you doing something else?
Are you doing something else?
Idk, I'm using gftools builder
. Maybe it sets the variable. Let me investigate this.
I'll start a PR after lunch.
I don't understand this.
gftools builder
doesn't set minimal
to False
anywhere, and in fact fontmake
doesn't even have an interface for that. It calls glyphsLib.to_designspace()
with minimal=True
explicitly, as you said, so it overwrites the default minimal=False
in the method definition of glyphsLib.to_designspace()
, which in turn hands over the incoming parameter directly to UFOBuilder()
.
In my case, both to_designspace()
and UFOBuilder()
receive a minimal=False
value and thus fail the color handling.
I'm running gftools builder sources/blakaink.yaml
in the Blaka repo
Can you please try to reproduce this? I feel like there's a ghost in my computer.
Update: There's a new ninja builder in gftools
that I overlooked which calls UFOBuilder()
directly with all the default arguments, so minimal
comes as False
here.
Let's keep this issue open anyway please because I want to make sure that COLRv1 fonts are tested correctly in glyphsLib.
great so now we have a new fontmake-but-not-quite to bug fix