-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Check if the tolerance circle is feasible when validating goals for path planning. #5593
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
Check if the tolerance circle is feasible when validating goals for path planning. #5593
Conversation
Signed-off-by: Abhishekh Reddy <[email protected]>
Signed-off-by: Abhishekh Reddy <[email protected]>
Signed-off-by: Abhishekh Reddy <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: Abhishekh Reddy <[email protected]>
I think making |
Thanks for adjusting the implementation, I had a score of things I was going to say but this implementation is much better and removes the redundant checks.
Such as? |
I don't have any specific cases right now, but I find this function as a more wider/approximate version of |
I think we should leave it in the goal manager for now, but keep an eye on that in the future in case that turns out to be useful as well! I think it creates a bit of complexity to have a Node itself creating and calling other Nodes to collision check in a region. That seems ... odd. Definitely something we could do, but we have other places in the goal manager already we call isNodeValid so I think this fits there perfectly fine and doesn't make first-time developers brains hurt with thinking about different instances of the same object class ... within the implementation details of a particular node class. Sometimes we make choices that are for the casual developer's benefit :-) |
Yea I agree with that as well, we'll keep it this way and I'll iron out the rest. |
Signed-off-by: Abhishekh Reddy <[email protected]>
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.
Otherwise LGTM - anything else we should consider before merging?
Signed-off-by: Abhishekh Reddy <[email protected]>
It looks good to me too, we can merge after the CI passes. |
…ath planning. (ros-navigation#5593) * Implemented goal tolerance validity check Signed-off-by: Abhishekh Reddy <[email protected]> * Fixed description for getCoords function Signed-off-by: Abhishekh Reddy <[email protected]> * Added a test with lower tolerance for zone validity checking Signed-off-by: Abhishekh Reddy <[email protected]> * Updated tolerance check function implementation Signed-off-by: Abhishekh Reddy <[email protected]> * Updated isZoneValid function Signed-off-by: Abhishekh Reddy <[email protected]> * Updated isZoneValid function Signed-off-by: Abhishekh Reddy <[email protected]> --------- Signed-off-by: Abhishekh Reddy <[email protected]>
Basic Info
Description of contribution in a few bullet points
getCoords
in Node2D class.Description of documentation updates required from your changes
--
Description of how this change was tested
Future work that may be required in bullet points
--
For Maintainers:
backport-*
.