Implement rosbag2 action play by Barry-Xu-2018 · Pull Request #1955 · ros2/rosbag2

Barry-Xu-2018 · 2025-04-02T02:20:23Z

Address ros-infrastructure/rep#405

Design #1928

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 · 2025-04-02T02:21:23Z

@MichaelOrlov @fujitatomoya

Please review this PR.

Signed-off-by: Barry Xu <barry.xu@sony.com>

fujitatomoya

@Barry-Xu-2018 1st review, can you check my comments?

in the meantime, i will start CI to see if other warnings during review.

ros2bag/ros2bag/verb/burst.py

rosbag2_transport/include/rosbag2_transport/play_options.hpp

rosbag2_storage_mcap/src/mcap_storage.cpp

rosbag2_transport/include/rosbag2_transport/player.hpp

rosbag2_transport/include/rosbag2_transport/player_action_client.hpp

rosbag2_transport/src/rosbag2_transport/player.cpp

rosbag2_transport/test/rosbag2_transport/mock_player.hpp

ros2bag/ros2bag/verb/play.py

fujitatomoya · 2025-04-03T20:17:36Z

Pulls: #1955
Gist: https://gist.githubusercontent.com/fujitatomoya/b5abc904756413ea7d3f28e439e0f919/raw/f55ebf7922ad03a51da906c027c3cb5b0d28c84b/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_test_common rosbag2_transport
TEST args: --packages-above ros2bag rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_test_common rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15575

Linux
Linux-aarch64
Linux-rhel
Windows

MichaelOrlov · 2025-04-04T05:28:13Z

Hi @Barry-Xu-2018, @fujitatomoya,
I am currently on vacation and can't review this PR.
I will be back Sunday.

fujitatomoya · 2025-04-04T05:34:49Z

@MichaelOrlov thanks for replying to this on your vacation 🍹 i think we can iterate before your review, and we will have 10 days until API freeze 🧊 i believe we can get this in next week 🙏🙏🙏 anyway, let's discuss when you get back! thanks and enjoy ✌️

fujitatomoya · 2025-04-04T05:35:46Z

@Barry-Xu-2018 CI looks fine, please address my comments above, and i can start CI again after that.

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 · 2025-04-07T08:46:45Z

@MichaelOrlov @fujitatomoya

In the current implementation, the goal ID is derived from recorded data. I'm considering whether it should be replaced with a new created goal ID. I would like to hear your thoughts.

If the action server is continuously running and the user repeatedly uses ros2 bag play to replay action requests, there will be an issue because the goal ID sent each time is the same.

fujitatomoya · 2025-04-07T17:55:22Z

@Barry-Xu-2018

i think that,

as bag information, we need to store the original goal ID for sure. so that user can match the goal ID with debug messages or something like that.
but when it is doing playback, i think goal ID needs to be replaced with new global unique identification, otherwise, you cannot play the action date to the action server concurrently.

fujitatomoya · 2025-04-07T17:56:20Z

Pulls: #1955
Gist: https://gist.githubusercontent.com/fujitatomoya/7fe3dc7bcfa57ed1effaab8db0c1c8fa/raw/f55ebf7922ad03a51da906c027c3cb5b0d28c84b/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_test_common rosbag2_transport
TEST args: --packages-above ros2bag rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_test_common rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15621

Linux
Linux-aarch64
Linux-rhel
Windows

Barry-Xu-2018 · 2025-04-08T01:47:18Z

@fujitatomoya

i think that,

as bag information, we need to store the original goal ID for sure. so that user can match the goal ID with debug messages or something like that.

but when it is doing playback, i think goal ID needs to be replaced with new global unique identification, otherwise, you cannot play the action date to the action server concurrently.

Thanks. I also think so.
I will update code.

Signed-off-by: Barry Xu <barry.xu@sony.com>

fujitatomoya · 2025-04-08T16:11:56Z

Pulls: #1955
Gist: https://gist.githubusercontent.com/fujitatomoya/9ecdcc6e32fe9e0732c3227bea6d1b02/raw/f55ebf7922ad03a51da906c027c3cb5b0d28c84b/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_test_common rosbag2_transport
TEST args: --packages-above ros2bag rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_test_common rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15633

Linux
Linux-aarch64
Linux-rhel
Windows

MichaelOrlov

@Barry-Xu-2018 Here are my findings so far.
However, I haven't thoroughly reviewed play_action_clients yet.
Thanks for the PR - the code quality became way better this time.

rosbag2_storage_mcap/src/mcap_storage.cpp

rosbag2_transport/include/rosbag2_transport/player.hpp

rosbag2_transport/include/rosbag2_transport/player_action_client.hpp

rosbag2_transport/src/rosbag2_transport/player.cpp

rosbag2_transport/CMakeLists.txt

README.md

Barry-Xu-2018 · 2025-04-09T11:43:19Z

@MichaelOrlov

Thank you for your review. I will address your review comments.

Signed-off-by: Barry Xu <barry.xu@sony.com>

MichaelOrlov

@Barry-Xu-2018 Thank you for addressing the review comments looks good to me now. 👍

fujitatomoya · 2025-04-10T17:37:10Z

@MichaelOrlov @Barry-Xu-2018 thanks 👍 i will start the CI now.

fujitatomoya · 2025-04-10T17:38:24Z

Pulls: #1955
Gist: https://gist.githubusercontent.com/fujitatomoya/e18e606861b500e8ad5890a100e3d71b/raw/f55ebf7922ad03a51da906c027c3cb5b0d28c84b/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_test_common rosbag2_transport
TEST args: --packages-above ros2bag rosbag2_cpp rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_test_common rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15658

Linux
Linux-aarch64
Linux-rhel
Windows

fujitatomoya · 2025-04-10T21:31:13Z

@MichaelOrlov @Barry-Xu-2018 CI is green and this is merged, thank you so much for your effort. (i will be working on the documentation for next week.)

Barry-Xu-2018 · 2025-04-11T00:34:40Z

@fujitatomoya

I think Michael's review is not finished yet. There should be additional comments.

@MichaelOrlov Please continue to put your comments here. I will handle them as a new PR.

fujitatomoya · 2025-04-11T03:41:46Z

@Barry-Xu-2018 @MichaelOrlov oh sorry, i thought this is ready since approved. anyway, if we need to follow up more comments, please open an another PR for that. thanks for letting me know.

MichaelOrlov · 2025-04-11T20:21:34Z

@Barry-Xu-2018 @fujitatomoya Sorry for the confusion.
I got approval and think we should merge this PR sooner rather than later because

I've reviewed the essential implementation part and after addressing the review comments it looks good to me
I don't have the capacity to review it more this week, and we need to merge this PR before the feature freeze on Monday.
If I or we find something in the play_action_clients - we can fix it as a bugfix after the feature freeze.

fujitatomoya · 2025-04-11T20:34:34Z

@MichaelOrlov appreciate your consideration, and totally understood.

we will address the follow-ups for sure, we are not going anywhere 👍

Implement rosbag2 action play

26cef4b

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 mentioned this pull request Apr 2, 2025

Action introspection rosbag2 support ros-infrastructure/rep#405

Open

Update _storage.pyi

3534997

Signed-off-by: Barry Xu <barry.xu@sony.com>

fujitatomoya requested review from MichaelOrlov and fujitatomoya April 2, 2025 06:06

fujitatomoya assigned Barry-Xu-2018 Apr 2, 2025

fujitatomoya requested review from fujitatomoya and removed request for fujitatomoya April 2, 2025 20:08

fujitatomoya reviewed Apr 3, 2025

View reviewed changes

Barry-Xu-2018 added 4 commits April 4, 2025 19:04

Address review comments

ac7de0b

Signed-off-by: Barry Xu <barry.xu@sony.com>

Remove unused dependency library in rosbag2_cpp/CMakeLists.txt

9625b06

Signed-off-by: Barry Xu <barry.xu@sony.com>

Optimize the code to reduce lock time

d1d611c

Signed-off-by: Barry Xu <barry.xu@sony.com>

Fix bugs while sending cancel/result request

f14b3db

Signed-off-by: Barry Xu <barry.xu@sony.com>

Use new goal id while playing recorded action message as action client

c4b0865

Signed-off-by: Barry Xu <barry.xu@sony.com>

fujitatomoya approved these changes Apr 8, 2025

View reviewed changes

MichaelOrlov requested changes Apr 9, 2025

View reviewed changes

Address review comments

de96f8d

Signed-off-by: Barry Xu <barry.xu@sony.com>

MichaelOrlov approved these changes Apr 10, 2025

View reviewed changes

fujitatomoya merged commit a08500c into ros2:rolling Apr 10, 2025
12 checks passed

claraberendsen mentioned this pull request Apr 15, 2025

Buildfarmer log 2025/04/14 - 2025/04/21 osrf/buildfarm-tools#142

Closed

Conversation

Barry-Xu-2018 commented Apr 2, 2025

Uh oh!

Barry-Xu-2018 commented Apr 2, 2025

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fujitatomoya commented Apr 3, 2025

Uh oh!

MichaelOrlov commented Apr 4, 2025

Uh oh!

fujitatomoya commented Apr 4, 2025

Uh oh!

fujitatomoya commented Apr 4, 2025

Uh oh!

Barry-Xu-2018 commented Apr 7, 2025

Uh oh!

fujitatomoya commented Apr 7, 2025

Uh oh!

fujitatomoya commented Apr 7, 2025

Uh oh!

Barry-Xu-2018 commented Apr 8, 2025

Uh oh!

fujitatomoya commented Apr 8, 2025

Uh oh!

MichaelOrlov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Barry-Xu-2018 commented Apr 9, 2025

Uh oh!

MichaelOrlov left a comment

Choose a reason for hiding this comment

Uh oh!

fujitatomoya commented Apr 10, 2025

Uh oh!

fujitatomoya commented Apr 10, 2025

Uh oh!

Uh oh!

fujitatomoya commented Apr 10, 2025

Uh oh!

Barry-Xu-2018 commented Apr 11, 2025

Uh oh!

fujitatomoya commented Apr 11, 2025

Uh oh!

MichaelOrlov commented Apr 11, 2025

Uh oh!

fujitatomoya commented Apr 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants