fix: correct loop increment in move-limiting logic#12129
fix: correct loop increment in move-limiting logic#12129nisshchayarathi wants to merge 2 commits intoopenstreetmap:developfrom
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Note that tests also passed before, so this specific functionality is not tested at all - so tests passing does not indicate much |
|
@matkoniecz the var j is defined and in for loop i is getting incremented obviously it needs to be fixed |
|
Yes, but fix needs to be tested. In some cases multiple bugs were masking each other, and fixing just one of them would make situation worse. Also, testing may reveal more problems with code here. If such mistake exists then surely something went quite wrong. |
tyrasd
left a comment
tyrasd
left a comment
There was a problem hiding this comment.
This does seem like an actual mistake in the for loop variable. See 9b200cf#diff-e1e4ae664eecbe2722b4542db88173ce69c14f939d89784c0ad32a61dc0d6451R296
Luckily, there are not too many cases where hits will have more than one entry, so that's probably why it wasn't causing much troubles in practice.
However, as mentioned by @matkoniecz: we should definitely add a unit test that checks that the method is working as intended. For this we need to create a test case where movedPath and unmovedPath meet in two points.
tyrasd
added
bug
| }); | ||
| }); | ||
|
|
||
| it('limits delta when movedPath and unmovedPath intersect twice', function() { |
There was a problem hiding this comment.
This test does not fail in the current implementation (without the bug fix). Please construct a test that actually fails in the current development branch, and is fixed in the PR.
There was a problem hiding this comment.
sir I checked
it fails without the fix: the buggy loop ,increments the outer i inside the inner loop, so it skips the later intersection object that actually constrains the x movement.
There was a problem hiding this comment.
If I am missing something please suggest
3 participants
Fixes #12128
Description
This PR fixes an issue in the move-limiting logic where the inner loop was incrementing the wrong variable, leading to incorrect iteration behavior.
Changes
Testing