Add callbacks for PlayerClock::jump(time_point) API with CI fix #779
Conversation
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
|
Gist: https://gist.githubusercontent.com/MichaelOrlov/017e746f8540278a3398bff0770d7dc7/raw/3646eee77875a931bc2b4cd92642a0bc0570f4d2/ros2.repos
|
MichaelOrlov
requested review from
mjeronimo and
jhdcs
and removed request for
a team
| { | ||
| rcl_duration_t time_jump_delta; | ||
| { | ||
| std::lock_guard<std::mutex> lock(state_mutex); |
There was a problem hiding this comment.
Since ROS 2 supports C++ 17, we now have access to the std::scoped_lock class. Should we use that here?
There was a problem hiding this comment.
We haven't yet changed rosbag2 to target C++17 - this is an upgrade that could be done in a new PR if we're inclined
| void remove_jump_callbacks(PlayerClock::JumpHandler::SharedPtr handler) | ||
| { | ||
| std::lock_guard<std::mutex> lock(callback_list_mutex_); | ||
| for (auto it = callback_list_.begin(); it != callback_list_.end(); ++it) { |
There was a problem hiding this comment.
The function name and comment make it sound like this will remove multiple jump callbacks from the processing list. However, the code only removes one. Should the comment/function name change to make it more clear that we're removing one callback?
There was a problem hiding this comment.
A JumpHandler contains multiple callback functions. Maybe remove_jump_handler would be more clear - but the language is consistent
| handler->pre_callback(); | ||
| } else if (!before_jump && handler->post_callback) { | ||
| handler->post_callback(time_jump); | ||
| } |
There was a problem hiding this comment.
There is no else statement here. If this is intentional, can we add a comment stating that we intend to do nothing?
delete-merged-branch
bot
deleted the
michaelorlov/player-clock_jump_callbacks-with-win-fix
branch
Add callbacks for PlayerClock::jump(time_point) API
Part of #696
Reapply changes from #775 with CI fix for Windows build.