Skip to content
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

Find tangents from point to cubic Bézier #288

Merged
merged 3 commits into from
Aug 22, 2024
Merged

Find tangents from point to cubic Bézier #288

merged 3 commits into from
Aug 22, 2024

Conversation

raphlinus
Copy link
Contributor

No description provided.

Copy link

@Keavon Keavon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

src/cubicbez.rs Outdated
///
/// Result is array of t values such that the line from the given point
/// to the curve evaluated at that value is tangent to the curve.
pub fn point_tangents(&self, p: Point) -> ArrayVec<f64, 4> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would tangents_though_point be a clearer name?

src/cubicbez.rs Outdated
@@ -348,6 +348,26 @@ impl CubicBez {
.filter(|t| *t >= 0.0 && *t <= 1.0)
.collect()
}

/// Find lines from the point to the curve tangent to the curve.
Copy link

@Keavon Keavon Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my initial few read-throughs of this sentence I had a hard time groking it. I can also take a shot at wording it, which might also be flawed, but maybe we can meet in the middle with something that's clearer to unbiased readers:

Find points on the curve where each one's tangent line passes through the given point p in space.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it a little shorter (it should be one line), but agree this is clearer. I think in my mind I was thinking about the line from the point to the curve, but thinking about it the other way is simpler because it's more apparent that you're getting both the tangent and the incidence.

@waywardmonkeys
Copy link
Contributor

@raphlinus i have heard that @Keavon might be interested in this being in a kurbo release… thoughts?

@waywardmonkeys waywardmonkeys added this to the August, 2024 Release milestone Aug 6, 2024
@raphlinus
Copy link
Contributor Author

I've fixed the merge conflict. @Keavon can you confirm this is still useful, and that review comments have been addressed to your satisfaction?

@Keavon
Copy link

Keavon commented Aug 16, 2024

@raphlinus This is still useful and the code/comments indeed look good!

It would also be useful to have a normal_to_point function as well, and add support for quadratic and linear segments in addition to cubic. (For linear, only normal is applicable and it will only return a single t value.) But I wouldn't suggest delaying this until those can be included, because this is useful in its own right.

For some context surrounding Graphite and Bezier-rs:

In Graphite, we're preparing to begin replacing some Bezier-rs functionality with Kurbo because it has better robustness, so this will be helpful to have. Our motivation for making Bezier-rs was twofold:

  • Needing more features/algorithms (normals/tangents to point is an example of one). But our scope also includes more sophisticated ones like Poisson-disk point sampling that would be out of scope for Kurbo. Rapid implementation mattered more than robustness/correctness in the short term for us, but now we're getting towards desiring more robustness. Kurbo's performance is likely also considerably better and that's starting to matter to us a lot more too.
  • Needing to represent the data structure as "manipulator groups" (anchor + that anchor's 0/1/2 handles) rather than SVG d path command style segments (0/1 outward handle, 0/1/ inward handle, and anchor) so our tools didn't need to pay the performance penalty for translating between Kurbo SVG-style and our tools' manipulator group style; but this requirement has changed and is no longer a factor, which means Kurbo would be just as efficient for us, leaving only point 1 above regarding features. Hence, the desire to have these tangent/normal to point algorithms implemented.

Maintenance and the future of Bezier-rs is still up in the air but I'm envisioning it as a possible wrapper for Kurbo that could use those algorithms that exist and augment it with our additional features that Kurbo currently lacks (or always will), such as the Poisson-disk point sampling linked above. Downside: Bezier-rs would no longer be a zero-dependency crate. Upside: we can reduce our maintenance burden through deleting a lot of our code by wrapping Kurbo's algorithms, provide better robustness and performance, and probably continue to use Bezier-rs as a testbed or incubator for more complex algorithms instead of needing to officially abandon our maintenance for the crate if we fully switched to Kurbo in Graphite.

Copy link

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Approving this, as @Keavon doesn't have the rights to do this, but has confirmed usefulness)

@waywardmonkeys
Copy link
Contributor

Unless I hear otherwise or someone else merges this, I will merge this during or after office hours today.

@raphlinus raphlinus added this pull request to the merge queue Aug 22, 2024
Merged via the queue into main with commit 987b6ff Aug 22, 2024
15 checks passed
@raphlinus raphlinus deleted the tangent branch August 22, 2024 04:05
@raphlinus
Copy link
Contributor Author

I'm merging it now. Sounds like there's a good discussion to be had about future coevolution of bezier-rs and kurbo. The poisson disk feature looks quite interesting, but as you say it's probably not in scope for kurbo.

@waywardmonkeys
Copy link
Contributor

I'm very excited about bezier-rs moving some things into Kurbo!

@Keavon
Copy link

Keavon commented Aug 22, 2024

I should go through and create a comparison chart about which algorithms each of our libraries implement in order to focus attention towards those which may be useful in Kurbo. But I need to emphasize that our algorithms are somewhat janky and probably aren't fast or robust enough for the standards of Kurbo. Though it would still be a useful way to evaluate what may be worth reimplementing in Kurbo with better attention towards those fine details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants