-
Notifications
You must be signed in to change notification settings - Fork 211
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
Added TwistStamped and AccelStamped default plugins #991
Conversation
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
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've left a few minor things to correct, otherwise this looks good to me.
rviz_default_plugins/src/rviz_default_plugins/displays/screw/screw_display.cpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/screw/screw_display.cpp
Outdated
Show resolved
Hide resolved
Ogre::Vector3(angular_length / 4, 0, angular_length / 2)); | ||
circle_angular_->clear(); | ||
circle_angular_->setLineWidth(width_ * 0.05); | ||
for (int i = 4; i <= 32; i++) { |
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.
Can you maybe explain where these numbers come from, either in a comment or by making them constants somewhere?
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 code is migrated from rviz1, let me take a deep look
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 comment is still open.
rviz_rendering/test/rviz_rendering/objects/screw_visual_test.cpp
Outdated
Show resolved
Hide resolved
rviz_rendering/test/rviz_rendering/objects/screw_visual_test.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
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 good with green CI !
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
friendly ping @clalancette |
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@clalancette friendly ping, CI is green |
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've left a bunch of small things to fix, and one slightly larger question.
void processMessage(geometry_msgs::msg::AccelStamped::ConstSharedPtr msg) override | ||
{ | ||
processMessagePrivate(msg->header, msg->accel.linear, msg->accel.angular); | ||
} |
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 is a small nit, but since we have to have a .cpp
for this class anyway, I'd prefer to move this implementation there. It will just make compilation for downstream consumers a bit faster.
rviz_default_plugins/include/rviz_default_plugins/displays/twist/twist_display.hpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/test/reference_images/acceles_are_displayed_ref.png
Outdated
Show resolved
Hide resolved
rviz_default_plugins/test/reference_images/twistes_are_displayed_ref.png
Outdated
Show resolved
Hide resolved
rviz_default_plugins/test/rviz_default_plugins/publishers/accel_publisher.hpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/test/rviz_default_plugins/publishers/twist_publisher.hpp
Outdated
Show resolved
Hide resolved
Ogre::Vector3(angular_length / 4, 0, angular_length / 2)); | ||
circle_angular_->clear(); | ||
circle_angular_->setLineWidth(width_ * 0.05); | ||
for (int i = 4; i <= 32; i++) { |
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 comment is still open.
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
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.
OK, just a handful of very small nits to fix. Once those are done, a green CI run and we can merge this. Thanks for iterating.
rviz_default_plugins/include/rviz_default_plugins/displays/twist/twist_display.hpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/test/rviz_default_plugins/page_objects/twist_display_page_object.cpp
Show resolved
Hide resolved
rviz_default_plugins/test/rviz_default_plugins/page_objects/twist_display_page_object.hpp
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/screw/screw_display.cpp
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/screw/screw_display.cpp
Show resolved
Hide resolved
rviz_default_plugins/test/rviz_default_plugins/page_objects/accel_display_page_object.cpp
Show resolved
Hide resolved
rviz_default_plugins/test/rviz_default_plugins/page_objects/accel_display_page_object.hpp
Show resolved
Hide resolved
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
https://github.com/Mergifyio backport humble iron |
✅ Backports have been created
|
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> (cherry picked from commit 9599dd4)
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> (cherry picked from commit 9599dd4)
Added TwistStamped and AccelStamped default plugins