Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Pause/resume PlayerClock #704

Merged
merged 5 commits into from
Apr 12, 2021
Merged

Pause/resume PlayerClock #704

merged 5 commits into from
Apr 12, 2021

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Mar 31, 2021 •
edited
Loading

Part of #696

Add set_paused and get_paused functions to PlayerClock and expose to Player.

@emersonknapp emersonknapp mentioned this pull request Mar 31, 2021
Closed
50 tasks
@emersonknapp emersonknapp changed the base branch from master to emersonknapp/player-clock March 31, 2021 21:05
@emersonknapp emersonknapp force-pushed the emersonknapp/player-clock-pause-resume branch from df6e5d8 to f0bb3cf Compare March 31, 2021 21:09
@emersonknapp emersonknapp force-pushed the emersonknapp/player-clock-pause-resume branch from 227a5e7 to e921b13 Compare March 31, 2021 21:54
@emersonknapp emersonknapp mentioned this pull request Apr 1, 2021
Merged
@emersonknapp emersonknapp force-pushed the emersonknapp/player-clock branch 2 times, most recently from 5e1012c to 20f37f3 Compare April 2, 2021 07:59
@emersonknapp emersonknapp mentioned this pull request Apr 2, 2021
Merged
1 task
@delete-merged-branch delete-merged-branch bot deleted the branch master April 3, 2021 02:42
@MichaelOrlov MichaelOrlov mentioned this pull request Apr 6, 2021
Closed
@emersonknapp emersonknapp force-pushed the emersonknapp/player-clock-pause-resume branch from e921b13 to 77161c8 Compare April 6, 2021 23:43
@MichaelOrlov
Copy link
Contributor

@emersonknapp Could you please rebase your branch on top of master?
I have dependent work in #718 and it's going to be some restructure after rebase.
It would be better to rebase before writing unit tests and polishing jump implementation.

@emersonknapp emersonknapp changed the base branch from emersonknapp/player-clock to master April 6, 2021 23:51
@emersonknapp
Copy link
Collaborator Author

@emersonknapp Could you please rebase your branch on top of master?

I read your mind and did it 2 minutes before you asked :)

@emersonknapp emersonknapp force-pushed the emersonknapp/player-clock-pause-resume branch 2 times, most recently from adab12f to ba1e476 Compare April 8, 2021 00:28
@emersonknapp emersonknapp changed the title [WIP] Pause/resume playback using PlayerClock Pause/resume playback using PlayerClock Apr 8, 2021
@emersonknapp emersonknapp changed the title Pause/resume playback using PlayerClock [WIP] Pause/resume playback using PlayerClock Apr 8, 2021
@emersonknapp emersonknapp force-pushed the emersonknapp/player-clock-pause-resume branch 3 times, most recently from 20c2131 to df78227 Compare April 8, 2021 21:41
Add pause/resume to PlayerClock
cdcd9a3 
Expose it on Player class as well

Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/player-clock-pause-resume branch from df78227 to cdcd9a3 Compare April 9, 2021 19:07
@emersonknapp emersonknapp marked this pull request as ready for review April 9, 2021 19:34
@emersonknapp emersonknapp requested a review from a team as a code owner April 9, 2021 19:34
@emersonknapp emersonknapp requested review from Karsten1987 and piraka9011 and removed request for a team April 9, 2021 19:34
@emersonknapp emersonknapp changed the title [WIP] Pause/resume playback using PlayerClock Pause/resume playback using PlayerClock Apr 9, 2021
@emersonknapp emersonknapp force-pushed the emersonknapp/player-clock-pause-resume branch from 5325519 to 3a52671 Compare April 9, 2021 19:44
Add tests, fix functionality problems revealed in test
98079dc 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/player-clock-pause-resume branch from 3a52671 to 98079dc Compare April 9, 2021 19:49
@emersonknapp emersonknapp changed the title Pause/resume playback using PlayerClock Pause/resume PlayerClock Apr 9, 2021
@emersonknapp emersonknapp mentioned this pull request Apr 9, 2021
Merged
@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Apr 10, 2021 •
edited
Loading

@emersonknapp Looks good to me.
Update: I will need to be able specify time point for snaphsot aka snapshot(time_point); when implementing jump method.
P.S. I can add it in my branch if you don't want to change it in yours.

Add new snapshot signature
71d5d68 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Collaborator Author

I will need to be able specify time point for snaphsot aka snapshot(time_point); when implementing jump method.

I've added a new snapshot(ros_time) signature for this purpose.

Add docstring for sleep_time_while_paused param
ce77997 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Separate pause tests into smaller individual tests
943a070 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp
Copy link
Collaborator Author

Gist: https://gist.githubusercontent.com/emersonknapp/93a1b355caa64dbd15867e203c48dcba/raw/e448dfbf5b37d9fabc6532e4c69b5961c7741736/ros2.repos
BUILD args: --packages-up-to rosbag2 rosbag2_tests rosbag2_cpp rosbag2_transport rosbag2_py
TEST args: --packages-select rosbag2 rosbag2_tests rosbag2_cpp rosbag2_transport rosbag2_py
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8205

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp emersonknapp merged commit 0c2f80f into master Apr 12, 2021
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/player-clock-pause-resume branch April 12, 2021 18:39
@emersonknapp emersonknapp mentioned this pull request Nov 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zmichaels11 zmichaels11 zmichaels11 approved these changes

@david-prody david-prody david-prody left review comments

@prajakta-gokhale prajakta-gokhale prajakta-gokhale approved these changes

@jikawa-az jikawa-az jikawa-az approved these changes

@Karsten1987 Karsten1987 Awaiting requested review from Karsten1987

@piraka9011 piraka9011 Awaiting requested review from piraka9011

@anuragdmadan anuragdmadan Awaiting requested review from anuragdmadan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@emersonknapp @MichaelOrlov @Karsten1987 @zmichaels11 @prajakta-gokhale @jikawa-az @david-prody