-
Notifications
You must be signed in to change notification settings - Fork 47
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
MultiArrayDimension.stride is bad #8
Comments
This looks like a reasonable suggestion. Unfortunately I think that the design meeting notes for this message were on the old wiki and consequently are going to be hard to come by. As a general note these messages were some of the first complex messages developed in ROS and were originally designed to be the underlying datatypes for Images and PointClouds. However they were determined that they could not capture all encodings for images. And for point clouds we mostly wanted to use external tools (ala pcl) for processing the point clouds instead of being restricted to the built in indexing. These super generic datatypes also lost their semantic meaning because they are so generic making introspection and debugging much harder. (If you recieve a Int8MultiArray on the wire do you pass it to an image viewer or a point cloud viewer?) As such they've basically been unused, but left for quick prototyping so as not to break anyone already using them. Also they required the complex indexing which clearly (#9) is vulnerable to getting wrong. This level of change of a core datatype is something that would break everything using it in it's current form. To not break any current user a new datatype could be added. But from our experience with the lack semantic meaning, I would highly recommend making the a very similar datatype but give it a non-generic name that means something, and consider adding any extra metadata fields which are appropriate for that semantic meaning. |
Is there scope for type parameters in ROS messages in future? For example, |
I'd argue their main purpose should be something like:
Because that gives an uniform implementation under the hood for different message types |
Templating is not possible in all languages. To start off with, if you're making a custom message you might as well collapse one layer. Otherwise you have to address data with
vs below would just be
from a definition like:
And although n-dimentional arrays are a very elegant in the pure CS sense. The number of use cases needing to support n-dimentional arrays of data are small, and those that can share code are even rarer. Thus supporting the common use cases with customized simpler datatypes provides a lot more value. For example an Image and a MultiEchoLaserScan look very similar data blocks (a 2 dimentional array of data), but semantically they cannot be treated the same at all. The type provides semantic meaning and there's no mistaking a multi echo laser scan for an image and trying to view it with the same tools, nor run it through the wrong type of filters. |
What's the intersection of that set with ros supported languages? Python would work as
But this could happen at the language runtime layer - just as
Actually, i think that's a bad example. A In those cases, having to convert one custom array format into another is a huge waste of programmer and processing time. If memory layout could be standardized, then, this burden is lifted from both - if you decide this sort of data conversion is a good idea, the code would just be |
One quick comment that the MultiEchoLaserScan is not a spherical depth map at all, the readings are on the same ray at different depths. We discussed this in the ROS team meeting and the disruptive nature of changing the indexing means that we cannot change it in the existing message. If there's an existing use case, we suggest creating a new message definition which covers the use case and provides a semantically meaningful name and documentation. @codebot is going to add some documentation also to explain how the messages in std_msgs are for examples and quick prototyping but are not recommended for regular reuse in due to their lack of semantic meaning. |
I added a bit of text to the std_msgs wiki page to summarize this. |
Also updated the one-line description of this package in the github repo. |
It's a bit weird how there's one message in this package ( |
Yeah, I agree completely, it's weird. Unfortunately, I don't have a good idea for how to separate the package into "useful for prototyping" and "useful for everything" since this package is ~8 years old now, and basically everyone using ROS is using this package either directly or indirectly. I'm almost too afraid to touch this package at all, to be honest. |
In
MultiArrayLayout.msg
, the definition given is:This is a fundamentally asymmetric definition:
dim[0].stride
has no meaningI would like to contest that this defeats one of the main purposed of striding, which is to allow memory order to be exchanged. Instead, the indexing rule should be:
And in our example, the dimensions would be
Note this allows us to serialize our array in a different order as
[red data] [green data] [blue data]
, and then label the axes differently:This makes the definition of
stride
match up with thenumpy
definition of stride.The text was updated successfully, but these errors were encountered: