-
Notifications
You must be signed in to change notification settings - Fork 698
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
Fix cutting of response when traffic splits an edge into three parts #2943
Conversation
…t the breakpoint, not the speed validity.
|
||
// Regression is when breakpoint2 is set to 255, | ||
// but speed3 is not set to UNKNOWN - we would incorrectly | ||
// emit an additional 0-length slice in the API response |
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.
For my understanding, the case of speed3
or congestion3
being set while breakpoint2
is 255 is still an invalid state, but you're making the code more defensive to gracefully handle such an invalid state?
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.
Yeah - the old logic allowed for doing something in that case (breakpoint=255, but still emitting the third split). The code now correctly does not make the third split if breakpoint=255, so set values in speed3/congestion3 will be properly ignored.
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
feel free to ignore the non required windows build, theres a problem with CI rather than the code |
Issue
When traffic splits an edge into three parts, we were looking at the wrong field to determine if the third segment needs to exist. Edges are not often split this many times, and when the bug occurred, it resulted a zero-length geometry, so not a huge impact.
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?