-
Notifications
You must be signed in to change notification settings - Fork 110
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
Example play to FSM #3402
base: master
Are you sure you want to change the base?
Example play to FSM #3402
Conversation
Remove Qt dependency from ER Force Simulator (UBC-Thunderbots#3311)
Merge main project to own branch
…cessary Files still need to be refactored, which will be done in the next commit
… `halt_tactic.h`, renaming variables and strings where necessary Variables and strings renamed for consistency and to allow tests to successfully run
…t`, renaming where necessary
…e necessary
`example_play_fsm.cpp` and associated files have not been implemented; planned to be implemented next commit
Still giving errors, as `example_play_fsm.cpp` has not been implemented
…play-to-fsm # Conflicts: # src/software/ai/hl/stp/play/example/example_play_fsm.h
Debugging still necessary, as the `example_play_test.cpp` is not passing
test case works now yippee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! The FSM looks good to me, just two comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should also write a FSM test and a pytest for your changes.
Take a look at Grayson's PR on how he does the FSM test: #3398
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good mostly. William had some good comments and I left some comments about adding more tests.
One of your tests is failing (software/geom/algorithms:end_in_obstacle_sample_test). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, from what I can see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that if you have implemented the example_play_fsm_test.cpp file you can remove this file, as it is now redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay some lore...
This is a simulated test. Simulated tests execute your AI changes against a simulator to simulate the world. We have two forms of these tests: a C++ test fixture and a newer Python test fixture. We are in the process of phasing the C++ tests out and replacing them with Python. Could you make a new example_play_test.py
? Here's an example of what these look like: https://github.com/UBC-Thunderbots/Software/blob/master/src/software/ai/hl/stp/play/defense/defense_play_test.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comment
This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
…2060/UBC-Thunderbots-Software into exampleplay-to-fsm
…into exampleplay-to-fsm
src/software/ai/hl/stp/play/BUILD
Outdated
@@ -119,7 +105,11 @@ cc_library( | |||
cc_library( | |||
name = "all_plays", | |||
deps = [ | |||
<<<<<<< HEAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like you've got merge conflict markers in here. Needs merge conflict fixing
from proto.ssl_gc_common_pb2 import Team | ||
|
||
|
||
# TODO issue #2599 - Remove Duration parameter from test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this issue does not seem to exist. we may not need it
[RobotSpeedEventuallyBelowThreshold(1e-3)] | ||
], | ||
ag_always_validation_sequence_set=[[]], | ||
ag_eventually_validation_sequence_set=[ | ||
[RobotSpeedEventuallyBelowThreshold(1e-3)] | ||
], | ||
test_timeout_s=10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead, I think you should assert that the robots come within a 1 m radius of the ball
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like you have a bad merge that you need to fix
This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Please fill out the following before requesting review on this PR
Description
Converts the Example Play from a co-routine to an FSM.
The example play was moved to its own folder, with appropriate BUILD files modified. This was done to maintain consistency with other FSMs.
Testing Done
example_play_test.cpp
successfully passed, and Example Play successfully ran on Thunderscope.Resolved Issues
resolves #3298
Length Justification and Key Files to Review
example_play.cpp
and corresponding.h
fileexample_play_fsm.cpp
and corresponding.h
fileexample/BUILD
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.h
file) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom
. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO
(or similar) statements should either be completed or associated with a github issue