-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[skrifa] autohint: CJK edges #1134
Conversation
This was similar enough to latin that changes were folded into the current code with some branches.
skrifa/src/outline/autohint/axis.rs
Outdated
@@ -184,6 +184,10 @@ impl Segment { | |||
edges.get(self.edge_ix.map(|ix| ix as usize)?) | |||
} | |||
|
|||
pub fn edge_next<'a>(&self, segments: &'a [Segment]) -> Option<&'a Segment> { |
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.
is there a reason this isn't next_edge
? Does it mean something different?
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 returns the next segment in the parent edge. This name was copied from FT but I agree that it's ambiguous. Renamed to next_in_edge
for clarity.
) { | ||
axis.edges.clear(); | ||
let scale = metrics.scale; | ||
let top_to_bottom_hinting = if axis.dim == Axis::HORIZONTAL { | ||
let top_to_bottom_hinting = if axis.dim == Axis::HORIZONTAL || group != ScriptGroup::Default { |
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.
horizontal implies not top-to-bottom makes sense. The group limitation somewhat less so. I suggest a comment.
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'm probably being overly defensive here but the CJK code always passes constant 0 where this value is expected. I added a comment with a link to FT code
'segments: for segment_ix in 0..axis.segments.len() { | ||
let segment = &axis.segments[segment_ix]; | ||
if segment.dir != Direction::None { | ||
continue; | ||
} |
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.
Why not for (ix, segment) in axis.segments.iter().enumerate().filter(no direction)
? - particularly the .iter().enumerate()
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.
Would that create unpleasently overlapping borrows? - if so suggest a comment noting as much
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.
Yes, we make mutable calls into axis
below which trips up the borrow checker. Added a comment
if segment.dir != Direction::None { | ||
continue; | ||
} | ||
// Find a matching edge |
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.
Suggest // Find the first matching edge
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.
Changed to "Try to find an edge that coincides with this segment within the threshold" which better explains what's happening here
axis.append_segment_to_edge(segment_ix, edge_ix); | ||
// Move to next segment | ||
continue 'segments; | ||
} |
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.
At a glance this also looks possible to do via iterators? - I didn't try so perhaps not; if not perhaps add a comment explaining why.
if let Some(edge_ix) = axis.edges.iter().enumerate().filter_map(if dist < threshold retain index).find()
type of deal?
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.
Good catch. Done
) { | ||
if axis.dim != Axis::VERTICAL { | ||
// For the default script group, we only handle vertical blues |
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.
because ...
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.
... FreeType does this. I added a link to the relevant code.
In reality, FT doesn't use any alignment zones in the horizontal direction but the infra exists for it. I believe this is an attempt at better hinting for vertical text but the code is permanently disabled with an #undef
. After this all lands, I think it's worth considering simplifying our code and removing this altogether. Filed #1140
// from the edge | ||
// See <https://gitlab.freedesktop.org/freetype/freetype/-/blob/57617782464411201ce7bbc93b086c1b4d7d84a5/src/autofit/afcjk.c#L1356> | ||
if (edge.fpos as i32 - unscaled_blue.position).abs() | ||
> (edge.fpos as i32 - unscaled_blue.overshoot).abs() |
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 adding some methods to things like ScaledBlue
and Edge
. For example, instead of let is_top = blue.flags & (blue_flags::TOP | blue_flags::LATIN_SUB_TOP) != 0;
we could have blue.is_top()
, instead of (edge.fpos as i32 - unscaled_blue.position).abs()
we could have edge.distance_to(unscaled_blue)
(...or would it need to be specifically unscaled_distance_to?)
This could be deferred to an issue if we don't want to address here. I feel like autohinting would be quite a bit easier to read if widely deployed.
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's a lot that can be done to clean up the whole autohint module but I prioritized (mostly) matching the FT code in the first pass. I agree that we can do much better but I would like to defer until after we land. Filed #1141
scale.y_scale | ||
}; | ||
// Initial threshold | ||
let initial_best_dest = fixed_mul(scale.units_per_em / 40, axis_scale).min(64 / 2); | ||
for edge in &mut axis.edges { | ||
let mut best_blue = None; | ||
let mut best_is_neutral = false; |
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.
Optional. I don't have time to try it but finding the best values for an edge smells a lot like reduce. Is it possible it would make sense to write this in terms of reduce? - if it does but we don't want to do it now feel free to defer to 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.
Added to #1141. This seems possible but the interior code would need to be factored out into a function to make the change worthwhile.
This was similar enough to latin that changes were folded into the current code with some branches.