-
Notifications
You must be signed in to change notification settings - Fork 252
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
Progress-bar for ros2 bag play #1836
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Nicola Loi <[email protected]>
video-example.mp4(at 2.6-2.7 seconds of bag duration, the progress bar is slightly updating while in paused state because I was clicking the left arrow for the play next keyboard control) Since [INFO] [1729028986.845909253] [rosbag2_player]: Pausing play.
Bag Time 1718553782.525239 Duration 1.272325/6.974378 [P] while this longer version of the progress bar doesn't work properly without extra line clearing (in this case, it's easy to clear the line since the logs come from rosbag2_player., but for "external" logs, this becomes more difficult): [INFO] [1729028986.845909253] [rosbag2_player]: Pausing play.us:Running]
Bag Time 1718553782.525239 Duration 1.272325/6.974378 [status:Paused] @MichaelOrlov looking forward to your feedback, for now I have made a test MVP code to check the feasibility of the feature. |
Signed-off-by: Nicola Loi <[email protected]>
d503e5f
to
2cb732d
Compare
Signed-off-by: Nicola Loi <[email protected]>
b815f9e
to
4ba1a8c
Compare
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.
@nicolaloi Thank you for your contribution.
I have a few concerns about the design and implementation.
First, we need to limit the frequency with which we will print in std::cout
. The best way to do this is to create a local variable, aka size_t progress_bar_update_freq_
(in Hz).
e.g. 1 Hz = once per/sec, 2 Hz = twice per second, 3 = three times per second etc..
Then, check the delay between the last call in the print_status(..)
. If the delay is more than the threshold, print; otherwise, skip.
We also need to move the check for the if (disable_progress_bar_)
inside print_status(..)
to make the logic cleaner and in one place.
Calling double current_time_secs = RCUTILS_NS_TO_S(static_cast<double>(clock_->now()));
each time in the print_status(..)
is very inefficient and expensive since we are locking mutex inside and transorming current time to the ROS time. We actually know the current ROS time at which we are playing. This is a timestamp from the message. I would suggest passing it to the print function as a parameter.
i.e., add tiemstamp parameter to the print_status(..)
function and use message_ptr->recv_timestamp
as a source of timestamp.
I found the implementation of the print_status(..)
function a bit messy.
That is a quintessence of the modern C++17 with templates and old style plain C code.
Need to avoid using C code. Please use C++ alternatives. I am not sure if we really need to flush buffer each time. Flush is an expensive operation since this is a blocking system call. Need to avoid it if possible.
IMO templating print_status(..)
function is overengineering and make code less readable and less understandable. The cleaner solution would be if using enum class values for different modes. In this case all extra wrappers like check_and_print_status()
will not be needed. We will be able to call print_status(PrintStatus::Paused)
directly from where we needed it.
@MichaelOrlov thanks for the comments.
Should the progress bar be disabled by default, or enabled by default with a default low update frequency? |
253256a
to
44fa94a
Compare
Signed-off-by: Nicola Loi <[email protected]>
Signed-off-by: Nicola Loi <[email protected]>
44fa94a
to
43fa0f8
Compare
I have made changes as discussed. Linked modifications: I have modified the
|
@nicolaloi I have not had a chance yet to look deeply into the code after your latest changes. However, I have a few comments.
|
Ok, I'll set the default rate to 3 Hz. For 2nd and 3rd points, there is no longer the |
Signed-off-by: Nicola Loi <[email protected]>
eb9ea35
to
9cdf5ad
Compare
I have realized the newly added variables |
Signed-off-by: Nicola Loi <[email protected]>
Signed-off-by: Nicola Loi <[email protected]>
@MichaelOrlov So, to avoid synchronization issues without using mutexes, etc to not increase overhead, the variable Regarding how to print the current timestamp: when a message is published, the Extras:
|
Closes #1762: Adds a progress bar for
ros2 bag play
, showing the bag time and duration, similar to what is seen in ROS1.In ROS1, the progress bar was easy to handle because the
rosbag play
command didn't print additional output during playback. However, in ROS2, many logs can be printed during playback (e.g., keyboard control logs).The progress bar is printed and continuously updated on the last line of the playback output. I have tested this initial implementation with keyboard controls (play, pause, play next, set rate), Ctrl+C, throwing an exception, and the flags
--log-level debug
,--delay X
, and--start-offset X
.To try it, use the new
--progress-bar
flag when playing a bag.