-
Notifications
You must be signed in to change notification settings - Fork 660
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
Bug fix to update freespace when bbx is set. #360
Conversation
Thanks for your contribution! Do you think it would be possible to add some basic unit tests as well? |
That's possible, I can add a new section in unit_tests.cpp and update CMakeLists.txt in src/testing. Is there anything else I should do to setup a basic unit test? |
Yes, either add a new .cpp file for a dedicated test (preferred) or add a section in unit_tests.cpp + update CMakeListst.txt. |
I added a dedicated test. While working on the test, I discovered that we should probably use a keyray iterator, not reverse_iterator, to iterate through keys from the origin to the end of the vector rather than starting at the end and moving towards the origin. I added this change to the PR as well. |
Thanks for the test and the change, indeed that makes sense. Sorry for the delay, I'm waiting for the CI builds to finish now. |
Looks like I deleted a line when refactoring the test to try to make the code cleaner. Hopefully the tests build/pass this time after my recent commit. I've had trouble building/running the tests on my end, and I can't trigger the CI builds on my own. So I'm sorry for this inconvenience. |
The last commit seemed to fail due to floating-point precision. I modified the test to use a larger epsilon when checking for floating point equality. |
To avoid going back and forth, could you compile and test locally on your machine before pushing? |
I found the source of the error, which was not in the new test but in testing.h. The testing macros did not put parentheses around the arguments, which results in incorrect computation when the arguments to the macros are an equation. I added parentheses around the arguments and ran the tests locally to ensure they pass. |
Thanks for the fixes! |
This pull request addresses #358. If the bounding box is set and the scan point is within the bounding box and within the max range (if set), then the cell containing the the scan point is added to occupied cells. Now, even if the scan point is outside the bounding box or beyond the max range, cells will be added to the free cells set. The end point is truncated to stop at the max range if the original scan point is beyond max range. Then, we iterate through the key ray and add cells to the free_cells list, breaking when we leave the bounding box.