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

Added TwistStamped and AccelStamped default plugins #991

Merged
merged 12 commits into from
Jul 18, 2023
Merged

Added TwistStamped and AccelStamped default plugins #991

merged 12 commits into from
Jul 18, 2023

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jun 8, 2023

Added TwistStamped and AccelStamped default plugins

@ahcorde
Added TwistStamped and AccelStamped default plugins
137222d 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Jun 8, 2023
clalancette
clalancette requested changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a 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
rviz_rendering/include/rviz_rendering/objects/screw_visual.hpp Outdated Show resolved Hide resolved
rviz_rendering/src/rviz_rendering/objects/screw_visual.cpp Outdated Show resolved Hide resolved
rviz_rendering/src/rviz_rendering/objects/screw_visual.cpp Outdated
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++) {
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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
@ahcorde
Adde feedback
813d691 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
adityapande-1995
adityapande-1995 approved these changes Jun 16, 2023
View reviewed changes
Copy link

@adityapande-1995 adityapande-1995 left a comment •
edited
Loading

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 !

@ahcorde ahcorde requested a review from clalancette June 19, 2023 07:18
@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 19, 2023

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ahcorde
Fix windows build and warnings
421852f 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jun 19, 2023 •
edited
Loading

  • Windows Build Status

@ahcorde
Try to fix windows buil
130ed36 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Fixed warnings
8b82383 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Fixed build
fce8be9 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Merge remote-tracking branch 'origin/ros2' into ahcorde/rolling/twist…
a50f91f 
…_plugin
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 10, 2023

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 10, 2023

friendly ping @clalancette

@ahcorde
Merge remote-tracking branch 'origin/ros2' into ahcorde/rolling/twist…
0a07528 
…_plugin
@ahcorde
Fix CMakelists.txt
3af9a79 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 11, 2023 •
edited
Loading

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 11, 2023

@clalancette friendly ping, CI is green

clalancette
clalancette requested changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a 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.

rviz_default_plugins/CMakeLists.txt Outdated Show resolved Hide resolved
rviz_default_plugins/CMakeLists.txt Outdated Show resolved Hide resolved
rviz_default_plugins/include/rviz_default_plugins/displays/accel/accel_display.hpp Outdated
Comment on lines 49 to 52
void processMessage(geometry_msgs::msg::AccelStamped::ConstSharedPtr msg) override
{
processMessagePrivate(msg->header, msg->accel.linear, msg->accel.angular);
}
Copy link
Contributor

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
rviz_rendering/src/rviz_rendering/objects/screw_visual.cpp Outdated
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++) {
Copy link
Contributor

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.

@ahcorde
Merge remote-tracking branch 'origin/rolling' into ahcorde/rolling/tw…
ba0e6d3 
…ist_plugin
@ahcorde
Added feedback
67bfdf8 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from clalancette July 18, 2023 08:50
clalancette
clalancette requested changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a 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_rendering/src/rviz_rendering/objects/screw_visual.cpp 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
@ahcorde
Added feedback
0b0f45e 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 18, 2023 •
edited
Loading

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ahcorde ahcorde requested a review from clalancette July 18, 2023 13:01
@ahcorde ahcorde merged commit 9599dd4 into rolling Jul 18, 2023
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the ahcorde/rolling/twist_plugin branch July 18, 2023 15:10
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 18, 2023

https://github.com/Mergifyio backport humble iron

@mergify
Copy link

mergify bot commented Jul 18, 2023 •
edited
Loading

backport humble iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 18, 2023
@ahcorde @mergify
Added TwistStamped and AccelStamped default plugins (#991)
8e1eee9 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
(cherry picked from commit 9599dd4)
@mergify mergify bot mentioned this pull request Jul 18, 2023
Merged
mergify bot pushed a commit that referenced this pull request Jul 18, 2023
@ahcorde @mergify
Added TwistStamped and AccelStamped default plugins (#991)
6658435 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
(cherry picked from commit 9599dd4)
@mergify mergify bot mentioned this pull request Jul 18, 2023
Merged
ahcorde added a commit that referenced this pull request Jul 18, 2023
@mergify @ahcorde
Added TwistStamped and AccelStamped default plugins (#991) (#1015)
8dce53f 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
(cherry picked from commit 9599dd4)

Co-authored-by: Alejandro Hernández Cordero <alejandro@openrobotics.org>
ahcorde added a commit that referenced this pull request Jul 19, 2023
@mergify @ahcorde
Added TwistStamped and AccelStamped default plugins (#991) (#1014)
df8c29f 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
(cherry picked from commit 9599dd4)

Co-authored-by: Alejandro Hernández Cordero <alejandro@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@adityapande-1995 adityapande-1995 adityapande-1995 approved these changes

@sloretz sloretz Awaiting requested review from sloretz

Assignees

@ahcorde ahcorde

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ahcorde @clalancette @adityapande-1995