Improve color tuple handling #363
Conversation
|
Something tells me this could be simplified. |
|
For more pragmatism, the alpha value when going to UFOs could be written as 0.35 or thereabouts, as Glyphs actually desaturates the colours so they don't overpower the rest of the UI. Then again, maybe not. |
|
There are different alpha values in different places. (the background of the cells in font view are drawn with alpha = 0.3). |
|
@schriftgestalt even if the alpha channel is not supported in Glyphs.app UI, why do you have to forcibly set it to The problem is UFO allows all the four RGBA channels to be set, and if Glyphs is going to arbitrarily change the alpha value when saving a .glyphs this creates a problem, as it makes it impossible to roundtrip between the two formats. You are free to discard that value when rendering the color if you prefer like that, but you should at least keep the data unchanged so one can store the UFO color in the glyphs file without losing data (besides the float rounding, but that's acceptable). |
|
I fixed that. It will now use the same 0–255 based storage for the alpha. It will round trip the "1" for now to not change the files. |
|
thanks for that, Georg 👍 |
|
@schriftgestalt we decided to treat the alpha channel as an uint8. Please change Glyphs so that it reads and writes the alpha channel, you can still ignore it in the UI. |
This is what I did. |
Lib/glyphsLib/builder/glyph.py
Outdated
| elif isinstance(color_index, int) and color_index in range(len(GLYPHS_COLORS)): | ||
| color_tuple = GLYPHS_COLORS[color_index] | ||
| color_values_clamped = tuple( | ||
| max(min(abs(int(v)), 255), 0) for v in color_index[:4]) |
There was a problem hiding this comment.
I don't understand why you have to clamp here. Raising ValueError when the color integer is not in range 0..255 was the correct thing to do here. The argument that Glyphs.app doesn't crash if a user attempts to set a color integer which is not in range to me does not apply here. In the case of glyphsLib, receiving invalid data must be shouted out loud, not silently fixed as you do here.
why do you take the first four items with the slice color_index[:4]?. Should you not raise if the color tuple does not contain 4 items, and is that even possible? I believe it always contains 4 items in the glyphs source file, and if it doesn't, well it's an error.
Lib/glyphsLib/builder/glyph.py
Outdated
| # Otherwise, make a Glyphs-formatted color list | ||
| return [round(component * 255) for component in list(color)] | ||
| # Otherwise, make a Glyphs-formatted color list: [u8, u8, u8, 1]. | ||
| return [round(component * 255) for component in tuple(color)[:4]] |
There was a problem hiding this comment.
again, the UFO comma-separated color string always contains 4 items. if it doesn't it's invalid. I think defcon or ufoLib would choke on such an incomplete color string, or one that contains more than 4 items.
Do an assert if you want to be cautious, but remove that slice.
Lib/glyphsLib/types.py
Outdated
|
|
||
| Color is either a fixed color (when coloring something from the UI, see | ||
| the GLYPHS_COLORS constant) or a list of the format [u8, u8, u8, 1], | ||
| negative numbers are abs()'d. Glyphs does not support an alpha channel |
There was a problem hiding this comment.
negative numbers are abs()'d
negative numbers are never written to file! this function is supposed to parse a color spec from a string found in a .glyphs file, no? So why should it handle this degenerate case?
Lib/glyphsLib/types.py
Outdated
| rgb = src[1:-1].split(",") | ||
| color_template = [0, 0, 0, 255] | ||
| for i, v in enumerate(rgb): | ||
| if v: # Could be "" |
There was a problem hiding this comment.
are you sure that one of the items in the color tuple in a .glyphs source file could be ""?
I could not verify this. When I set a glyphs' colorObject from the Macro Panel and save, I always get a tuple of four integers in range 0..255 (with exception of the last which is always 1, but Georg will fix that).
Lib/glyphsLib/types.py
Outdated
| color_template[i] = max(min(abs(int(v)), 255), 0) | ||
| return tuple(color_template) | ||
|
|
||
| rgba = tuple(int(v) for v in src[1:-1].split(",") if v) |
There was a problem hiding this comment.
I think you should raise ValueError here if the items are not 4
tests/types_test.py
Outdated
| "(-1)": (1, 0, 0, 255) | ||
| "(100, 200)": (100, 200), | ||
| "()": (), | ||
| "(-1)": (-1,) |
There was a problem hiding this comment.
and these last three tests don't make sense to me, incomplete or out of range color tuples are not "colors" any more
Glyphs saves a glyphs' color tuple as a List of type [u8, u8, u8, 1]. The alpha channel is not supported as of Glyphs 2.5.1 (confirmed by Georg Seifert by mail) and is always set to 1. The UFO specification uses floats to store color values, using 3 decimal points is enough to round-trip u8 to float and back.
|
Hah. I just did some Glyphs test and it turns out... Glyphs throws the tuple away or mishandles it unless the tuple is on a single line! So, currently: Will be read and written as (0, 0, 0, 1). This works as expected: This misinterprets the color: |
|
so what? both us and Glyphs.app write the color tuple on a single line, we don't need to handle all possible edge cases! |
|
argh... I meant "we don't need to handle all possible edge cases" |
Nope. We apparently do That's how I even found it ;) |
|
ouch.. 🤕 |
|
ok then this is definitely a bug in Glyphs.app own parser, because the whitespace in old-style OpenStep property lists is irrelevant, and a valid array can be written with both notations. @schriftgestalt can you please take a look? thank you |
|
Still looking into turning this into a one-liner because we settled on using 2.4.x internally for now. |
|
@madig you can probably work around this and force our own glyphsLib Writer to serialize color tuples in one line, see |
|
As as side note, I'd curious to know if there are other places in the .glyphs file format where it diverges (whether deliberately or unknowingly) from the OpenStep property list format. |
|
Currently this is the only exception in terms of line breaks. Unicode not having quotes is another. There will be some more in the next version (all the I have a look at my parser to be able to read colors from multiple lines. |
|
It is crucial that all the divergences from the standard OpenStep property list are documented. The latter has many shortcomings but at least it is clear and simple to parse, and there are publicly available implementations, like Apple's CoreFoundation or the one from GNUStep. |
|
of course, I'm not talking about the way the data is structured, which you're free to change as you please, but about the format's grammar, i.e. valid tokens and syntactical rules, etc. |
Otherwise, Glyphs 2.4.x won't read it correctly.
|
Okay, stuff now seems to pass through Glyphs intact. @anthrotype is the last commit okay? Can I merge then? |
Glyphs saves a glyphs' color tuple as a List of type [u8, u8, u8, 1]. The alpha
channel is not supported as of Glyphs 2.5.1 (confirmed by Georg Seifert by
mail) and is always set to 1. The UFO specification uses floats to store color
values, using 3 decimal points is enough to round-trip u8 to float and back.