Naive clock jump implementation - allows for clock reuse and simplified Player setup #754
Conversation
emersonknapp
requested review from
jaisontj and
zmichaels11
and removed request for
a team
|
@MichaelOrlov FYI |
emersonknapp
force-pushed
the
emersonknapp/simple-clock-jump
branch
from
53ac074
to
5224a91
Compare
emersonknapp
force-pushed
the
emersonknapp/simple-clock-jump
branch
from
5224a91
to
5183692
Compare
|
Gist: https://gist.githubusercontent.com/emersonknapp/c71f7d18aab7ff09541be9b2d0f22c1b/raw/8401041718d2e42dd2704c81592b62925181b0e2/ros2.repos
|
There was a problem hiding this comment.
I am generally okay with it, just a few comments inline.
Before really approving it, I'd like to @emersonknapp's opinion of tests for this. Are we having tests covering all the new things in this PR?
rosbag2_cpp/include/rosbag2_cpp/clocks/time_controller_clock.hpp
Outdated
Show resolved
Hide resolved
…ed setup Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
… Add tests for jump Signed-off-by: Emerson Knapp <eknapp@amazon.com>
emersonknapp
force-pushed
the
emersonknapp/simple-clock-jump
branch
from
5183692
to
89fe16a
Compare
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
|
@Karsten1987 @MichaelOrlov let me know how you feel about the latest revision - it's maybe a little overloaded (doing a couple different things), but since this is a "reconfigure some things for ease of use" PR I am thinking that's probably OK |
There was a problem hiding this comment.
@emersonknapp New version looks good to me among a few minor comments in tests and doxygen comments.
rosbag2_cpp/include/rosbag2_cpp/clocks/time_controller_clock.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This looks good to me. A few nitpicks inline and I also agree with some comments of @MichaelOrlov to extend a few test cases.
I do request changes though because I don't understand why you deleted a constructor in the Player. Did I miss something here?
rosbag2_cpp/include/rosbag2_cpp/clocks/time_controller_clock.hpp
Outdated
Show resolved
Hide resolved
…n comments, jump backward test Signed-off-by: Emerson Knapp <eknapp@amazon.com>
|
Clock can be created in the constructor so it is never null, and have all its state simply maintained while looping or repeated calls to
play.This is an intentionally simple implementation that accomplishes this single goal, and is definitely not safe for use during playback. There is significantly more complexity involved in handling jumping during playback - but that is out of the scope of this change.