-
Notifications
You must be signed in to change notification settings - Fork 800
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
Minor fix in getClosestPosition to avoid wrong indices #306
base: master
Are you sure you want to change the base?
Minor fix in getClosestPosition to avoid wrong indices #306
Conversation
Thank you very much. Could you check if you could add this scenario as a unit test as well? This test should give some inspiration. |
Thanks @maximilianwulf. I'll add the unit test and will ask your re-review. |
Hi @maximilianwulf, thanks for asking for the test, it made me realize the solution I was proposing is not solving the real problem. I pushed a test ( It breaks when trying to clip a point to the lower bounds and clips the indices to 200 instead of 199 (I attached the output below). It doesn't break with the settings in I ran out of places to look at, but if you have any suggestions I can go further. Output: /home/matias/catkin_ws/src/grid_map/grid_map_core/test/GridMapTest.cpp:447: Failure
Expected equality of these values:
expectedIndex.y()
Which is: 199
insideIndex.y()
Which is: 200
closestInsidePosition 1.9999999999999998
-1.9999999999999998
/home/matias/catkin_ws/src/grid_map/grid_map_core/test/GridMapTest.cpp:450: Failure
Value of: isInside
Actual: false
Expected: true
position is:
1.9999999999999998
-1.9999999999999998
index is:
0
200
/home/matias/catkin_ws/src/grid_map/grid_map_core/test/GridMapTest.cpp:493: Failure
Expected equality of these values:
expectedIndex.y()
Which is: 199
insideIndex.y()
Which is: 200
closestInsidePosition 0
-1.9999999999999998
/home/matias/catkin_ws/src/grid_map/grid_map_core/test/GridMapTest.cpp:496: Failure
Value of: isInside
Actual: false
Expected: true
position is:
0
-1.9999999999999998
index is:
100
200
/home/matias/catkin_ws/src/grid_map/grid_map_core/test/GridMapTest.cpp:516: Failure
Expected equality of these values:
expectedIndex.x()
Which is: 199
insideIndex.x()
Which is: 200
closestInsidePosition-1.9999999999999998
1.9999999999999998
/home/matias/catkin_ws/src/grid_map/grid_map_core/test/GridMapTest.cpp:520: Failure
Value of: isInside
Actual: false
Expected: true
position is:
-1.9999999999999998
1.9999999999999998
index is:
200
0
/home/matias/catkin_ws/src/grid_map/grid_map_core/test/GridMapTest.cpp:539: Failure
Expected equality of these values:
expectedIndex.x()
Which is: 199
insideIndex.x()
Which is: 200
closestInsidePosition-1.9999999999999998
0
/home/matias/catkin_ws/src/grid_map/grid_map_core/test/GridMapTest.cpp:543: Failure
Value of: isInside
Actual: false
Expected: true
position is:
-1.9999999999999998
0
index is:
200
100
/home/matias/catkin_ws/src/grid_map/grid_map_core/test/GridMapTest.cpp:563: Failure
Expected equality of these values:
expectedIndex.x()
Which is: 199
insideIndex.x()
Which is: 200
closestInsidePosition-1.9999999999999998
-1.9999999999999998
/home/matias/catkin_ws/src/grid_map/grid_map_core/test/GridMapTest.cpp:564: Failure
Expected equality of these values:
expectedIndex.y()
Which is: 199
insideIndex.y()
Which is: 200
closestInsidePosition-1.9999999999999998
-1.9999999999999998
/home/matias/catkin_ws/src/grid_map/grid_map_core/test/GridMapTest.cpp:567: Failure
Value of: isInside
Actual: false
Expected: true
position is:
-1.9999999999999998
-1.9999999999999998
index is:
200
200 |
Thank you, I will import it. |
Hi, this is a minor fix when using
getClosestPosition()
with positions outside the grid map.The actual implementation used the
std::epsilon
value to avoid this but I still observed some issues when callinggetIndex()
. I used the same factor of 10 for epsilon used here and seems to do the trick (I guess that just using epsilon is still too tight)