Only write explicit tuple min/maxes if there is an intermediate tent #1345
Conversation
write-fonts/src/tables/gvar.rs
Outdated
| let required_intermediates = axis_coordinates | ||
| .iter() | ||
| .map(|coords| coords.required_intermediates()) | ||
| .collect::<Option<Vec<_>>>(); |
There was a problem hiding this comment.
we should be able to get rid of this allocation and do this all in one iter chain? I think? 🤔
There was a problem hiding this comment.
This did indeed turn out to be elegant and possible, with this neat new addition: https://users.rust-lang.org/t/unzip-with-error-handling/110250/2
...but I then realised that when reimplementing in fontations I had accidentally switched the semantics from any(...) to all(...), and so unfortunately it all had to be scrapped again ☔
With the intention corrected, there doesn't seem to be a better semantic match than a mutable fold, which @RickyDaMa and I have now implemented, although it does at least stay readable and keep allocations to a minimum.
In general, representing a combination of collect(...) and any(...) in an iter chain has turned out to be a bit of an abstraction tar-pit; each time you follow an improvement, you get bumped back into the same narrow path :(
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.
Unfortunately we lose some of the simplicity of the prior approach, as there is no Option `collect()` for `any()`, and we need to make the implied intermediates available outside of the coordinates struct.
Hoolean
force-pushed
the
conditional-gvar-intermediates
branch
from
c13a50d to
0aad38f
Compare
|
I've now added an upstream fontc PR to follow the new API, and unit tests for the new logic; we'll probably want to feed a font all the way through a ttx-diff pipeline before merging |
|
We should be good to undraft here - ready for review! |
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Okay so I'm trying to think through this, and understand if this is ultimately the API that makes the most sense.
Thinking out loud:
- I like the term 'tent', which although it was not immediately intuitive to me has the advantage that it is used elsewhere in the font ecosystem, and is fairly easy to understand.
- having an explicit
Intermediatetype feels maybe too verbose. If I look atAxisCoordinates::new, it even takesOption<Intermediate>, so if I don't need intermediates am I supposed to passNone, or am I supposed to callimplied_by_peak?
My instinct, then, would be to keep using the name Tent, instead of AxisCoordinate, and so I'd have something like,
.... okay so I started sketching out what I would've expected to see, and then it just ended up easier for me to push a commit. Check out e61f2cf and see if it makes sense?
Moved from original implementation in googlefonts/fontc#1225.
Motivation for moving:
This PR roughly sketches how the optimisation could look in a new home here instead. In general I think it has reduced the complexity of the code in the original PR, at the cost of some additional
Vecs that could be targeted later if we end up in the hot path, and some boilerplate when we already know we do not have intermediate regions in the test cases.If we are happy with this direction then we can polish the types and tidy the PR, and I will look to furnish it with unit tests too.