`ros2 bag play --topics/--services` cannot work without extra option before bag file path

[fujitatomoya]

Description

--topics and --services option arguments cannot work without having the extra option before the bag file path.

Expected Behavior

it should work even without extra option before bag file path.

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 bag play --topics chatter rosbag2_2024_04_25-10_55_16/
[INFO] [1714067979.777474156] [rosbag2_player]: Set rate to 1
[INFO] [1714067979.783672709] [rosbag2_player]: Adding keyboard callbacks.
[INFO] [1714067979.783702132] [rosbag2_player]: Press SPACE for Pause/Resume
[INFO] [1714067979.783707171] [rosbag2_player]: Press CURSOR_RIGHT for Play Next Message
[INFO] [1714067979.783712411] [rosbag2_player]: Press CURSOR_UP for Increase Rate 10%
[INFO] [1714067979.783716795] [rosbag2_player]: Press CURSOR_DOWN for Decrease Rate 10%
[INFO] [1714067979.783814715] [rosbag2_player]: Playback until timestamp: -1

Actual Behavior

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 bag play --topics chatter rosbag2_2024_04_25-10_55_16/
usage: ros2 bag play [-h] [-s {sqlite3,my_test_plugin,mcap,my_read_only_test_plugin}]
                     [--read-ahead-queue-size READ_AHEAD_QUEUE_SIZE] [-r RATE] [--topics topic [topic ...]]
                     [--services service [service ...]] [-e REGEX] [-x EXCLUDE_REGEX] [--exclude-topics topic [topic ...]]
                     [--exclude-services service [service ...]] [--qos-profile-overrides-path QOS_PROFILE_OVERRIDES_PATH]
                     [-l] [--remap REMAP [REMAP ...]] [--storage-config-file STORAGE_CONFIG_FILE] [--clock [CLOCK] |
                     --clock-topics CLOCK_TOPICS [CLOCK_TOPICS ...] | --clock-topics-all] [-d DELAY]
                     [--playback-duration PLAYBACK_DURATION]
                     [--playback-until-sec PLAYBACK_UNTIL_SEC | --playback-until-nsec PLAYBACK_UNTIL_NSEC]
                     [--disable-keyboard-controls] [-p] [--start-offset START_OFFSET] [--wait-for-all-acked TIMEOUT]
                     [--disable-loan-message] [--publish-service-requests]
                     [--service-requests-source {service_introspection,client_introspection}]
                     bag_path
ros2 bag play: error: the following arguments are required: bag_path

To Reproduce

1. start some publishers.

ros2 topic pub -r 1 /greetings  example_interfaces/msg/String "{data: 'Hello fron greeting topic'}"
ros2 topic pub -r 1 /chatter std_msgs/msg/String "{data: 'Hello on greeting topic'}"

2. record all topics.

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 bag record -a
[INFO] [1714067717.014419278] [rosbag2_recorder]: Press SPACE for pausing/resuming
[INFO] [1714067717.018212708] [rosbag2_recorder]: Listening for topics...
[INFO] [1714067717.018257318] [rosbag2_recorder]: Event publisher thread: Starting
[INFO] [1714067717.026120002] [rosbag2_recorder]: Subscribed to topic '/rosout'
[INFO] [1714067717.029323308] [rosbag2_recorder]: Subscribed to topic '/parameter_events'
[INFO] [1714067717.031138205] [rosbag2_recorder]: Subscribed to topic '/events/write_split'
[INFO] [1714067717.031189731] [rosbag2_recorder]: Recording...
[INFO] [1714067721.022214861] [rosbag2_recorder]: Subscribed to topic '/greetings'
[INFO] [1714067724.435570665] [rosbag2_recorder]: Subscribed to topic '/chatter'
[INFO] [1714067736.480997299] [rosbag2_recorder]: Pausing recording.
[INFO] [1714067736.481258938] [rosbag2_cpp]: Writing remaining messages from cache to the bag. It may take a while
[INFO] [1714067736.488452363] [rosbag2_recorder]: Event publisher thread: Exiting
[INFO] [1714067736.488516934] [rosbag2_recorder]: Recording stopped
[INFO] [1714067736.577997951] [rclcpp]: signal_handler(signum=2)

3. try to replay the topic from the bag file with --topics optional argument.

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 bag play --topics chatter greetings rosbag2_2024_04_25-10_55_16/
usage: ros2 bag play [-h] [-s {sqlite3,my_test_plugin,mcap,my_read_only_test_plugin}]
                     [--read-ahead-queue-size READ_AHEAD_QUEUE_SIZE] [-r RATE] [--topics topic [topic ...]]
                     [--services service [service ...]] [-e REGEX] [-x EXCLUDE_REGEX] [--exclude-topics topic [topic ...]]
                     [--exclude-services service [service ...]] [--qos-profile-overrides-path QOS_PROFILE_OVERRIDES_PATH]
                     [-l] [--remap REMAP [REMAP ...]] [--storage-config-file STORAGE_CONFIG_FILE] [--clock [CLOCK] |
                     --clock-topics CLOCK_TOPICS [CLOCK_TOPICS ...] | --clock-topics-all] [-d DELAY]
                     [--playback-duration PLAYBACK_DURATION]
                     [--playback-until-sec PLAYBACK_UNTIL_SEC | --playback-until-nsec PLAYBACK_UNTIL_NSEC]
                     [--disable-keyboard-controls] [-p] [--start-offset START_OFFSET] [--wait-for-all-acked TIMEOUT]
                     [--disable-loan-message] [--publish-service-requests]
                     [--service-requests-source {service_introspection,client_introspection}]
                     bag_path
ros2 bag play: error: the following arguments are required: bag_path

System (please complete the following information)

• OS: Ubuntu 24.04 Nobel
• ROS 2 Distro: Rolling
• Install Method: source code
• Version: ros2/ros2@4d07f58

Additional context

as a work-around, if we have an extra option before the bag file path, it works as expected.

the following example command includes -s mcap before bag file path.

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 bag play --topics chatter greetings -s mcap rosbag2_2024_04_25-10_55_16/
[INFO] [1714067979.777474156] [rosbag2_player]: Set rate to 1
[INFO] [1714067979.783672709] [rosbag2_player]: Adding keyboard callbacks.
[INFO] [1714067979.783702132] [rosbag2_player]: Press SPACE for Pause/Resume
[INFO] [1714067979.783707171] [rosbag2_player]: Press CURSOR_RIGHT for Play Next Message
[INFO] [1714067979.783712411] [rosbag2_player]: Press CURSOR_UP for Increase Rate 10%
[INFO] [1714067979.783716795] [rosbag2_player]: Press CURSOR_DOWN for Decrease Rate 10%
[INFO] [1714067979.783814715] [rosbag2_player]: Playback until timestamp: -1

[fujitatomoya]

@Barry-Xu-2018 can you come up with the patch for this? i think this should work.

i would recommend that 2 different PRs.

• the one for --topics option, so that we can backport this fix to all released distro including humble and iron.
• the other for --services option, this only needs to be backported into jazzy.

CC: @MichaelOrlov

[fujitatomoya]

@MichaelOrlov you can assign this either @fujitatomoya or @Barry-Xu-2018 .

[fujitatomoya]

adding braces does not work either.

# ros2 bag play --topics [chatter greetings] -s mcap rosbag2_2024_04_25-10_55_16/
Traceback (most recent call last):
  File "/root/ros2_ws/colcon_ws/install/ros2cli/bin/ros2", line 33, in <module>
    sys.exit(load_entry_point('ros2cli', 'console_scripts', 'ros2')())
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/ros2_ws/colcon_ws/build/ros2cli/ros2cli/cli.py", line 91, in main
    rc = extension.main(parser=parser, args=args)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/ros2_ws/colcon_ws/build/ros2bag/ros2bag/command/bag.py", line 38, in main
    return extension.main(args=args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/ros2_ws/colcon_ws/build/ros2bag/ros2bag/verb/play.py", line 241, in main
    player.play(storage_options, play_options)
ValueError: Invalid topic name: topic name must not contain characters other than alphanumerics, '_', '~', '{', or '}':
  '[chatter'
   ^

[Barry-Xu-2018]

i would recommend that 2 different PRs.

• the one for --topics option, so that we can backport this fix to all released distro including humble and iron.
• the other for --services option, this only needs to be backported into jazzy.

That's a good suggestion.
I'll look into solutions that don't change the parameter interface.
Additionally, the record command will face the same issue.

[Barry-Xu-2018]

Currently, if we define a parameter that can take multiple arguments, we use nargs='+' to specify this in the command configuration. This setting allows the parameter to accept one or more arguments following it.
e.g.

        parser.add_argument(
            '--topics', type=str, default=[], metavar='topic', nargs='+',
            help='Space-delimited list of topics to play.')

So if the command arguments are written as "--topics chatter greetings rosbag2_2024_04_25-10_55_16/", it cannot be resolved. The command-line interface cannot differentiate the bag file name from the topic names due to the nargs='+' configuration, leading to confusion.

There are 3 ways.

1. use -- to signify the end of the options. Bash provided this feature. Window platform may not support this.

   ros2 bag play --topics chatter greetings -- rosbag2_2024_04_25-10_55_16/

2. Adjust the position of parameters. That is, we should update the description of usage

   ros2 bag play rosbag2_2024_04_25-10_55_16/ --topics chatter greetings

3. Add an option for bag_path (e.g. -i/--input). But this way cannot support backport.

   ros2 bag play --topics chatter greetings --input rosbag2_2024_04_25-10_55_16/

By now, I don't find other better ways.
Do you have any better ideas? @fujitatomoya @MichaelOrlov

[fujitatomoya]

can we take the topics as string with separator ,? and parse it as str then, call split(",") to take them into the list? in that case, space separator before bag file should not be included in the string?

we would want to keep the backward compatibility for this, we obviously do not want to break the user space because it is likely that users developed the some scripts to call ros bag2 xxx command.

[Barry-Xu-2018]

can we take the topics as string with separator ,? and parse it as str then, call split(",") to take them into the list? in that case, space separator before bag file should not be included in the string?
we would want to keep the backward compatibility for this, we obviously do not want to break the user space because it is likely that users developed the some scripts to call ros bag2 xxx command.

Yes. But I think this also affect backward compatibility.

This is the usage of rosbag play command for iron.

usage: ros2 bag play [-h] [-s {sqlite3,mcap}] [--read-ahead-queue-size READ_AHEAD_QUEUE_SIZE] [-r RATE] [--topics TOPICS [TOPICS ...]] [-e REGEX] [-x EXCLUDE] [--qos-profile-overrides-path QOS_PROFILE_OVERRIDES_PATH] [-l]
                     [--remap REMAP [REMAP ...]] [--storage-config-file STORAGE_CONFIG_FILE] [--clock [CLOCK] | --clock-topics CLOCK_TOPICS [CLOCK_TOPICS ...] | --clock-topics-all] [-d DELAY]
                     [--playback-duration PLAYBACK_DURATION] [--playback-until-sec PLAYBACK_UNTIL_SEC | --playback-until-nsec PLAYBACK_UNTIL_NSEC] [--disable-keyboard-controls] [-p] [--start-offset START_OFFSET]
                     [--wait-for-all-acked TIMEOUT] [--disable-loan-message]
                     bag_path

The --topics and --clock-topics has asked the arguments following option is separated by spaces. For backward compatibility, we cannot break it.

[MichaelOrlov]

@fujitatomoya @Barry-Xu-2018 I found a good solution for this issue.
We can guard the topics list in the quotes. i.e. ros2 bag play --topics "topic1 topic2" bagpath
I will prepare PR. I am working on it.

[MichaelOrlov]

@fujitatomoya @Barry-Xu-2018 Please see draft PR #1629
What do you think?

[Barry-Xu-2018]

@MichaelOrlov

We can guard the topics list in the quotes. i.e. ros2 bag play --topics "topic1 topic2" bagpath

Yes. This way is similar with #1628 (comment).
So this way also might have compatibility issues.

As I said (#1628 (comment)), Iron had the option [--topics TOPICS [TOPICS ...]. If users have written scripts in this style, those scripts have to be modified.

This issue also exists in previous versions. I support completely resolving this problem in the Jazzy release, despite the potential impacts. What do you think? @fujitatomoya

[MichaelOrlov]

@Barry-Xu-2018 @fujitatomoya As regards backward compatibility.
My proposal would be that we will not try to backport it to the Iron and Humble for the following reasons:

1. It turns out that in any scenario, we will have to introduce rosbag2 "API" CLI breaking changes. The less invasive option is option 1 with trailing "--". However, it doesn't work out of the box (I at least don't know how to implement it in Python) and would be not a very good solution for current Jazzy and future releases. Also, support on the Windows platform is questionable. I would prefer to fix it more appropriate and correct way on Rolling and Jazzy.
2. The issue is not so harmful because:
   a. The error is well defined, the recorder and player will fail to start rather than silently misinterpret parameters. i.e., no undefined behavior.
   b. There is a workaround for this issue by using the optional parameter "-s mcap" or "-s sqlite3" before specifying a path to the bag.
3. It seems no one has been affected by it so far. Since it was found by @Barry-Xu-2018 recently. It seems no one worries about it on previous ROS 2 distros.
4. Iron getting close to the EOL (End Of Life) Oct-Nov 2024 (in 6 months).

[fujitatomoya]

It seems no one has been affected by it so far. Since it was found by @Barry-Xu-2018 recently

this is so true. nobody else found this one yet. i think we can review #1629, and i will.