search

georust / geo

Geospatial primitives and algorithms for Rust
https://crates.io/crates/geo
Other
1.49k stars 194 forks source link

Haversine Closest Point panic on intersection #1084

Closed ianthetechie closed 6 months ago

ianthetechie commented 9 months ago

Given a LineString and one of the points of the line, linestring.haversine_closest_point(&point) will panic due to a todo in the code claiming that this does not happen.

Here is the offending code: https://github.com/georust/geo/blob/d95daf8bae0ff63557b283e01ddede278902323f/geo/src/algorithm/haversine_closest_point.rs#L154C55-L154C55

I'd be happy to open a PR and include a regression test, but I just wanted to make sure I wasn't missing something obvious.

gibbz00 commented 9 months ago

Match arms that should not be triggered usually use unimplemented! over todo!, so my two cents are that this is in fact a todo 🙂

urschrei commented 9 months ago

@bbetov-clgx blame says this is your todo. wdyt?

ianthetechie commented 9 months ago

@gibbz00 I wasn't going to come in all guns blazing, but yeah, that was actually my first thought 🎃

bbetov commented 9 months ago

@urschrei Indeed, it is a todo!. I haven't had a chance to come back to it to finish, but if the OP already has a solution, great!

urschrei commented 9 months ago

@ianthetechie If you have a PR, we'd be happy to have it.

fegies commented 8 months ago

Hi. Is there any update on this? A project of mine was recently bitten by this, so I'd be willing to put a quick PR if there is not one already in progress.

ianthetechie commented 8 months ago

Not at the moment; I realized that I didn’t need this formula (Cartesian was OK for very close distances) for our use case and I haven’t gotten back to this yet.Asking the obvious question, why would it be anything other than the intersection variant?There’s also a similar TODO waiting to blow up in the same file IIRC.11/22/23 03:43, Felix Giese @.***> 작성: Hi. Is there any update on this? A project of mine was recently bitten by this, so I'd be willing to put a quick PR if there is not one already in progress.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

fegies commented 8 months ago

Well, there might be the degenerate case of multiple points of the Linestring actually intersecting. So the correct behavior in that case would be to return Closest::Indeterminate.

I'll prepare a PR with a repro and a fix

ianthetechie commented 8 months ago

Valid point ;)11/22/23 04:00, Felix Giese @.***> 작성: Well, there might be the degenerate case of multiple points of the Linestring actually intersecting. So the correct behavior in that case would be to return Closest::Indeterminate. I'll prepare a PR with a repro and a fix

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

fegies commented 8 months ago

PR ready for review: https://github.com/georust/geo/pull/1119