-
Notifications
You must be signed in to change notification settings - Fork 682
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(freespace_planning_algorithms): throw error when no waypoints stored #2505
fix(freespace_planning_algorithms): throw error when no waypoints stored #2505
Conversation
Signed-off-by: Hirokazu Ishida <h-ishida@jsk.imi.i.u-tokyo.ac.jp>
Signed-off-by: Hirokazu Ishida <h-ishida@jsk.imi.i.u-tokyo.ac.jp>
Codecov ReportBase: 11.61% // Head: 11.66% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2505 +/- ##
==========================================
+ Coverage 11.61% 11.66% +0.04%
==========================================
Files 1319 1319
Lines 92105 92174 +69
Branches 24416 24461 +45
==========================================
+ Hits 10700 10752 +52
Misses 70234 70234
- Partials 11171 11188 +17
*This pull request uses carry forward flags. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
@HiroIshida Thank you for this PR.
Question: What will happen when the solver fails to compute the waypoint? Before it results in an invalid index access exception, and with this PR the planner still dies with another exception?
...g/freespace_planning_algorithms/include/freespace_planning_algorithms/abstract_algorithm.hpp
Outdated
Show resolved
Hide resolved
...g/freespace_planning_algorithms/include/freespace_planning_algorithms/abstract_algorithm.hpp
Outdated
Show resolved
Hide resolved
And although the error is simple, typically user need to compile it with debug flag and have to use debugger to detect this error. But if we throw a error with explicit message, user can easily detect the reason of bug without any debugger. |
Signed-off-by: Hirokazu Ishida <h-ishida@jsk.imi.i.u-tokyo.ac.jp>
Does this operation happen accidentally? For example, in the case of path search failure? |
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
…red (autowarefoundation#2505) * fix(freespace_planning_algorithms): throw error when no waypoints stored Signed-off-by: Hirokazu Ishida <h-ishida@jsk.imi.i.u-tokyo.ac.jp> * test: continue if plan is failed Signed-off-by: Hirokazu Ishida <h-ishida@jsk.imi.i.u-tokyo.ac.jp> * Return waypoints even when solution is not found Signed-off-by: Hirokazu Ishida <h-ishida@jsk.imi.i.u-tokyo.ac.jp> * ci(pre-commit): autofix --------- Signed-off-by: Hirokazu Ishida <h-ishida@jsk.imi.i.u-tokyo.ac.jp> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Takamasa Horibe <horibe.takamasa@gmail.com>
Description
1. safer access to waypoints
Current implementation of
compute_length
is problematic because it can invalidly access vector element and cause the following error.This is mainly happens when solver failed to solve, which means no waypoints are set. So, also to fix the root cause, I add check if the waypoints is empty in
getWaypoints
function.2. update test
related to 1, I updated the test so that if planning fails, don't call
getWaypoints
note
This may related to #2439 but if invalid access in
compute_length()
causes the error, that's mean it takes more than 10 seconds to solve a single problem by astar, and the chance is quite low. So, probably this PR will not fix #2439Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.