-
Notifications
You must be signed in to change notification settings - Fork 13
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
Generate kern fea and pass to fea-rs #373
Conversation
74bb6c6
to
efc748c
Compare
.collect(); | ||
|
||
// Compute the default on the unrounded deltas | ||
let default_value = deltas |
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.
// Compute the default
hm I'd expect the "default" to not be computed, but already defined in the input (as we discussed in #345)
otherwise we end with with default kerning values being applied twice, see #402 (comment) When we build glyph variations we also filter the default region deltas that VariationModel::deltas returns, we must do the same for variable FEA
that's what ufo2ft kernFeatureWriter does -- fixes the aV kerning in Oswald and similar
// WARNING: this will fail if the fea location isn't also a glyph location. In time we may wish to fix that. | ||
let var_model = &self.static_metadata.variation_model; |
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.
I expect this reuse of a single VariationModel for all kerning pairs to create similar issues as in the sparse glyph variations #400
The correct way to do this is to make specific VariationModels for each set of locations, fonttools does that and caches the models for the pairs referencing the same set of locations, but am not sure if the latter optimization is needed for the Rust port. I think we should prefer correctness at first and only do the memoization if the profiler confirms it's worth it.
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.
SG but I would like to defer this, will file an issue.
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.
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.
A couple notes inline, nothing major. :)
#345 with exploration of sparseness removed. Produces fea akin to:
Note use of user coords as fea-rs does not support unit suffixes at time of writing.
There are a fair # of TODOs remaining, plus the variable kerning built doesn't quite work (#402). Even so, I think this moves us forward enough to be worth capturing. Update: #406 helps a lot with having variable kerning that works by not throwing away data :)