Skip to content

Handle line segments intersecting at their endpoints. The algorithm I - #378

Draft
dabreegster wants to merge 1 commit into
mainfrom
less_scary_geom_code
Draft

Handle line segments intersecting at their endpoints. The algorithm I#378
dabreegster wants to merge 1 commit into
mainfrom
less_scary_geom_code

Conversation

@dabreegster

Copy link
Copy Markdown
Collaborator

ported ages ago doesn't handle this, and somehow I didn't notice until
b2519e3.

@michaelkirk, any thoughts on this incremental improvement? I wanted to take this opportunity to rely on a better tested library for line segment intersection, but all I found was https://crates.io/crates/line_intersection. I gave it a shot at https://github.com/dabreegster/abstreet/blob/21eb14ba52afd549f0d6d0732ec3fe405a19869e/geom/src/line.rs#L75, but it said the line segments in the unit test were co-linear, which doesn't seem correct to me.

I'll keep looking around for ways to reduce the core code in geom. Ideally it'd mostly just wrap other crates that're better tested.

ported ages ago doesn't handle this, and somehow I didn't notice until
b2519e3.

@michaelkirk michaelkirk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

- /// Undefined if the two lines have more than one intersection point 

Is there a clear behavior for collinear lines now?

It seems like in the case that their endpoints overlap we have clear behavior, but what if their overlap is more than a point?

Comment thread geom/src/line.rs
if self.pt1() == other.pt2() {
return Some(self.pt1());
}
if self.pt2() == other.pt1() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

equality is reflexive, so this one check is redundant ( the subsequent p2 == p2 check should stay though)

@michaelkirk

Copy link
Copy Markdown
Collaborator

but it said the line segments in the unit test were co-linear, which doesn't seem correct to me.

Oh, hmm... you mean the lines (0.0, 0.0) -> (1.0, 1.0) and (2.0, 2.0) -> (1.0, 1.0)?

Those indeed are collinear. Does their code not work with collinear lines or something?

@dabreegster

Copy link
Copy Markdown
Collaborator Author

Those indeed are collinear. Does their code not work with collinear lines or something?

It returns collinear in this case, but I'd expect just the endpoint (1, 1). Maybe it depends if the line segment endpoints are open or closed; the docs use [0, 1] notation, so I'm expecting closed.

It seems like in the case that their endpoints overlap we have clear behavior, but what if their overlap is more than a point?

I'll add some more tests and figure out how to implement. I would expect None as the result. But honestly, it's maybe time to audit all of the callers of this method and either return the more nuanced enum of different types of intersections, or indeed keep the 0 or 1 point result.

@michaelkirk

michaelkirk commented Nov 19, 2020

Copy link
Copy Markdown
Collaborator

I mentioned this in Slack, but depending on how urgent this is I'm hoping the geo crate will have a line intersection API merged "soon" (1-2 weeks?).

@dabreegster

Copy link
Copy Markdown
Collaborator Author

I might hold off on this, then -- thanks! No rush; not planning to work on intersection geometry anytime soon again

@dabreegster
dabreegster marked this pull request as draft November 19, 2020 19:41
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.

3 participants