-
Notifications
You must be signed in to change notification settings - Fork 186
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
Position Monitoring Framework #2194
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.
Looks pretty good! There's one tiny bug due to C++ annoyances, and I recommended a quick rename. Make those fixes and then re-request my review and you should be good.
@@ -75,6 +76,9 @@ class Position { | |||
void update_alive_robots(std::vector<u_int8_t> alive_robots); | |||
const std::string get_name(); | |||
|
|||
//returns the current state of the robot | |||
virtual std::string return_current_state() = 0; |
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.
virtual std::string return_current_state() = 0; | |
virtual std::string get_current_state() = 0; |
Just a quick suggestion. It's not common to name a function "return...". I would suggest either get_current_state
or even simply current_state
@@ -9,6 +10,10 @@ std::optional<RobotIntent> Defense::derived_get_task(RobotIntent intent) { | |||
return state_to_task(intent); | |||
} | |||
|
|||
std::string Defense::return_current_state() { | |||
return "Defense" + static_cast<int>(current_state_); |
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.
Yeah, this doesn't work the way you think it does. Try this instead:
return "Defense" + static_cast<int>(current_state_); | |
return std::string{"Defense"} + std::to_string(static_cast<int>(current_state_)); |
The basic problem you have is that "Offense"
is not of type std::string
but rather const char*
. It does get implicitly converted to an std::string
but not before you add an integer to the pointer.
@@ -14,6 +14,10 @@ std::optional<RobotIntent> Offense::derived_get_task(RobotIntent intent) { | |||
return state_to_task(intent); | |||
} | |||
|
|||
std::string Offense::return_current_state() { | |||
return "Offense" + static_cast<int>(current_state_); |
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.
Same problem as in Defense, the same fix should work. Note the clang-tidy warning present in Github. In the future, please check these warnings before requesting a review. I am happy to help you fix them if you don't know how.
Hey also, it doesn't matter too too much, but there's no need for you use a fork of our repo. Feel free to make a new branch and push them directly to our repo. |
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 great. Going to push a style-fix commit and then merge
Created new AgentState message type with one string field called state.
Made a pure virtual function in the position class that returns current state, each position returns the position name, and offense and defense also return integer value of state enum.
RobotFactoryPosition forwards this function call.