Only use IUP representation when more compact #616
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #568; when presented with deltas that include interpolatable values, we will generate both representations and then choose the more compact.
This is mostly straightforward, but there is some API to bikeshed, particularly around the
iup_delta_optimize
API.Previously, this returned the deltas as
Vec<Option<Vec2>>
, withNone
values representing deltas that could be interpolated. This meant that we did not have access to these deltas, which are necessary in order for us to generate the non-IUP representation.We have some options here. The simplest one would be to return the
GlyphDelta
type that we use when building gvar; the only concern here is that this type represents a point as(i16, i16)
, where we are currently usingVec2
. I personally think this is fine, since as far as I can tell we do no subsequent processing between when we check IUP and when we compile the final binary, but I want to minimize changes for the time being.Another option would be to add another new type that is basically
(Vec2, bool)
; a third option is to just returnVec<bool>
with an entry for each input item, indicating if it can be interpolated.I chose this last option, but after trying to integrate into
fontc
it feels bad; fontc passes ownership of the deltas toiup_delta_optimize
, which means we need an additional clone of all the deltas infontc
in order to convert from our bools back to some useful representation.Finally there's an additional headache; if we define a type in
write-fonts
we can't easily store that type infontc
contexts becausewrite-fonts
types do not implement the serde traits; this has come up before and so maybe is another argument for merging #392.tl;dr: life is easiest of
iup_delta_optimize
returnsGlyphDelta
directly, and if this type implementsSerialize
andDeserialize
?On a happy note, if I use this patch in fontc than the gvar diff for our fonts drops dramatically, and it looks like the only remaining difference would be fixed by #614.
progress on googlefonts/fontc#422