-
Notifications
You must be signed in to change notification settings - Fork 701
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
attempt to fix infinite loop in isochrones generation #4547
Conversation
@munckymagik did you have time to give this code a try? |
@kevinkreiser just tried it out now with my failing example and it now returns a response 🎉 ✅ |
@@ -17,10 +17,22 @@ constexpr float METRIC_PADDING = 10.f; | |||
template <typename PrecisionT> | |||
std::vector<GeoPoint<PrecisionT>> OriginEdgeShape(const std::vector<GeoPoint<PrecisionT>>& pts, | |||
double distance_along) { | |||
// just the endpoint really | |||
if (distance_along == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you are snapped to the end of the edge we just return a segment of zero length at the end
src/thor/isochrone.cc
Outdated
// skip 0 length segments | ||
if (len == 0) | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not needed. if len
is 0 then it cant change suffix_len
which means it cant change the outcome of that if
below and get in there and do a divide by 0.
@@ -1,4 +1,4 @@ | |||
name: syntax | |||
name: lint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i prefer to call it like it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the additional comments!
fixes #4546
we may still want to protect bresenhams separately but perhaps this is a start