search

googlefonts / nanoemoji

A wee tool to build color fonts.
Apache License 2.0
239 stars 19 forks source link

support OT-SVG color variables for CPAL palette entries #431

Closed anthrotype closed 1 year ago

anthrotype commented 2 years ago

Fixes #422

Needs tests, don't merge yet. Tests added

anthrotype commented 1 year ago

@rsheeter PTAL. I think this is good enough to merge (it allows to build Nabla OT-SVG with multiple CPAL palettes from its COLRv1 variant).

rsheeter commented 1 year ago

How much does the size of the SVG table for Nabla change with/without this? - not suggesting we block the PR, just an interesting datapoint

anthrotype commented 1 year ago

Re: namedtuple vs dataclass, i hinted at the reason in the commit message, I can can add a comment maybe: https://github.com/googlefonts/nanoemoji/pull/431/commits/043bcdfceacb594d5dc0121af94c85bb3079b399

In general I think the dataclass interface is much superior to namedtuple, but if you really prefer the latter I can change it back to that (and do away with ClassVar which namedtuple can't support).

I'll address the rest of the comments next week when I'm back, thanks for the review

anthrotype commented 1 year ago

How much does the size of the SVG table for Nabla change with/without this?

the total font file size increased by 45KB, from 1,598,000 bytes to 1,643,616 bytes (of course, all due to the more verbose SVG table)

anthrotype commented 1 year ago

but the WOFF2 version is only 2.5KB larger, going from 185,004 to 187,540 bytes

rsheeter commented 1 year ago

the dataclass interface is much superior to namedtuple

How so?

the total font file size increased by 45KB, from 1,598,000 bytes to 1,643,616 bytes (of course, all due to the more verbose SVG table)

That seems like 45kb well spent to me :)

anthrotype commented 1 year ago

the dataclass interface is much superior to namedtuple

I already gave up, I'm changing it back to nanedtuple, life is too short

anthrotype commented 1 year ago

FWIW see https://peps.python.org/pep-0557/#why-not-just-use-namedtuple

rsheeter commented 1 year ago

https://peps.python.org/pep-0557/#why-not-just-use-namedtuple

Ty, I had not seen that. That seems sufficient to have a strong preference for dataclasses indeed. ...now I wonder if we have other namedtuples we should remove!

anthrotype commented 1 year ago

ha.. so shall I revert 588fdae now?

rsheeter commented 1 year ago

ha.. so shall I revert https://github.com/googlefonts/nanoemoji/commit/588fdaeaf20e95560f1f51a060352671f988acf0 now?

Yes please. Sorry for the delay and thank you - for the many'th time - for taking the time to explain python basics to me :D