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.

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

Playback Scrubber #299

Merged
merged 39 commits into from
Sep 18, 2020
Merged

Playback Scrubber #299

merged 39 commits into from
Sep 18, 2020

Conversation

JShep1
Copy link

@JShep1 JShep1 commented Aug 19, 2020

This is a base implementation draft mainly to get feedback on the gui portion. Below is a small demo with only the scrubber bar itself. Any enhancements or usability improvement recommendations would be appreciated. @ColeOSRF @nkoenig @chapulina
playbackscrubber

So far I am going to add:

  • Start and end times on either side of the slide bar
  • The current time displayed above the slider marker
  • Manually enter time to jump to

John Shepherd added 8 commits August 15, 2020 15:24
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
@JShep1 JShep1 requested a review from chapulina as a code owner August 19, 2020 23:35
@github-actions github-actions bot added the 📜 blueprint Ignition Blueprint label Aug 19, 2020
@JShep1 JShep1 marked this pull request as draft August 19, 2020 23:35
@ColeOSRF
Copy link

After you add the current time it might be good to be able to click on that and type in a specific time. Could be useful for getting the time right where you want it without having to fiddle with the slider to get small increments. Speaking of small increments maybe an arrow on each side of the slider that you can click on and it will move the time by a small increment. Not sure the best way to determine how big it should jump each time you press the arrow. Perhaps a percentage of the time? Maybe the user should be able to set the increments?

@JShep1
Copy link
Author

JShep1 commented Aug 20, 2020

After you add the current time it might be good to be able to click on that and type in a specific time. Could be useful for getting the time right where you want it without having to fiddle with the slider to get small increments. Speaking of small increments maybe an arrow on each side of the slider that you can click on and it will move the time by a small increment. Not sure the best way to determine how big it should jump each time you press the arrow. Perhaps a percentage of the time? Maybe the user should be able to set the increments?

Yeah entering the time would definitely be useful. I think we're going to rely on the World Control buttons on the lower left to control the stepping through the log playback. Perhaps this prompts a step back button to accompany the play and step forward buttons

John Shepherd added 8 commits August 27, 2020 23:07
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
@JShep1 JShep1 marked this pull request as ready for review September 1, 2020 18:58
@JShep1
Copy link
Author

JShep1 commented Sep 1, 2020

logplayback
Ready for another review, here's a preview

John Shepherd added 2 commits September 1, 2020 12:29
Signed-off-by: John Shepherd <[email protected]>
@JShep1
Copy link
Author

JShep1 commented Sep 1, 2020

Depends on gazebosim/gz-math#152

chapulina added a commit that referenced this pull request Sep 17, 2020
@adlarkin
Copy link
Contributor

I just wanted to comment that this seems to work well with #351. This is definitely a great addition, thanks @JShep1!

@chapulina
Copy link
Contributor

The INTEGRATION_log_system test failures look suspicious:

2020-09-17T19:34:09.1303931Z /github/workspace/test/integration/log_system.cc:745: Failure
2020-09-17T19:34:09.1304402Z Value of: common::exists(logFile)
2020-09-17T19:34:09.1304708Z   Actual: false
2020-09-17T19:34:09.1304964Z Expected: true
2020-09-17T19:34:09.1305924Z �[1;33m[Wrn] [Filesystem.cc:288] �[0m�[1;33mFailed to open file [�[0m�[1;33m/github/workspace/build/test/test_logs/test_logs_record/state.tlog�[0m�[1;33m]: �[0m�[1;33mNo such file or directory�[0m�[1;33m�[0m
2020-09-17T19:34:09.1306845Z /github/workspace/test/integration/log_system.cc:750: Failure
2020-09-17T19:34:09.1307359Z Value of: common::exists(logPlaybackFile)
2020-09-17T19:34:09.1307721Z   Actual: false
2020-09-17T19:34:09.1307963Z Expected: true
2020-09-17T19:34:09.1308316Z Failed to open the requested sqlite3 database
2020-09-17T19:34:09.1308809Z /github/workspace/test/integration/log_system.cc:773: Failure
2020-09-17T19:34:09.1309713Z Expected: (batch.end()) != (recordedIter), actual: 8-byte object <90-85 47-7B B7-55 00-00> vs 8-byte object <A0-2D 00-8C 5E-7F 00-00>

@chapulina chapulina mentioned this pull request Sep 18, 2020
@JShep1
Copy link
Author

JShep1 commented Sep 18, 2020

The INTEGRATION_log_system test failures look suspicious:

Agreed, is this a flaky test? Jenkins shows it as not being a new failure, unless I'm interpreting it wrong.

@chapulina
Copy link
Contributor

Agreed, is this a flaky test? Jenkins shows it as not being a new failure

If you look here, it seems to be flaky because it's failed before:

https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/4177/testReport/junit/(root)/INTEGRATION_log_system/test_ran/history/

But if you look into each of those builds, they've all been builds related to this PR.

If you look at the history of this test for the ign-gazebo2 branch, it looks like it's been passing consistently:

https://build.osrfoundation.org/job/ignition_gazebo-ci-ign-gazebo2-bionic-amd64/149/testReport/(root)/LogSystemTest/history/

I haven't tried reproducing the failure locally yet, but I think we should take a look.

@chapulina
Copy link
Contributor

I can reproduce the failures locally and the latest ign-gazebo2 build that I just triggered doesn't have them. I'm looking into it.

https://build.osrfoundation.org/job/ignition_gazebo-ci-ign-gazebo2-bionic-amd64/150/

@chapulina
Copy link
Contributor

chapulina commented Sep 18, 2020

The changes in 802e92b fixed the tests for me locally. I have no idea why that would be an issue on this PR but not on ign-gazebo2 though 🤷‍♀️

@chapulina
Copy link
Contributor

Phew, so it looks like log_system is fixed. And I just noticed that Gui_TEST is also a new failure that I can reproduce locally. On it.

Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor

Ok, Gui_TEST is fixed in a7fbd0c.

@JShep1
Copy link
Author

JShep1 commented Sep 18, 2020

Thanks for fixing those issues! I'm assuming we'll be good to merge once CI runs now?

@chapulina
Copy link
Contributor

Argh, it looks like log_system has been passing consistently on Mac:

https://build.osrfoundation.org/job/ignition_gazebo-ci-ign-gazebo2-homebrew-amd64/156/testReport/(root)/LogSystemTest/history/

And I see it passed on previous commits of this PR. Let's run it one more time just to sanity check. I retriggered it.

@chapulina
Copy link
Contributor

Mac passed now! Merging! 🎉

@chapulina chapulina merged commit 5f38e9d into ign-gazebo2 Sep 18, 2020
@chapulina chapulina deleted the jshep1/add_playback_scrubber branch September 18, 2020 22:51
doisyg pushed a commit to wyca-robotics/ign-gazebo that referenced this pull request Dec 13, 2020
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: Guillaume Doisy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants