Conversation
| // If the threshold is exceeded, we can stop looping only if it isOkayToReturnStartFeature OR the currentNode is not the originalNode | ||
| while (currentDistance < thresholdDistance || ( !isOkayToReturnStartFeature && isNodeOriginalNode )) { | ||
| // If the threshold is exceeded, we can stop looping only if the currentNode is not the originalNode | ||
| while (currentDistance < thresholdDistance || isNodeOriginalNode) { |
There was a problem hiding this comment.
What I am concerned about (but I am not certain that I am right) is that by removing the isOkayToReturnStartFeature then this while loop risks getting into an infinite loop.
Here is what I think can happen:
If the road is too short and is made up of just one edge AND ALSO the edge has more than one point then currentDistance will never exceed thresholdDistance AND isNodeOriginalNode will always be true.
If that can happen then the while loop will never be exited.
I can't find a test case where this happens without using really gimmicky inputs like putting the thresholdDistance to just a few feet.
If an infinite loop is a possibility though then having a counter that would kick the execution out of the while loop if it seems stuck (like if the while loop is executed some obscene number of times) would prevent an infinite loop.
What is the reason to get rid of isOkayToReturnStartFeature? It might be completely the right thing to do but I am not understanding the value in getting rid of it.
There was a problem hiding this comment.
The reason I got rid of it is because we weren't adding the coordinates to the result path that came from calling this method in the first place when isOkayToReturnStartFeature was false and so I personally felt like this method shouldn't be called if we weren't going to use the path it generated. Therefore, this method is now only called when isOkayToReturnStartFeature is true.
This is a good point though, I am going to look into this and get back to you on this.
There was a problem hiding this comment.
After looking at it further, I believe the isNodeOriginalNode in the outer while condition is actually redundant and safe to remove for the following reasons:
- the pre-loop guard lines 525-527 ensures the loop is always entered with currentDistance being less than the threshold distance and therefore the isNodeOriginalNode is never needed to force initial entry which was it's initial purpose
- Lines 655-658 checks the original node and ensures we complete at least one full edge traversal even if that traversal exceeds the threshold.
Let me know what you think about this update. I wasn't able to find any infinite loop risks when stepping through this but am open to further discussion
4 participants
Summary
A path was being generated for an Indiana CARS message because we were creating a point between the starting point and a node that was very far down the road after the road curved. This logic assumed that the two points we were using for interpolating the new point were connected.
Fix
The fix was to use the nearest node on the edge that the starting point is on so that we know for certain that the two points we are using for interpolation are linear.