Curve curve intersection tylers #219

tylers-epilog · 2022-04-01T19:15:48Z

This pull-request supersedes PR #199.

A note from @simoncozens original PR:
This borrow's lyon's curve-curve intersection algorithm.

@raphlinus I've tried to make this as numerically robust as I know how to. I'm open to discussion on what could be done better. I still think that there is great value in being able to get sections of the curves that fall within some distance from each other as I think that will help make my future path operation code much more robust.

* Added Epilog Laser/myself to the authors file * Added tests for real.rs and fixed bug * Renamed function for clarity, made public * Reworded comment for clarity * Fixed testing for curves that have multiple points at a t-value * Made testing multiple t values along curve easier * Fixed last commit * Added crate for comparing real values

* Added tests * Enabled intersections between lines and other curve types * Added tests for intersections between quadratic bezier curves * Moved ParamCurveBezierClipping into new file

* Added more flexibility to root finding to account for edge cases * Added function that checks if two curves perfectly overlap for some range * Added flags to curve intersection methods

* Now checking perpendicular fat line, fixed bugs

…oint

raphlinus

I've taken a skim through this. I haven't gone deep into the algorithm (and am not sure I'll have time to), but want to give my thoughts now.

I'm concerned about putting things in the public namespace. Some things (like the conversion to polynomial form) seem fine to me, as it's not hard to imagine other uses, and they are rather obviously correct.

Others, like the ParamCurveBezierClipping trait, don't feel like they belong as a trait in the public API. The intent of the ParamCurve trait family is that they could be implemented by clients of this library on other curves (with Euler spirals and related curves as an example of what I had in mind), but it seems unlikely that could be done numerically.

I'd also like a disclaimer that these methods are provisional and may change.

But as far as the substance, I'm happy to get this in. Thanks for your work in pushing it forward!

raphlinus · 2022-04-05T22:09:56Z

src/common.rs

+/// Find real roots of linear equation.
+///
+/// Return values of x for which c0 + c1 x = 0.
+pub fn solve_linear(c0: f64, c1: f64) -> ArrayVec<[f64; 1]> {


The return type is slightly odd. Is there a particular reason you didn't use Option<f64>? I'm sympathetic to the desire to be more parallel to the other cases.

I have it like this so that it's compatible with the return types in solve_t_for_x and solve_t_for_y. It also makes it so that it follows the same format as solve_quadratic and solve_cubic. I don't have any issues with changing it to use Option<f64>, but I'd like your thoughts on my reasoning first.

raphlinus · 2022-04-06T13:57:30Z

src/param_curve.rs

@@ -213,3 +214,20 @@ pub trait ParamCurveExtrema: ParamCurve {
        bbox
    }
 }
+
+/// A parameterized curve that can be used in the Bezier clipping algorithm
+pub trait ParamCurveBezierClipping: ParamCurve + ParamCurveDeriv + ParamCurveExtrema + ParamCurveArclen {


As mentioned in my cover message, I'd rather this not be a public trait, mostly because I think it's unlikely it could be reasonably implemented for curves other than beziers. It can be a trait internally, but the public API should be on concrete types.

I rearranged the code so that all of the ParamCurveBezierClipping is in one file, but I still can't make it private. Doing so causes issues with functions like curve_curve_intersections. I'm pretty new to rust, so maybe you can help me understand how to get around that issue.

@raphlinus What do you think? How could I go about doing this? I'm fairly new to rust, so maybe there's just something that I'm missing.

Also, I'm done making commits to fix the tests that failed, so they should all pass now.

raphlinus · 2022-04-06T13:58:09Z

src/param_curve.rs

+/// A parameterized curve that can be used in the Bezier clipping algorithm
+pub trait ParamCurveBezierClipping: ParamCurve + ParamCurveDeriv + ParamCurveExtrema + ParamCurveArclen {
+    /// Returns a line from the curve's start point to its end point
+    fn baseline(&self) -> Line {


This is a useful method and I can see putting it into ParamCurve. I think I've used the word "chord" in my writings, but "baseline" is also fine.

I moved it to ParamCurve, but I kept the wording because I think that's how Lyon refers to it.

raphlinus · 2022-04-06T14:02:49Z

src/point.rs

@@ -170,6 +170,37 @@ impl Point {
    pub fn is_nan(self) -> bool {
        self.x.is_nan() || self.y.is_nan()
    }
+
+    /// If we're comparing distances between samples of curves, our epsilon should


I'd rather this not go in the public API, mostly because "appropriate" is vaguely defined. I understand it's useful for improving numerical robustness, but is not "obviously correct."

I still have this concern, but am happy for it to be addressed with doc changes. Presumably it's "appropriate" for some specific use case, but that use case isn't defined. So either describe that use case or explicitly document what the function does.

Could we use ULPS here as in float-cmp? Or ApproxEqRatio?

raphlinus · 2022-04-06T14:03:32Z

src/point.rs

+        accuracy.max(max.abs() * 0.0000000001).max(f64::EPSILON)
+    }
+
+    /// Compare if two points are approximately equal


Same for this and the following method. If they go in the public API, the docstring must contain a rigorous definition.

raphlinus · 2022-04-19T18:17:22Z

Apologies for not being very responsive. I have a paper deadline coming up fast, so it's very hard for me to switch gears into something that requires deep attention. I am willing to merge this after light review, and of course the tests passing.

tylers-epilog · 2022-04-19T18:19:23Z

No worries. Take your time, I just want to make sure this doesn't totally get dropped. It looks like a failed some more tests, but they shouldn't be hard to fix.

simoncozens · 2022-06-24T20:59:52Z

src/lib.rs

@@ -84,6 +84,7 @@ mod bezpath;
 mod circle;
 pub mod common;
 mod cubicbez;
+mod curve_intersections;


Now we've got the algorithm down, do we actually have a public interface to curve intersections? It looks like I forgot to add one altogether. Should we just expose this module and its function, or add it as a method on the trait, or...?

I'm ok with this being a naked function.

raphlinus

Apologies for the delay, I've been juggling too many things.

There are a few things that need attention here, especially figuring out the public interface. I believe just making the submodule pub may be the best approach.

I don't want to block on multiple rounds or hold this up much longer. Whatever you feel comfortable addressing, please do, then anything I still feel uneasy about I'll try to do a followup PR. The documentation tries to speak with a very distinctive "voice," especially being careful about mathematical accuracy, and it's probably easier for me to tweak the language to fit that voice than back-seat drive in code reviews.

Thanks again.

raphlinus · 2022-07-12T17:58:35Z

src/common.rs

+/// Find real roots of linear equation.
+///
+/// Return values of x for which c0 + c1 x = 0.
+pub fn solve_linear(c0: f64, c1: f64) -> ArrayVec<f64, 1> {


I'm wondering if Option might be a cleaner type. It's essentially the same type, and far more idiomatic Rust, but on the other hand, maybe it's better to keep it as it is, for consistency with the other methods.

It would also be good to document the degenerate case, as is done for quadratic.

Personally, I like the idea of keeping it consistent. You're absolutely right that what I have is the equivalent of option though. I think it comes down to personal preference, but since you maintain this repo, I'll let you have the final say.

raphlinus · 2022-07-12T18:04:45Z

src/cubicbez.rs

@@ -238,7 +237,9 @@ impl CubicBez {
        })
    }

-    fn parameters(&self) -> (Vec2, Vec2, Vec2, Vec2) {
+    /// Get the parameters such that the curve can be represented by the following formula:


It bothers me slightly that this is in decreasing exponent, where, for example, solve_cubic is in increasing order. Not a dealbreaker though, if it's documented. (and I realize this is existing code, just thinking about it more carefully when we're making it public)

It also bothers me that these functions are inconsistent. Since they're both public though, wouldn't it be an issue to change them? Perhaps it's better to just document the exponent order?

raphlinus · 2022-07-12T18:06:43Z

src/point.rs

@@ -170,6 +170,37 @@ impl Point {
    pub fn is_nan(self) -> bool {
        self.x.is_nan() || self.y.is_nan()
    }
+
+    /// If we're comparing distances between samples of curves, our epsilon should


I still have this concern, but am happy for it to be addressed with doc changes. Presumably it's "appropriate" for some specific use case, but that use case isn't defined. So either describe that use case or explicitly document what the function does.

raphlinus · 2022-07-12T18:09:30Z

src/quadbez.rs

@@ -265,6 +280,11 @@ impl ParamCurveArclen for QuadBez {
        let c2 = 2.0 * c.sqrt();
        let ba_c2 = b * a2 + c2;

+        if a2.is_infinite() {
+            // The arclength is zero


This is the case where all control points are on top of each other? Would it be addressed by changing a < 5e-4 * c to a <= 5e-4 * c above? Also it would be great for this case to be tested.

a2 is purely calculated from the value of a, which means changing the code to a <= 5e-4 * c doesn't necessarily fix the issue. Also, the difference between < and <= is infinitesimally small and therefore doesn't really matter with real values. Since it's existing code that I didn't write, I don't want to change < to <= without more discussion since I don't entirely know the intent there.

Also, I'll add a test in my next commit.

raphlinus · 2022-07-12T18:21:30Z

src/lib.rs

@@ -84,6 +84,7 @@ mod bezpath;
 mod circle;
 pub mod common;
 mod cubicbez;
+mod curve_intersections;


I'm ok with this being a naked function.

raphlinus · 2022-07-12T18:23:53Z

src/curve_intersections.rs

+{
+    /// Find the time `t` at which the curve has the given x value
+    fn solve_t_for_x(&self, x: f64) -> ArrayVec<f64, 3>;
+    /// Find the time `t` at which the curve has the given x value


raphlinus · 2022-07-12T18:24:56Z

src/curve_intersections.rs

+    let b = vec2.x;
+    let c = -(a * l.start().x + b * l.start().y);
+
+    let len = (a * a + b * b).sqrt();


Would be faster to do .sqrt().recip() here. I checked .powf(-0.5), which I believe is used elsewhere, and that's surprisingly not optimized, seems to a call into the math library.

I'm surprised because this is well known to be optimizable (both in the famous Quake code and SIMD). But probably that's "fast-math" only, not IEEE compliant. Perhaps we should have a function in common for it.

raphlinus · 2022-07-12T18:34:07Z

src/curve_intersections.rs

+pub fn curve_curve_intersections<T: ParamCurveBezierClipping, U: ParamCurveBezierClipping>(
+    curve1: &T,
+    curve2: &U,
+    flags: CurveIntersectionFlags,


I think we have a problem here, the fn is publicly exported, but the flags type is not.

CurveIntersectionFlags is public. Or am I missing something?

See curve_intersections.rs line 259: pub struct CurveIntersectionFlags

raphlinus · 2022-07-12T18:34:42Z

src/curve_intersections.rs

+    //          curve 2:               O-----------O
+
+    // Use this function to get the t-value on another curve that corresponds to the t-value on this curve
+    #[inline]


inline seems dubious here given the size of the function. I'd just let the compiler do its thing.

raphlinus · 2022-07-12T18:37:17Z

src/curve_intersections.rs

+            &CubicBez::new((0.0, 1.0), (1.0, 1.0), (1.0, 1.0), (1.0, 0.0)),
+            1,
+            CurveIntersectionFlags::NONE, // Standard algorithm
+            0.,                           // Tightest possible accuracy


An accuracy of 0 seems wrong and basically impossible to meet. Generally the guarantee is that the function produces a result within that tolerance of the "exact" value. I'd prefer something like 1e-12 as a placeholder for "really accurate."

This is where that epsilon function comes in that you commented on before. The curve intersection function automatically uses the minimum recommended epsilon because 0 accuracy can never be achieved. However, I can see how this could be concerning from a numerical stance since the "minimum recommended epsilon" is fairly arbitrary. As such, what do you think would be the appropriate route to take? Maybe we should have a function called suggested_min_epsilon for floats, points, curves, and lines that the developer would have to call if they wanted the tightest accuracy?

raphlinus · 2022-07-12T18:51:01Z

One other thought on this. I'm seriously considering having an "extra" feature to hold things that are not generally used by most clients, and for which one might be worried about the compile time cost, and also with a lower stability/documentation guarantee than the core. This seems like a good candidate for that. Does that seem reasonable? It's likely I'll introduce that in a followup PR but if you think it's a great idea you can introduce it here, which would prevent things from bouncing around too much.

Under that feature gate, having "pub mod" feels like a good approach.

tylers-epilog · 2022-08-23T16:12:41Z

Hey @raphlinus, I'm so sorry for the delayed response. I promise my responses going forward won't take so long (although I am taking a week-long vacation soon).

I do think that making this an "extra" feature is a good idea, but I'm not entirely what all is involved in doing that other than having it be a "pub mod". Perhaps you can take care of that in a future PR.

I'm about to make a commit that will address several of your comments. The big thing still looming for me is the epsilon values for the intersections algorithm. Let me know what you think about having the suggested_min_epsilon functions.

raphlinus · 2022-08-23T16:17:26Z

Hi Tyler! Thanks for starting to respond. I did some experiments and analysis on this, and have found some definite problems. I've started working on my own intersection logic. An outline of my approach is in raphlinus/raphlinus.github.io#79. I have a draft implementation that does a good job on simple intersections, but I haven't yet implemented coincidence detection. As you can see from the linked issue, for robust path operations you need more than just a list of intersections, you need additional metadata about crossings and coincidence regions, so a significant part of the work is coming up with a new API.

I'm juggling a bunch of different stuff right now, but can try to prioritize making this into a proper PR, and I'd be more than happy for you to take a look.

tylers-epilog · 2022-08-23T16:56:30Z

I would be pretty excited to see your improvements. If you have time to tackle that, then great. If not, then no worries since I have other stuff that I'm working on. I'll go ahead and make those changes to this PR just in case we do decide to merge it in for some reason, but there's no obligation to use this code especially if you come up with a better method.

simoncozens and others added 11 commits September 16, 2021 09:54

Borrow Lyon’s implementation of curve-curve intersection

6e0ec0f

Make clippy less sad

f52accd

Squashed in curve-curve-intersection-cubic-quad-line:

d5d0be8

* Added tests * Enabled intersections between lines and other curve types * Added tests for intersections between quadratic bezier curves * Moved ParamCurveBezierClipping into new file

Squashed in curve-curve-intersection-overlap:

1375912

* Added more flexibility to root finding to account for edge cases * Added function that checks if two curves perfectly overlap for some range * Added flags to curve intersection methods

Squashed in curve-curve-intersection-perpendicular-fatline:

dc5b41a

* Now checking perpendicular fat line, fixed bugs

Split KEEP_ENDPOINT_INTERSECTIONS into individual flags for each endp…

f4cf191

…oint

Added ability to define how much accuracy is needed

84400ef

Removed real comparisons, only point comparisons remailn

7398c23

Reduced arclen epsilon to improve performance

cb5a672

Merge branch 'master' into curve-curve-intersection-tylers

6e993f9

raphlinus reviewed Apr 6, 2022

View reviewed changes

tylers-epilog added 7 commits April 7, 2022 09:48

Fixed merge with master

fc0bdaf

Fixed formatting

1a2cb70

Moved fn baseline to ParamCurve

c23c4ef

Organized code to reduce ParamCurveBezierClipping spaghetti

5bfbf1e

Added disclaimer for curve intersection functions

9f1c40f

Fixed running tests in release mode

531d538

Fixed clippy issues

5a6ac30

Replaced vec object with splice

7ddb666

simoncozens reviewed Jun 24, 2022

View reviewed changes

raphlinus reviewed Jul 12, 2022

View reviewed changes

Merge branch 'master' into curve-curve-intersection-tylers

64db1a8

Addressed pull request comments

099da3e

Cargo clippy and fmt fixes

ba180f7

richard-uk1 mentioned this pull request Apr 26, 2023

Proposal: Intersection trait #274

Open

raphlinus mentioned this pull request Apr 27, 2023

Tracking issue for boolean path operations #277

Open

simoncozens mentioned this pull request Jul 9, 2024

Segment/segment intersections #194

Open

Curve curve intersection tylers #219

Are you sure you want to change the base?

Curve curve intersection tylers #219

Conversation

tylers-epilog commented Apr 1, 2022

raphlinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphlinus commented Apr 19, 2022

tylers-epilog commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphlinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphlinus commented Jul 12, 2022 • edited Loading

tylers-epilog commented Aug 23, 2022

raphlinus commented Aug 23, 2022

tylers-epilog commented Aug 23, 2022

raphlinus commented Jul 12, 2022 •

edited

Loading