-
Notifications
You must be signed in to change notification settings - Fork 27
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
[skrifa] autohint: CJK edge hinting #1138
Conversation
This was similar enough to latin that changes were folded into the current code with some branches.
Completes the CJK hinting algorithm
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 is much to be dismayed about, but all of it is faithful to the original and I think that's definitely the way to go.
GlyphId::new(9), | ||
&style::STYLE_CLASSES[style::StyleClass::HANI], | ||
); | ||
let expected_coords = [ |
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.
might be useful to just note where these came from..
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.
Noted that these are generated with some lovely printf debugging in FreeType
let edge2_ix = edge.link_ix.map(|x| x as usize); | ||
let edge2 = edge2_ix.map(|ix| &edges[ix]); | ||
// If we have two neutral zones, skip one of them. | ||
if let (true, Some(edge2)) = (edge.blue_edge.is_some(), edge2) { |
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.
can't believe we still don't have if-let chains!? but apparently not (rust-lang/rust#53667)
(if we did, this could be,
if let Some(edge2) = edge2 && edge.blue_edge.is_some() { .. }
)
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.
This seems to always be just over the horizon... at least we can avoid the double if
with the tuple pattern matching so I don't want to complain too much
// 1px < width < 1.5px | ||
(38, 26) | ||
}; | ||
if cur_len < 96 { |
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.
same :/
note where our expected values come from
Agreed. I filed #1141 to clean all of this up after we ensure matching behavior. |
Completes the CJK hinting algorithm
Based on #1134