-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
Only write explicit tuple mins/maxes if there is an intermediate tent #1225
base: main
Are you sure you want to change the base?
Only write explicit tuple mins/maxes if there is an intermediate tent #1225
Conversation
This optimisation is also implemented in fontTools, and reduced `gvar` table size by ~50% when testing locally on a large variable font.
(clippy lints are valid, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I think it would be possible to continue using the builder if we wanted, and change it so that build()
returns (Tuple, Option<(Tuple, Tuple)>)
but I also think it's fine inlined since it isn't used anywhere else.
I wouldn't worry about putting back the trace!
, and I'll be happy to merge when CI is green.
let mut peaks = Vec::<F2Dot14>::new(); | ||
let mut maxes = Vec::<F2Dot14>::new(); | ||
|
||
// Optimisation: only write explicit mins and maxes if at least one of the tents is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to add test(s) to prove it works?
I've hit a road block adding tests: fontations' We could make this field public, but I am tempted to argue that we make the wider change of moving the abstraction boundary lower into fontations, and treating this as serialisation logic. e.g. give I think this optimisation is higher-level than delta set optimisation but lower-level than IUP optimisation -- it is lossless, ttx does not expose it, and there has never been motivation to toggle it -- and so it may be more ergonomic there. We could avoid unzipping until serialisation time, ditch the assert that the tuple lengths are the same, and gain a bit more immutability for example. Am happy to go either way; interested to hear your thoughts :) |
I also think this logic belongs to fontations |
I'll put together a PR there to see how this looks |
Moved from original implementation in googlefonts/fontc#1225, see discussion. This optimisation is also implemented in fontTools, and reduced `gvar` table size by ~50% when testing locally on a large variable font.
Moved from original implementation in googlefonts/fontc#1225, see discussion. This optimisation is also implemented in fontTools, and reduced `gvar` table size by ~50% when testing locally on a large variable font.
Moved to googlefonts/fontations#1345 |
This is a lossless optimisation that is also implemented in fontTools, and reduced
gvar
table size by ~50% when testing locally on a large variable font.I inlined
TupleBuilder
as there was a precise ordering needed for converting the types, comparing values, and making the output conditional that was tricky to represent in the existing abstraction without lots of zipping and unzipping.I tried a few functional patterns but couldn't find anything elegant without wider changes to types owing to the previous ordering, so thought it would be better to leave it inline and imperative and reach out for feedback first before doing anything more opinionated 😄
Before merging, this new logic would benefit greatly from unit test coverage too.