-
Notifications
You must be signed in to change notification settings - Fork 7
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 point_cloud_transport_py #26
Conversation
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
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 non-blocking comments but otherwise looks good to me!
Very nice reorganization 👍
@@ -151,12 +151,24 @@ class PointCloudTransport : public PointCloudTransportLoader | |||
return ret; | |||
} | |||
|
|||
//! Advertise a PointCloud2 topic, simple version. | |||
POINT_CLOUD_TRANSPORT_PUBLIC |
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.
Is this necessary, considering the existing definition of advertise on line 167 below?
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.
Ah is this to make the python binding easier?
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 to simplify things
self.transport_publishers[transport].publish(compressed_buffer) | ||
else: | ||
self.get_logger().error('Error encoding message!') | ||
# for transport, transport_info in self.topics_to_publish.items(): |
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 you need to uncomment this for loop, right?
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.
publisher.py and subscriber.py should be removed. This functionality is available from pybind11
&point_cloud_transport::Publisher::getNumSubscribers, | ||
"Returns the number of subscribers this publisher is connected to.") | ||
.def("shutdown", &point_cloud_transport::Publisher::shutdown, | ||
"Unsubscribe the callback associated with this Publisher.") |
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 reword this please?
.def("advertise", | ||
pybind11::overload_cast<const std::string &, uint32_t>( | ||
&point_cloud_transport::PointCloudTransport::advertise)); | ||
// TODO(ahcorde) |
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.
Just noting that i am ok with merging without addressing this TODO. But could you please make an issue pointing to the TODO so we dont forget about it?
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Sorry for the big refactoring
The idea is to split the current package in two.
Examples included in this PR ros-perception/point_cloud_transport_tutorial#5