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

Test play improvements #1899

Merged
merged 10 commits into from
Jun 12, 2022
Merged

Test play improvements #1899

merged 10 commits into from
Jun 12, 2022

Conversation

kasohrab
Copy link
Contributor

@kasohrab kasohrab commented May 30, 2022

Description

See issue below for description.

Associated Issue

Resolves #1896

Steps to test

Test Case 1

1. note the required format of the plays (see the plays set close to the top of gameplay node in its constructor).
2. go to testing tab in ui, add play in that format.
3. click the desired play in the top table, and hit run.
4. all of them will work (except lineup because it's broken rn)

I did not implement the next button. It would be easy to set up later though.
I also did not use the python local param server (I commented it out, as I think it's overengineering for now, but it could be important later)
Later we can easily just at launch populate all valid plays into the selected plays list.

Also this pr deletes some unused ui stuff.
But at this point because it is functional, this pr is closed to new features.

Expected result: can change test play at runtime

Someone should double check trying to run invalid formats, non-existent plays, etc.. The code should handle all those cases.

Also you can use ros2 param set /gameplay_node test_play formatted_name

@kasohrab kasohrab marked this pull request as ready for review June 6, 2022 21:00
@kasohrab kasohrab requested review from kfu02 and p-nayak11 June 6, 2022 21:34
Copy link
Contributor

@kfu02 kfu02 left a comment

Choose a reason for hiding this comment

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

Once #1898 is merged, could you merge in ros2? I can't test this on my pip=22.1 machine :(

Also, I would really strongly prefer there to be a list of checkbox options to select valid plays, since we have that list already. Typing in the same play on every restart of the UI will be a pain when developing new gameplay features.

Not sure what the best way to do this is--have both the ui code + GameplayNode read from some yaml? Publish a list of valid play strings as part of the msg on the test play topic from GN?

Copy link
Contributor

@kfu02 kfu02 left a comment

Choose a reason for hiding this comment

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

Following up on the last review, here are some nits.

soccer/src/soccer/ui/main_window.cpp Outdated Show resolved Hide resolved
soccer/src/soccer/ui/main_window.hpp Show resolved Hide resolved
kasohrab and others added 3 commits June 10, 2022 23:45
- the alternative was to send all plays via ros but that seems excessive.
@kasohrab kasohrab requested a review from kfu02 June 11, 2022 03:50
Copy link
Contributor

@p-nayak11 p-nayak11 left a comment

Choose a reason for hiding this comment

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

The test tab works as intended. The keepaway play also does not work. Will need to check on it.

@kasohrab kasohrab merged commit 9aa16b8 into ros2 Jun 12, 2022
@kasohrab kasohrab deleted the run-tests branch June 12, 2022 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More Configurable Test Play
3 participants