-
Notifications
You must be signed in to change notification settings - Fork 493
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
Port trajectory_rviz_plugin #201
Port trajectory_rviz_plugin #201
Conversation
f3c25a6
to
52a5691
Compare
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 opened a PR to enable the trajectory visualization in the demo RoboticsYY#1
@@ -72,7 +74,7 @@ class TrajectoryDisplay : public rviz::Display | |||
void onInitialize() override; | |||
void onEnable() override; | |||
void onDisable() override; | |||
void setName(const QString& name) override; | |||
// void setName(const QString& name) override; |
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's set as final in display.hpp, so this should be removed
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 didn't figure out why Rviz2 made this method final
. But I think removing this method will not affect the functionality of this plugin. Done remove.
@JafarAbdi Thanks for helping change the MoveItCpp demo! |
72b2a8e
to
9837670
Compare
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 great and works as intended. I've already based this on the sync without issues and will merge this after comments are addressed
@@ -145,6 +147,9 @@ void TrajectoryVisualization::onInitialize(const rclcpp::Node::SharedPtr& node, | |||
context_ = context; | |||
node_ = node; | |||
|
|||
auto ros_node_abstraction = context_->getRosNodeAbstraction().lock(); |
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.
Do we need to check if the weak_ptr lock was successful?
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, it's better to make sure the lock() return non-empty pointer. A check has been added.
@@ -58,21 +58,23 @@ void TrajectoryDisplay::onInitialize() | |||
{ | |||
Display::onInitialize(); | |||
|
|||
trajectory_visual_->onInitialize(scene_node_, context_, update_nh_); | |||
node_ = context_->getRosNodeAbstraction().lock()->get_raw_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
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.
Added check here as well.
fd0bfbe
to
4d9dce8
Compare
4d9dce8
to
abf4162
Compare
Description
This PR intends to port the
moveit_ros_visualization/trajectory_rviz_plugin
to ROS2. The dependencies were ported previously in PR #160.Checklist