-
Notifications
You must be signed in to change notification settings - Fork 728
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
ROS 2 node that flips image and publishes updated transform #756
Conversation
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.
A few comments.
ca97c58
to
135a1da
Compare
@JWhitleyWork I think I have cleaned up most, but the header out of order has me flummoxed. I thought I have matched image rotate. I think I have abused Jenkins enough for tonight. Let me know if you want further cleanup, and if you'd prefer me to re-push as a single squashed commit for this PR. |
#include <math.h> | ||
|
||
#include <memory> | ||
#include <string> |
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.
Move these two below line 43 to resolve the linter error.
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.
Sorry, just a couple more minor things. After this it should pass CI and I'll be happy to merge it.
image_flip/src/image_flip_node.cpp
Outdated
config_.in_image_topic_name = | ||
this->declare_parameter("in_image_topic_name", std::string("image")); | ||
config_.out_image_topic_name = | ||
this->declare_parameter("out_image_topic_name", std::string("rotated_image")); |
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.
For topics which don't use a configurable namespace, please allow users to use remapping instead of passing in the topic name as a parameter. This causes confusion and may make users frustrated if they try to use remapping but the name has been changed with configuration parameters.
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.
IIRC, using a parameter was a hack around an issue with the rclcpp resolve_topic_name and how it dealt with images. It will be a bit before I can dig into that again. Do you happen to know if the resolve_topic_name issue was resolved?
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 think it's fixed now. During testing I found I could not use the image_view, and rviz2 was crashing with rolling, so I added a parameter to let you select QoS settings for input and output topics. Defaults to sensor_data (best effort) on input, and default (reliable) on output.
…a mounted upside down)
9a3f67d
to
ca872f4
Compare
rebased to latest rolling branch |
Assigning myself to review this and get it merged (I'm inclined though to not add yet another package - maybe make this another node in image_rotate? just because of the overhead of each package with separate build/documentation/etc) |
@mikeferguson It will probably be May before I would have to work on reorganizing this into "another node in image_rotate" |
Updating for latest changes to Rolling release (Humble compatible) and closing PR #743 from older branch
(e.g. for camera mounted upside down)
The node does a simple image rotation (by 90, 180, 270) as simple pixel transpose .
The node publishes a static tf to allow images to be viewed right side up in RViz if camera is mounted upside down.
This is less general than image_rotate, but performs simpler operations.
It also preserves the image size and aspect ratio.
It does NOT attempt to remap the distortion parameters at this time.
The code has been tested under Humble. Still required camera topics as parameters due to issue with extracting camera/camera_info names from base topic.
Posting pull request for review and comment. It does not currently have any specific unit tests.