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

Refactoring of the yarpdataplayer module #2442

Merged
merged 10 commits into from
Jan 12, 2021
Merged

Conversation

vtikha
Copy link
Member

@vtikha vtikha commented Dec 22, 2020

This PR involves a complete refactoring of the yarpdataplayer module. With @vvasco, we implemented the yarpdataplayer library in order to separate the GUI from the processing and thus create a console version of the yarpdataplayer.

This has been done following the structure of the other GUI / console modules eg: yarplogger and yarpmanager

This PR addresses the following opened issues in YARP concerning the yarpdataplayer:

  • #2400: yarpdataplayer --hidden cannot start on headless system
  • #2401: yarpdataplayer closes immediately after loading data with --hidden flag is used
  • #2151: step command in rpc exhibits odd behaviors
  • #2140: Provide status flag to check if player has finished playing a loaded dataset
  • #1144: yarpdataplayer enhancements

The new version of the yarpdataplayer has been tested on:

  • macOS: Catalina (10.15+) and Big Sur (11.1 +) (not with Xcode)
  • linux: 18.04 and 20.04
  • windows: Windows 10

Here are some screenshots:

GUI version

Screenshot 2020-12-18 at 09 22 29

Console version

Screenshot 2020-12-18 at 14 28 22

Aside all the commands available on the yarpdataplayer, the user can still make use of the following commands in rpc:

dataplayer-help

The yarpdataplayer-console on the other hand allows the user to:

console-help

As a further note, not included in the changes that we did for this PR, we wanted to add some progress bars in the console version of the dataplayer to make things clearer, and found this very powerful library.
Before going ahead with this, and including the files as is, @vvasco and me wanted to discuss about a "nicer" way to include this lib into YARP so that other terminal modules such as the yarprobotinterface and other, can make use of it. The licence is MIT so it should be ok, but let's discuss this further if this is deemed interesting for YARP.

@vtikha vtikha requested a review from drdanz as a code owner December 22, 2020 11:34
@update-docs
Copy link

update-docs bot commented Dec 22, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update the release notes by adding a file in doc/release/<target_branch>, based on your changes.

@CLAassistant
Copy link

CLAassistant commented Dec 22, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@drdanz drdanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

(Reminder to myself: the scripts/admin/update-license and COPYING file should be updated, since the yarpdataplayer-console tool is LGPL)

About the indicators library, I'm in favour of including it in YARP, the MIT license is compatible with the BSD-3-Clause, therefore I think it is safe to include the single header version in extern with a corresponding README.txt file (see extern/catch for an example).
Probably you should import also the LICENSE and LICENSE.termcolor files.
You might also have a look at the COPYING file, and see if the MIT license should be mentioned there, but I think it is not required.

@drdanz

This comment has been minimized.

@drdanz drdanz added the PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP label Jan 8, 2021
@drdanz
Copy link
Member

drdanz commented Jan 11, 2021

I'm not sure what's wrong with the CI, the failing builds do not produce any output... let's try rebasing this again

@drdanz

This comment has been minimized.

@drdanz drdanz merged commit a367556 into robotology:master Jan 12, 2021
@drdanz
Copy link
Member

drdanz commented Jan 12, 2021

It was some problem on github action, after the rebase everything was built successfully.
Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: GUI - yarpdataplayer PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Resolution: Merged Type: Refactor Involves refactoring some part of code
Projects
None yet
4 participants