cleanup robot interaction code#1194
Conversation
moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_display.cpp
Outdated
Show resolved
Hide resolved
5e4f600 to
e7063e6
Compare
85f41e4 to
0522f39
Compare
0522f39 to
4040f0d
Compare
davetcoleman
left a comment
There was a problem hiding this comment.
This looks ok to me but I'd prefer another reviewer as I don't have much experience in this part of the code. Acorn made these components very complex :-/
| static std::string fixName(std::string name); | ||
|
|
||
| public: | ||
| // DEPRECATED FUNCTIONS. |
| } | ||
|
|
||
| // value for normalized quaternion | ||
| inline constexpr double qv() |
There was a problem hiding this comment.
better function name. also why isn't this a constant variable instead of a function?
| addBackgroundJob(boost::bind(&MotionPlanningDisplay::publishInteractiveMarkers, this, false), | ||
| "publishInteractiveMarkers"); | ||
| addBackgroundJob(boost::bind(&MotionPlanningDisplay::publishInteractiveMarkers, this, false), "publishInteractiveMark" | ||
| "ers"); |
There was a problem hiding this comment.
Is this really according to the code style? It seems odd. =]
There was a problem hiding this comment.
Thanks for pointing this out. This has improved since #1214.
|
|
||
| MotionPlanningFrame::~MotionPlanningFrame() | ||
| { | ||
| delete ui_; |
There was a problem hiding this comment.
Is MotionPlanningFrame copyable or movable? If it is either, this could lead to double deletes if the ui_ pointer ends up being copied (the default move constructor also copies pointers, if it exists). It would be safer to wrap the object in a unique_ptr.
There was a problem hiding this comment.
At least it is not intended to be copied ;-)
There was a problem hiding this comment.
Right, then I submit to your judgment wether we should keep it like this or put it in a std::unique_ptr :)
There was a problem hiding this comment.
Nevermind, just noticed the deleted copy constructor :)
There was a problem hiding this comment.
I added it due to your comment.
| MotionPlanningParamWidget::~MotionPlanningParamWidget() | ||
| { | ||
| if (property_tree_model_) | ||
| delete property_tree_model_; |
There was a problem hiding this comment.
Same as above, if the MotionPlanningParamWidget is copyable or movable, this could lead to double frees.
|
I did my best to review what I can, but I don't have a clear picture of the interaction between the components, or their lifetimes. So I'm afraid my comments are rather general. |
... to get rid of rviz warning: ros.rviz.quaternions: Interactive marker 'EE:goal_ra_tool_mount' contains unnormalized quaternions.
... to avoid error message: "No robot state or robot model loaded"
…nsMap As RobotInteraction and it's KinematicsOptionsMap is stored in constructor, there is no need to update this map. This finishes 4e9b8a1 (moveit/moveit_ros#422).
4040f0d to
dfce54f
Compare
davetcoleman
left a comment
There was a problem hiding this comment.
I built and tested panda in Rviz with this locally
| Q_OBJECT | ||
|
|
||
| public: | ||
| MotionPlanningFrame(const MotionPlanningFrame&) = delete; |
There was a problem hiding this comment.
Ah, I see this now makes sure it can't get copied. The move constructor is also automatically deleted in this scenario if I'm not mistaken, so that makes the delete in the destructor always safe :)
Tracking down
shared_ptrleaks, I stumbled over several minor issues inrobot_interactioncode for rviz plugins. For review, you best look at individual commits instead of the overall bulk.Please keep commit history.