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.

Sign up for GitHub

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

Jump to bottom

cleanup robot interaction code #1194

Merged
merged 9 commits into from
Nov 22, 2018
Merged

cleanup robot interaction code #1194

merged 9 commits into from
Nov 22, 2018

Conversation

rhaschke
Copy link
Contributor

Tracking down shared_ptr leaks, I stumbled over several minor issues in robot_interaction code for rviz plugins. For review, you best look at individual commits instead of the overall bulk.
Please keep commit history.

@rhaschke rhaschke force-pushed the cleanup-robot-interaction branch 3 times, most recently from 85f41e4 to 0522f39 Compare November 20, 2018 07:25
davetcoleman
davetcoleman approved these changes Nov 21, 2018
View reviewed changes
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.

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 :-/

moveit_ros/robot_interaction/include/moveit/robot_interaction/interaction_handler.h
@@ -323,18 +309,6 @@ class InteractionHandler : public LockedRobotState

// remove '_' characters from name
static std::string fixName(std::string name);

public:
// DEPRECATED FUNCTIONS.
Copy link
Member

Choose a reason for hiding this comment

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

thanks for removing!

moveit_ros/robot_interaction/src/interactive_marker_helpers.cpp Outdated
@@ -139,30 +139,37 @@ void addErrorMarker(visualization_msgs::InteractiveMarker& im)
im.controls.push_back(err_control);
}

// value for normalized quaternion
inline constexpr double qv()
Copy link
Member

Choose a reason for hiding this comment

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

better function name. also why isn't this a constant variable instead of a function?

de-vri-es
de-vri-es reviewed Nov 21, 2018
View reviewed changes
moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_display.cpp Outdated
addBackgroundJob(boost::bind(&MotionPlanningDisplay::publishInteractiveMarkers, this, false),
"publishInteractiveMarkers");
addBackgroundJob(boost::bind(&MotionPlanningDisplay::publishInteractiveMarkers, this, false), "publishInteractiveMark"
"ers");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really according to the code style? It seems odd. =]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. This has improved since #1214.

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_frame.cpp
@@ -197,6 +197,7 @@ MotionPlanningFrame::MotionPlanningFrame(MotionPlanningDisplay* pdisplay, rviz::

MotionPlanningFrame::~MotionPlanningFrame()
{
delete ui_;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least it is not intended to be copied ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, then I submit to your judgment wether we should keep it like this or put it in a std::unique_ptr :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, just noticed the deleted copy constructor :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it due to your comment.

moveit_ros/visualization/motion_planning_rviz_plugin/src/motion_planning_param_widget.cpp
@@ -51,6 +51,8 @@ MotionPlanningParamWidget::MotionPlanningParamWidget(QWidget* parent) : rviz::Pr

MotionPlanningParamWidget::~MotionPlanningParamWidget()
{
if (property_tree_model_)
delete property_tree_model_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, if the MotionPlanningParamWidget is copyable or movable, this could lead to double frees.

@de-vri-es
Copy link
Contributor

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.

@rhaschke
use normalized quaternions
03e63de 
... to get rid of rviz warning:
ros.rviz.quaternions: Interactive marker 'EE:goal_ra_tool_mount' contains unnormalized quaternions.
@rhaschke
fix memory leaks
e188318
@rhaschke
postpone subscription to trajectory topic
98232f6 
... to avoid error message: "No robot state or robot model loaded"
@rhaschke
InteractionHandler: remove deprecated handling of own KinematicsOptio…
cea048c 
…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).
@rhaschke
forbid copy-construction of MotionPlanningFrame + MotionPlanningParam…
cba8278 
…Widget
@rhaschke
clang-format
dfce54f
davetcoleman
davetcoleman approved these changes Nov 22, 2018
View reviewed changes
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.

I built and tested panda in Rviz with this locally

@rhaschke rhaschke merged commit dfce54f into moveit:melodic-devel Nov 22, 2018
rhaschke added a commit that referenced this pull request Nov 22, 2018
@rhaschke
Merge PRs #1166 (pass RobotModel to IK) and #1194 (cleanup robot inte…
87482b2 
…raction)
@rhaschke rhaschke deleted the cleanup-robot-interaction branch November 22, 2018 09:05
de-vri-es
de-vri-es reviewed Nov 22, 2018
View reviewed changes
...tion_planning_rviz_plugin/include/moveit/motion_planning_rviz_plugin/motion_planning_frame.h
@@ -100,6 +100,7 @@ class MotionPlanningFrame : public QWidget
Q_OBJECT

public:
MotionPlanningFrame(const MotionPlanningFrame&) = delete;
Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@de-vri-es de-vri-es de-vri-es left review comments

@davetcoleman davetcoleman davetcoleman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rhaschke @de-vri-es @davetcoleman