Skip to content
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

separate collision_object creation from publishing #55

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mlautman
Copy link
Contributor

@mlautman mlautman commented Aug 12, 2019

Git seems to have messed up the diff. Any suggestions to avoid this?

@mlautman
Copy link
Contributor Author

@davetcoleman

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this more general!

* \param pose - location of center of block
* \param name - semantic name of MoveIt collision object
* \param size - height=width=depth=size
* \param color to display the collision object with
* \return true on sucess
**/
moveit_msgs::CollisionObject createCollisionBlock(const geometry_msgs::Pose& block_pose, const std::string& name,
double block_size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible for the CollisionObject msgs type to support color as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we would have to change the msg

@@ -323,33 +323,40 @@ class MoveItVisualTools : public rviz_visual_tools::RvizVisualTools
const rviz_visual_tools::colors& color = rviz_visual_tools::GREEN);

/**
* \brief Create a MoveIt Collision block at the given pose
* \brief Create/Publish a MoveIt Collision block at the given pose
* \param pose - location of center of block
* \param name - semantic name of MoveIt collision object
* \param size - height=width=depth=size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to block size

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -323,33 +323,40 @@ class MoveItVisualTools : public rviz_visual_tools::RvizVisualTools
const rviz_visual_tools::colors& color = rviz_visual_tools::GREEN);

/**
* \brief Create a MoveIt Collision block at the given pose
* \brief Create/Publish a MoveIt Collision block at the given pose
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd rather you copy this doxygen for reach function. it seems these two functions due fairly different things, and the \brief should explain that difference better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -323,33 +323,40 @@ class MoveItVisualTools : public rviz_visual_tools::RvizVisualTools
const rviz_visual_tools::colors& color = rviz_visual_tools::GREEN);

/**
* \brief Create a MoveIt Collision block at the given pose
* \brief Create/Publish a MoveIt Collision block at the given pose
* \param pose - location of center of block
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block_pose

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* \param point1 - top left of rectangle
* \param point2 - bottom right of rectangle
* \param name - semantic name of MoveIt collision object
* \param color to display the collision object with
* \return true on sucess
**/
moveit_msgs::CollisionObject createCollisionObject(const Eigen::Vector3d& point1, const Eigen::Vector3d& point2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be createCollisionCuboid right? Even better: createCollisionCuboidMsg

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -358,6 +365,10 @@ class MoveItVisualTools : public rviz_visual_tools::RvizVisualTools
* \param color to display the collision object with
* \return true on sucess
**/
moveit_msgs::CollisionObject createCollisionObject(const Eigen::Isometry3d& pose, double width, double depth,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comments:
rename function to include Cuboid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

double depth, double height,
const std::string& name)
{
geometry_msgs::Pose pose_msg = tf2::toMsg(pose);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typically in *_visual_tools, we use the built-in converter helper function rather than tf2, could you use that? e.g. convertPose()

same below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@mlautman mlautman force-pushed the create-and-pub-separately branch 2 times, most recently from e626338 to 6c354a5 Compare August 18, 2019 09:45
@mlautman
Copy link
Contributor Author

@davetcoleman Done

@davetcoleman
Copy link
Member

Ive just retriggered travis, but i think you need to rebase on master with the travis fix you just made?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants