-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add classes for collision checking #737
Add classes for collision checking #737
Conversation
|
||
CollisionChecker::~CollisionChecker() {} | ||
|
||
bool CollisionChecker::isCollisionFree( |
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.
Exceptions in C++ to my understanding are pretty heavy, could we have this isCollisionFree return an enum type with the type of failure rather than a bool?
The only package in navigation that uses exceptions heavily is DWB and I dont thin kwe should add more. Printing the exception text is only useful for debug, but that doesnt help much for autonomous systems where the handler of isCollisionFree may want to know why to react appropriately.
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.
I'll look and give some thought into returning enum types. The exceptions here are modified from those in DWB as you mentioned.
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.
IMO exceptions should be used when the local code can't recover from the error but someone up the stack may be able to. They shouldn't be used to propagate a normal return value. For example, if isCollisionFree may commonly return true or false, a return value is more appropriate. So, both exceptions and return values are appropriate, depending on the circumstances, and should be applied the right way.
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.
I could modify scorePose
to return a boolean and replace isCollisionFree
, but in this PR I was moving the code (including the exceptions) from the ObstacleFootprint
critic which returns a score for the pose requested. My intention was to replace the ObstacleFootprint
critic code with this class to avoid code duplication, which is why I thought to keep it as is.
} | ||
|
||
bool | ||
CollisionChecker::getRobotPose(geometry_msgs::msg::PoseStamped & global_pose) const |
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.
This should probably be moved to the Robot
class/util for getting pose and then this should be a consumer of that so we don't have raw TF everywhere
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.
I agree. We should move the one in Costmap2dROS
as well.
@@ -0,0 +1,209 @@ | |||
// Copyright (c) 2019 Intel Corporation |
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.
I like this alot. We should remove the same code from costmap_2d to use this so we dont have repeating sections to update if bugs are found later.
@@ -0,0 +1,52 @@ | |||
// Copyright (c) 2019 Intel Corporation |
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.
I see no issue with this, but I would believe that having this read the configuration file and it being a ROS agnostic helper to read from configuration file and getting as necessary may be more clear since somewhere else inside of costmap_2d we read the footprint and then publish it. Seems odd to have things running in the same process in the same node talking to each other over ROS to share data
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.
I believe in Costmap2DROS
, the footprint subscription is listening on a Polygon topic in order to update the footprint across all layers via setUnpaddedFootprint
in its callback. Since in this class we're listening on the published PolygonStamped topic and have a default callback to convert and store the robot oriented footprint, I see them as separate use cases.
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.
Mostly style recommendations, just one or two things that need to be fixed before merging.
|
||
~CollisionChecker(); | ||
|
||
double scorePose( |
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.
Maybe add a comment about the range and meaning of this score.
const geometry_msgs::msg::Pose2D & pose) | ||
{ | ||
try { | ||
if (scorePose(pose) < 0) { |
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.
A score < 0 means collision?
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.
Yes, this is according to the code in ObstacleFootprintCritic
double CollisionChecker::scorePose( | ||
const geometry_msgs::msg::Pose2D & pose) | ||
{ | ||
Costmap2D * costmap_; |
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.
costmap_
-> costmap
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.
Why have a costmap pointer? Maybe move the worldToMap
checks to CostmapSubscriber
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.
Because the intention of the CostmapSubscriber
was to convert the msg into the Costmap2D
object to access its API calls. I think it's better not to duplicate that code in the subscriber class
throw CollisionCheckerException(e.what()); | ||
} | ||
|
||
unsigned int cell_x, cell_y; |
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.
Looks like you don't actually need cell_x, cell_y but only checking if pose is within the map.
And since it's used on multiple places, maybe make a separate function isOnMap(x,y)
?
If so, maybe put costmap_
inside this function.
And ideally the whole check inside CostmapSubscriber
.
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.
To get the bool flag, no we don't need the cell_x, cell_y, though I think it is useful for debugging and it is part of the Costmap2D
API. I'm opposed to doing the check inside CostmapSubscriber
because that class should be general for anyone subscribing to the costmap topic, not just for collision checking
|
||
Footprint footprint_spec; | ||
unorientFootprint(footprint, footprint_spec); | ||
transformFootprint(pose.x, pose.y, pose.theta, footprint_spec, footprint); |
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.
Suggestion: auto footprint = getUnorientedFootprint(pose) and move code above to this function.
|
||
double CollisionChecker::pointCost(int x, int y) | ||
{ | ||
Costmap2D * costmap_ = costmap_sub_->getCostmap(); |
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.
costmap_
-> costmap
void CostmapSubscriber::toCostmap2D() | ||
{ | ||
if (!costmap_received_) { | ||
costmap_ = new Costmap2D( |
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.
Where do we delete
? alternatively use std::shared_ptr
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.
Or unique_ptr if it is not shared.
std::bind(&CostmapSubscriber::costmap_callback, this, std::placeholders::_1)); | ||
} | ||
|
||
Costmap2D * CostmapSubscriber::getCostmap() |
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.
Instead of providing the costmap to the clients, can we define an interface that provides what the clients really need, i.e. poseWithinMap(), poseCost(), etc.
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.
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.
Perhaps, though I would address this as a future feature of the world model in a different PR. Also, I'm not sure how much client-specific context we'd really like at the interface level. Calling something like poseCost()
is very context-specific and I'm not sure in the scope of the WorldModel. In my view, the WorldModel could be a plugin container for abstract map representations (of which costmap2D is one) of the environment from which clients can grab and use as needed on their end. We can continue this discussion
/* | ||
* Software License Agreement (BSD License) | ||
* | ||
* Copyright (c) 2017, Locus Robotics |
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.
Add:
Copyright (c) 2019, Intel Corporation
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.
This looks like mostly copied code. I wouldn't change the copyright.
{ | ||
public: | ||
CollisionChecker( | ||
rclcpp::Node::SharedPtr ros_node, |
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.
Should make this work for both lifecycle nodes and non-lifecycle nodes. Typical method would be to accept any required interface pointers.
{ | ||
public: | ||
CostmapSubscriber( | ||
rclcpp::Node::SharedPtr ros_node, |
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.
Same here. Should take interface pointers.
{ | ||
public: | ||
FootprintSubscriber( | ||
rclcpp::Node::SharedPtr ros_node, |
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.
Should work for lifecycle and non-lifecycle nodes.
void CostmapSubscriber::toCostmap2D() | ||
{ | ||
if (!costmap_received_) { | ||
costmap_ = new Costmap2D( |
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.
Or unique_ptr if it is not shared.
3f904d7
to
3314b10
Compare
I've addressed feedback, so this PR is open again for review. I'm adding the WIP to note that I am waiting on PR #793 to be merged so I can get rid of the tf_buffer to grab the robot pose and instead use the service call. Right now, I've hard-coded the global frames so that I don't have to pass in a node in the |
3314b10
to
9e4e8df
Compare
Open again for review now that #793 is merged and modifications were made. |
9e4e8df
to
7add0ba
Compare
#include "nav2_costmap_2d/footprint_subscriber.hpp" | ||
#include "nav2_util/get_robot_pose_client.hpp" | ||
#pragma GCC diagnostic push | ||
#pragma GCC diagnostic ignored "-Wpedantic" |
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.
Not gating this PR, but perhaps you can submit an issue to fix this upstream so we don't have to work around the warnings.
|
||
double scorePose( | ||
const geometry_msgs::msg::Pose2D & pose); | ||
bool isCollisionFree( |
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.
Why the line breaks for these methods? Makes for inconsistent formatting.
|
||
CollisionChecker::~CollisionChecker() {} | ||
|
||
bool CollisionChecker::isCollisionFree( |
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.
IMO exceptions should be used when the local code can't recover from the error but someone up the stack may be able to. They shouldn't be used to propagate a normal return value. For example, if isCollisionFree may commonly return true or false, a return value is more appropriate. So, both exceptions and return values are appropriate, depending on the circumstances, and should be applied the right way.
adding test cases move collision checker to nav2_costmap_2d
revert dwb_critics cmake changes
7add0ba
to
6a4b217
Compare
Closing because #889 is merged. |
This PR adds classes to abstract the footprint and costmap2D across the ROS2 topic interface and enable collision checking in other modules. The collision checking code is modified from the
ObstacleFootprintCritic
methodsBasic Info
Description of contribution in a few bullet points
nav2_msgs::msg::Costmap
in theCostmapPublisher
FootprintSubscriber
classCostmapSubscriber
classCollisionChecker
classCollisionChecker
line_iterator.hpp
innav2_util
(to be later removed fromdwb_critics
)Future work