-
Notifications
You must be signed in to change notification settings - Fork 12
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
Compile simple VFs w/gvar that works in a browser #236
Conversation
5c15f95
to
335c8be
Compare
|
22a3c9e
to
76547d8
Compare
Somewhat dodgy, but still a significant step forward.
Compile a gvar for Texturina that ttx accepts
Tweak to make testing with glyph name filter easier
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 quick run-through for me, but overall seems sane? Some notes inline, I imagine its worth getting cosimo's input here as well.
@@ -17,6 +17,8 @@ pub enum Access<I> { | |||
All, | |||
/// Access to one specific resource is permitted | |||
One(I), | |||
/// Access to two specific resource is permitted | |||
Two(I, I), |
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 can see where this is going
Cosimo is OOO so I will merge + send him an email to please review when back. |
@@ -248,6 +252,10 @@ impl<T: Copy> Location<T> { | |||
self | |||
} | |||
|
|||
pub fn rm_pos(&mut self, axis: &str) { |
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.
where is this rm_pos
being used? Also, the name is a bit cryptic
@@ -41,32 +41,40 @@ pub struct StaticMetadata { | |||
/// See <https://learn.microsoft.com/en-us/typography/opentype/spec/name>. | |||
pub names: HashMap<NameKey, String>, | |||
|
|||
/// Every axis used by the font being compiled | |||
/// Every axis used by the font being compiled, including point axes. |
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 suggest using "discrete axes" as that's the name used in DesignSpace format
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.
wait, maybe is this about something else?
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.
"an axis of variation defined only at a single point"... It's not immediately clear to me why we need to special case these
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 see you want to exclude these so-called "point axes" when constructing the VariationModel. In reality, these kind of setups (a variaiton axis where min=default=max) do not actually occur in real world font sources because they do not make sense to have, they would be no op. They may occur in your made-up test data (e.g. static.designspace) but I don't think it is worth having two different Vec<Axis>
in the static metadata, one for axes in general and one for actually variable ones (which is 100% of them in an actually variable font project). You can filter on demand when needed, no?
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.
FWIW fontmake does not treat these point-axes specially; e.g. if you feed your static.designspace to fontmake-py and ask it to make a "variable" font, fontmake resources/testdata/static.designspace -o variable
, it will make one with fvar containing that point axis, and all the other variation tables basically empty.
If you ask it to create static ttfs from the same designspace (-o ttf), it will make one for each of the masters (or one for each of the instances with additional -i option), and would be equivalent to running fontmake directly on the master UFO as input. I'm not saying we should replicate this UX in fontmake-rs, I think the latter should be designed to build variable fonts only.
Just saying that a font like our test static.designspace does not normally occur in the wild; if one wants to build a static font, one builds that one, doesn't create a dummy designspace to wrap 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.
it's nice to finally see variable glyphs render in browser, great work reaching this milestone.
A belated LGTM!
Builds on #230, #239. Fixes #207. Fixes #95.
Update fontations to pickup gvar fixes and thereby fixes #245.
cargo run -p fontc -- --source resources/testdata/mov_xy.designspace
now produces a VF that works in a browser.I have punted some feedback to issues as I believe just compiling any vf that works is worth capturing, specifically:
Sufficient to compile an at least somewhat functional Oswald:
$ git clone https://github.com/googlefonts/OswaldFont $ cd fontmake-rs $ cargo run -p fontc -- --source ../OswaldFont/sources/Oswald.glyphs
In browser: