Add line_intersection to compute point or segment intersection of two Lines.

[michaelkirk]

• I agree to follow the project's code of conduct.
• I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

---

Fixes #288

I pulled this out of my Geometry Graph work (which is getting close, I swear!).

[michaelkirk]

As well as these tests, my main confidence for correctness comes transitively from using this code and passing the JTS suite of "Relate" tests.

[frewsxcv]

short on time this evening so I can't review the whole thing! but could you add a new entry in the index for the geo crate root documentation (that was recently added)?

[urschrei]

Also pushed for time so haven't thoroughly reviewed, but this lgtm.
Am I interpreting some of these test comments correctly in that the tests are correctly passing in geo, but have failed in JTS in the past? That's interesting (in a good way).

[michaelkirk]

short on time this evening so I can't review the whole thing! but could you add a new entry in the index for the geo crate root documentation (that was recently added)?

Thanks for calling this out. Done!

I slightly reorganized some items into a new "Topology" subheading. WDYT?

[michaelkirk]

Am I interpreting some of these test comments correctly in that the tests are correctly passing in geo, but have failed in JTS in the past? That's interesting (in a good way).

I have perhaps confused you by my commenting convention. Those tests are mostly just ported from the JTS suite. I tried to capture the comments from those tests to capture at least a little context. I tried to indicate which lines were quoted directly from JTS by prefixing them with >, as in:

/// Based on JTS test `testLeduc_1`
///
/// > Test involving two non-almost-parallel lines.
/// > Does not seem to cause problems with basic line intersection algorithm.

So JTS worked out all the kinks, and the geo code also passes these ported tests because the georust code is ported from the modern JTS code.

[michaelkirk]

short on time this evening

Also pushed for time so haven't thoroughly reviewed

paging @rmanoka 🥺

[frewsxcv]

Looks good! Just commented on some nonblocking minor stuff

[frewsxcv]

Suggested change

	//! - **[`line_intersection`](algorithm::line_intersection::line_intersection)**: Calculates the
	//! - **[`LineIntersection`](algorithm::line_intersection::LineIntersection)**: Calculates the

[michaelkirk]

The reason it's capitalized like this is because line_intersection is a free function, not a trait. I should have called this out explicitly, because I went back and fourth on it.

My first thought was on one end of the spectrum: "eventually we want everything to intersect with everything else, so I should just introduce a super generic intersection trait" like:

trait Intersection<T> {
    type Output;
    fn intersection(&self, other: T) -> Self::Output
}

impl Intersection<Line<F>> for Line<F> {
    type Output = Option<LineIntersection<F>>
    fn intersection(&self, other: Line<F>) -> Self::Output {
        ...
    }
}

But since there aren't any near-term plans to introduce impls other than Intersection<Line> for Line, I thought suggesting that we support a generic concept of Intersection might just end up confusing people.

So then I thought maybe a more specific trait:

trait LineIntersection {
    fn line_intersection(&self, other: Line<T>) -> LineIntersection<T>;
}

impl LineIntersection<T> for Line<T> {
    fn line_intersection(&self, other: Line<T>) -> LineIntersection<T>;
        ...
    }
}

I'd be OK with this, but I think I would only ever want to have one thing (Line) implement that trait... so it seems like a lot of machinery, and if we did eventually introduce a more generic Intersection trait, this trait would likely become wholly redundant.

So that's why I eventually settled on just a bare non-trait function, and why it's capitalized that way in the docs. It seemed like a stable way to expose the functionality that would be less likely to surprise people.

If we did eventually add intersections for more than just lines, I'd be in favor of adding a generic Intersecion<T> trait.

WDYT?

[frewsxcv]

This check could go before the q_rect assignment above, if we wanted to save a few calls. Breaks the flow a little so it's a tradeoff 🤷🏻

[frewsxcv]

I played around with formatting these conditional checks a different way:

    Some(match (p1q1p2, p1q2p2, q1p1q2, q1p2q2) {
        (true, true, _, _) => LineIntersection::Collinear { intersection: q },
        (_, _, true, true) => LineIntersection::Collinear { intersection: q },
        (true, false, true, false) if q.start == p.start => {
            LineIntersection::SinglePoint {
                intersection: q.start,
                is_proper: false,
            }
        }
        (true, _, true, _) => {
            LineIntersection::Collinear {
                intersection: Line::new(q.start, p.start),
            }
        }
        ...
        _ => return None,
    })

As always, take it or leave it. Was just experimenting

[michaelkirk]

It's easier to see the patterns when laid out as pattern matching -- good call. I've done that (and introduced a couple helper functions for brevity).

[frewsxcv]

Is this the same as:

    let min_x0 = p.start.x.min(p.end.x);

https://docs.rs/num/0.4.0/num/trait.Float.html#tymethod.min

[michaelkirk]

Indeed it is! I've updated to use that, and it's much more concise.

[michaelkirk]

I think I've addressed all your feedback @frewsxcv. I'll wait another day before merging incase you or anyone else wants to re-review.

[rmanoka]

@michaelkirk Looks great! I'm a bit pressed for time this week, but can take a look at the details of the core logic next week.

[frewsxcv]

Lgtm, trait or plain ol function is fine with me

…

On Wed, Mar 10, 2021, 12:13 PM Michael Kirk ***@***.***> wrote: I think I've addressed all your feedback @frewsxcv <https://github.com/frewsxcv>. I'll wait another day before merging incase you or anyone else wants to re-review. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#636 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADFWP6HTNP5ZCYJYZJQNBLTC6LBVANCNFSM4YZ7N5RA> .

[michaelkirk]

@michaelkirk Looks great! I'm a bit pressed for time this week, but can take a look at the details of the core logic next week.

@rmanoka: No problem - would you prefer that I leave this open until then?

[rmanoka]

@michaelkirk Pls feel free to merge it.

[michaelkirk]

bors r=frewsxcv

(rebased and squashed the review commits)

[bors]

Build succeeded:

• ci result