-
Notifications
You must be signed in to change notification settings - Fork 538
Add reflection based support for deserializing ROS2 MCAP #11367
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
base: main
Are you sure you want to change the base?
Conversation
Web viewer built successfully.
Note: This comment is updated whenever you push a commit. |
02b8ef4
to
c568ddd
Compare
Is this the right "spec" for ROS2? (do we care about ROS1) https://docs.ros.org/en/kilted/Concepts/Basic/About-Interfaces.html#interfaces |
a8ace95
to
4ef2fc6
Compare
This implements the spec for ROS2 .msg messages, which is only slightly different from the ROS1 message spec. Adding support for ROS1 in the future is trivial with this in place. This does not implement parsing of ROS2 .idl message definitions |
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've reviewed the interface of the reflection-based parser and re_mcap
and that part LGTM. This is so cool to have!
Please let me know if you want a more detailed review of the parser too.
| re_mcap | Convert MCAP into Rerun-compatible data. | | ||
| re_memory | Run-time memory tracking and profiling. | | ||
| re_perf_telemetry | In and out of process performance profiling utilities for Rerun & Redap | | ||
| re_ros_msg | Parsing and deserializing ROS messages | |
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.
Looks like we still need to update the Architecture figma (see component above table).
if let Some(field_value) = message_fields.get(&spec_field.name) { | ||
append_value(field_builder, field_value, schema)?; | ||
} else { | ||
//TODO(gijsd): Field is missing in the message, append null |
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 still need that comment?
|
||
// For nested messages, we need to find the matching specification from dependencies | ||
// Since we don't have type information here, we'll try to match by field names | ||
let matching_spec = find_matching_message_spec(schema, message_fields); |
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.
This is quadratic over the number of dependencies. append_value
is called a lot, does it make sense to cache / precompute all specs when creating the parser? append_value
could then take a &self
?
What
Adds support for dynamically deserializing ROS2 MCAP messages into arrow data.
This is done by parsing the ROS2 message definition and using the type definition to deserialize the CDR-encoded data into arrow types.
TODO: