Skip to content
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 use IUP representation when more compact #616

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Sep 19, 2023

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>>, with None 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 using Vec2. 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 return Vec<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 to iup_delta_optimize, which means we need an additional clone of all the deltas in fontc 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 in fontc contexts because write-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 returns GlyphDelta directly, and if this type implements Serialize and Deserialize?

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

@anthrotype
Copy link
Member

anthrotype commented Sep 20, 2023

I also think it would be fine if iup_delta_optimize API returns GlyphDeltas (which carry along the required flag), it's fine if they get rounded to integer as the caller doesn't need to process them further. And I don't like too much that we need to clone the deltas before calling iup_delta_optimize. Could we pass a reference or slice instead?

If we decide to return rounded GlyphDeltas, maybe the rounding method itself could be a parameter. Right now I think we don't match fonttools, which in the case of rounding of deltas it uses the built-in Python round() function instead of the usual OtRound (python built-in round() uses the Banker's method which is more statistically robust), but I am digressing and this belong to a separate issue (googlefonts/fontc#235)..

+1 for optional serde support

@cmyr cmyr force-pushed the only-iup-when-better branch 2 times, most recently from c2ec882 to 10b227d Compare September 20, 2023 17:20
@cmyr
Copy link
Member Author

cmyr commented Sep 20, 2023

I've added serde and updated this on top of it to return GlyphDeltas, and I think it's much cleaner overall.

This requires changing the API for the PackedDeltas type so that the
caller does not pre-prune deltas that can be interpolated, since we need
to know these values if we decide *not* to interpolate; this also
requires changing the iup_delta_optimize method to not discard deltas
that can be interpolated.

In a quick test on GS this reduces the size of the gvar table from
3231 kb to 2829 kb.

There is one other potentially significant optimization remaining, which
is to share points between tuples where possible.
@cmyr cmyr force-pushed the only-iup-when-better branch from 10b227d to 90f47a1 Compare September 26, 2023 20:26
@cmyr cmyr merged commit c922bae into main Sep 26, 2023
9 checks passed
@cmyr cmyr deleted the only-iup-when-better branch September 26, 2023 20:40
anthrotype added a commit to googlefonts/fontc that referenced this pull request Sep 28, 2023
so that fontc can work again with fontations/HEAD following two breaking changes:
googlefonts/fontations#616
googlefonts/fontations#619
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

should prefer non-IUP-optimized deltas when more compact
2 participants