-
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
Python: raw_data + easier read_messages interface #50
Conversation
b392f0c
to
7bec18f
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.
Alright I installed the wheel of this PR and tested locally. It looks like it works well. Just left a couple of comments
lib/ros_message.h
Outdated
const std::string& topic, | ||
const RosValue::ros_time_t& timestamp, | ||
const std::string& md5, |
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'm a little worried that adding these args at the front could cause backwards compatibility issues. Is this a class that is only used internal to embag?
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.
Theoretically, it would be kind of weird to create the class externally since the msg_def
field is private and theres no actual way to get the data without it. Thus, only View
can actually interact with the class. I can remove this change though as its not necessary, thought it would just be good cleanup
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 just removed it!
.def_property_readonly("raw_data", [](std::shared_ptr<Embag::RosMessage> &m) { | ||
return py::bytes(&m->raw_buffer->at(m->raw_buffer_offset), m->raw_data_len); | ||
}) |
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 also want to add raw=True
functionality to the read_messages
interface in embag.Bag
?
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 could - but to do that (and maintain compatibility with rosbag) I would need to change the constructor of RosMessage and add a new field for the raw message definition.
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.
Going to leave this for potential future improvement!
7425316
to
b6f1859
Compare
Description of Changes
This adds the ability to get the raw data from a
ROSMessage
in python. In addition,Test Plan
Added a test to compare the output of the
raw_data
property with whatrosbag
generates.