-
Notifications
You must be signed in to change notification settings - Fork 0
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
Imports Kenobi changes from comp/2024. #270
base: main
Are you sure you want to change the base?
Conversation
..::---===*%@%%%%@@@@@@@@@@%@@%%@%%%%%%%##*+: | ||
.::---====+++**###%%%%%%%%%%%%###= | ||
.:--==+++++++=-. | ||
*/ |
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 is a beautiful file with no flaws
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.
sus
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.
return std::nullopt; | ||
} | ||
|
||
const double safe_speed = 0.5; |
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 assume this won't change but maybe we want this "safe speed" defined somewhere as a sort of constant (to be used in other places where we might need to do similar "escape" plans)?
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.
It's a good idea. I'm not sure where else this would get used at the moment off the top of my head, but worth keeping an eye out.
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.
Are there any specific cases we aren't using EasyMoveTos for robot movements? (I didn't notice any briefly looking through all our plays, just wanted to verify my understanding of how things are working.)
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.
There are a handful of places that modify / create motion commands without using EasyMoveTo
(ie. LineKick::RunKickBall()
). This mostly happens in skills. I think Basic122
is the only play that does this (in runStriker
to implement the backoff when opponent has possession).
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.
We don't have a way to currently visualize/list all of our plays at a glance, do we? (Nor the states that trigger the state machine)
If there's a straightforward way to do it, maybe it would be useful to generate a diagram from this file or the imports in all plays to get a in-the-moment idea what we currently have running.
My thought is we (I?) can probably create a quick script/app using either Plotly or Go.js to do this if it sounds like a good idea.
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.
Depends on what you mean. We have the play list in the UI that lists all of the plays and shows their current evaluation scores.
Visualizing the play decision process is more complicated because it's all done through arbitrary scoring functions in the plays themselves. There is no longer a top level state machine choosing the plays.
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.
Ah, gotcha. I thought that was still the case, so this doesn't seem as practical based on the current way we're evaluating.
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've noticed that in general for the objects updated per frame we're not using a consistent style for function naming, even for the same thing, i.e. update()
vs Update()
. Maybe we can set the linter up to catch some of these?
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 saw this in between this file and in_play_eval, for example
world.our_robots, [this, &world](const std::optional<Robot> & maybe_robot) { | ||
if (!maybe_robot) { | ||
world.our_robots, [this, &world](const Robot & robot) { | ||
if (!robot.visible) { |
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 like the use of the visible robots helpers instead of std::optional for these checks :)
This should be the last comp PR. I haven't read through all of it since comp. Realistically, it's more code than I personally feel like reviewing until something breaks. I'm sure we did a great job when writing it.