-
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
Load .glyphs instances and use them to inform axis rigging #252
Conversation
0dce517
to
16512c2
Compare
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.
LGTM with nits. I can’t comment on glyphs format specific things but will approve to unblock until our expert returns.
pub exports: Option<i64>, | ||
pub active: Option<i64>, | ||
pub type_: Option<String>, | ||
pub axes_values: Option<Vec<OrderedFloat<f64>>>, |
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.
Just a general observation. It would be nice if we could encapsulate the use of OrderedFloat
to cases where it’s absolutely necessary (e.g. internal to types that are used as keys).
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.
Some rust pedantry inline; the business logic seems reasonable but I'm not very familiar with the glyphs format.
Co-authored-by: Colin Rofls <colin@cmyr.net>
@@ -929,7 +1020,14 @@ fn default_master_idx(raw_font: &RawFont) -> usize { | |||
.map(|(idx, _)| idx) | |||
.unwrap_or(0) | |||
}) | |||
.unwrap_or(0) | |||
// TODO: implement searching for "a base style shared between all masters" as FontTools does | |||
// Still nothing? - just look for one called Regular |
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.
it's actually more complicated than "if no Variable Font Origin custom parameter, then use the master whose name is 'Regular'", as you can see from get_regular_master
function in glyphsLib.builder.axes...
If you don't want to implmenet the "find a base style shared between all master" logic right now, the best fallback value for no explicit Variable Font Origin is taking the first font.masters[0], rather than the one called "Regular".
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.
For Oswald fontmake falls back to Regular after failing at base style matching so this is incrementally closer. I need to find - or create - a source we build wrong due to not having the full find a shared base style. 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.
it must have been not fun to digest the messy logic from glyphsLib... LGTM!
Improve, though not yet to parity with fontmake, handling of glyphs 2/3 instances:
Step toward #250