Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add rosbag support, add CompressedImage, add geometry_msgs Stamped types #3

Closed

Conversation

awesomebytes
Copy link
Contributor

Sorry about the massive amount of changes... but is all stuff I needed so...

First, as I reported in #2, if I tried to transform messages from directly reading from a rosbag, it fails. That is fixed (maybe not in the most elegant way, but following your style on the package).

Secondly, CompressedImage to numpy array. Not the other way around... as that would pull big dependences like OpenCV or PIL. I did my best, I don't know how to make it more generic, sorry!

Lastly, I wanted to transparently convert also XXXStamped (PointStamped, for example) to numpy array without adding extra code, so I added that too.

I added documentation on the README too.

@eric-wieser
Copy link
Owner

Sorry for missing this - can you split this into a separate PR for rosbags and everything else?

A quick summary of my views here:

  • Completely in favor of the XXXStamped changes - I think I was just too lazy to add them.
  • CompressedImage does't looks particularly useful as a numpy conversion, and using the builtin numpy_msg tools gives you all you need - but doesn't seem harmful either
  • The rosbag stuff feels like a hack, and really ought to be fixed upstream. There are two cases to cover:
    • The message type has not changed since the bag was written. This should just work, and does not due to what I'd consider a bug in genpy. Is it possible to compare the md5 hash of the message that ROS computes instead of manging the name?
    • The message type has changed, in which case not matching could be considered the "correct" thing to do - I think that's what happens if you try to subscribe to an old message type

@awesomebytes
Copy link
Contributor Author

Superseeded by #5 & #6 which also fix #2

Closing.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants