-
Notifications
You must be signed in to change notification settings - Fork 143
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
CPU-side dashing for GPU strokes & encoding-time filtering of zero-length segments #412
Conversation
c9171b0
to
5c82590
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 modulo small style suggestion. Thanks!
crates/encoding/src/path.rs
Outdated
fn start_tangent_for_curve( | ||
p0: (f32, f32), | ||
p1: (f32, f32), | ||
p2: (f32, f32), | ||
p3: (f32, f32), | ||
p2: Option<(f32, f32)>, | ||
p3: Option<(f32, f32)>, | ||
) -> Option<(f32, f32)> { |
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.
Small style suggestion: making this a method of PathEncoder
and dropping the p0
parameter (since it always appears to be PathEncoder::first_point
?) should simplify the call sites a bit.
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.
Done!
let missing_movetos = [ | ||
MoveTo((0., 0.).into()), |
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 find this more amusing than is reasonable :)
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 probably a good idea to fix these up and remove the MoveTo(0, 0)
s when we fix the path encoding.
When GPU-side stroking is enbled, a stroke with a dash style now gets converted to a series of undashed strokes which then get encoded separately and in sequence. To support this, `Encoding` now accepts an iterator over `PathEl` as an alternative to `Shape`.
Zero-length segments lead to numerical errors and lead to extra complexity and wasted work (in the form of thread allocation) for GPU flattening. We now detect and drop them at encoding time. Currently, a path segment is considered zero-length if all of its control points (in local coordinates) are within EPSILON (1e-12) of each other. This may not be the desired behavior under transform. Also since the length here isn't in terms of the actual arc length, this won't detect all curves that are within an EPSILON sized bounding box (which may have a larger distance between their control points). For stroking purposes, the distance between the control points is what matters most as that's what's used to compute a curve's tangent.
5c82590
to
8001151
Compare
When GPU-side stroking is enabled, a stroke with a dash style now gets converted to a series of regular strokes which then get encoded separately and in sequence.
Zero-length segments lead to numerical errors and lead to extra complexity and wasted work (in the form of thread
allocation) for GPU flattening. We now detect and drop them at encoding time.
Currently, a path segment is considered zero-length if all of its control points (in local coordinates) are within EPSILON (1e-12) of each other. This may not be the desired behavior under transform.
Also since the length here isn't in terms of the actual arc length, this won't detect all curves that are within an EPSILON sized bounding box (which may have a larger distance between their control points).
For stroking purposes, the distance between the control points is what matters most as that's what's used to compute a curve's tangent.
This PR is based on #410
#303