add topic remapping option to rosbag2 play #388
Conversation
ketatam
force-pushed
the
amine/topic_remapping
branch
from
7b6cf27
to
adea292
Compare
| void play(const StorageOptions & storage_options, const PlayOptions & play_options); | ||
| void play( | ||
| const StorageOptions & storage_options, const PlayOptions & play_options, | ||
| const std::vector<std::string> & topic_remapping_options = {}); |
There was a problem hiding this comment.
Do you think it would be more appropriate to add topic_name_map to PlayOptions/RecordOptions rather than adding new arguments to the API?
| @@ -52,6 +52,10 @@ def add_arguments(self, parser, cli_name): # noqa: D102 | |||
| '-l', '--loop', action='store_true', | |||
| help='enables loop playback when playing a bagfile: it starts back at the beginning ' | |||
| 'on reaching the end and plays indefinitely.') | |||
| parser.add_argument( | |||
There was a problem hiding this comment.
should we call this --ros-args (and call out that it enables topic name remapping in the help string), since you can pass any ros-args here and it'll work?
There was a problem hiding this comment.
Yes, we were wondering that. My reasoning not to go that route was the following:
--ros-argswas introduced in particular to avoid name clashes with user defined flags on user ros2 nodes, in other words to separate flags defined by rclcpp or rclpy, etc. from flags defined on nodes deriving from those- Rosbag isn't the classical node and I don't see a scenario where you'd want to provide a custom rosbag node, so separating "namespaces" of flags is not a big deal here
- It could be interesting to expose more flags from rosbag's node, but I wonder whether it is a good idea to expose all of them. Therefore, I'd say: keep things hidden, so we'd only expose
--ros-args --remapfor now. I also wondered whether at that point it isn't confusing if the notation is the same for a general node but not all options you can generally pass to--ros-argsare available.
Does that make sense? What do you think about this?
There was a problem hiding this comment.
Yeah, I think it makes sense to provide --ros-args --remap under the hood https://github.com/ros2/rosbag2/pull/388/files#diff-fa7b2593ea899bd8b6c1fd104229ec63R203, so that this new argument really is just for topic name remapping.
There was a problem hiding this comment.
I have added added a new topic remapping options to the play options. Please let me know if there are still some other problems
ketatam
force-pushed
the
amine/topic_remapping
branch
from
adea292
to
0a7db8a
Compare
| void play( | ||
| const StorageOptions & storage_options, const PlayOptions & play_options); |
There was a problem hiding this comment.
Remove unrelated change
| void play( | |
| const StorageOptions & storage_options, const PlayOptions & play_options); | |
| void play(const StorageOptions & storage_options, const PlayOptions & play_options); |
Signed-off-by: ketatam <mohamed.amine.ketata@tngtech.com>
ketatam
force-pushed
the
amine/topic_remapping
branch
from
0a7db8a
to
23527d8
Compare
|
|
@emersonknapp the CI is unstable but I think it's not because of my code. So can you please check this and if I do have to change somthing just let me know. Thanks! |
|
I took a look through the builds and yes, I don't see anything here that's specifically related to this PR. The Action CI is the continuing #381 We'll need to keep an eye on these issues leading up to the release candidate package, but for now I think we're good to merge. |
With the option --remap or -m it's now possible to do topic remapping in rosbag2 play.
This is done by passing the remapping options to the play node.
Closes #326.