Fixes based on code review

soccer/src/soccer/strategy/agent/position/robot_factory_position.cpp

@@ -18,7 +18,7 @@ RobotFactoryPosition::RobotFactoryPosition(int r_id) : Position(r_id, "RobotFact
 
     std::string node_name{"robot_factory_position_"};
     _node = std::make_shared<rclcpp::Node>(node_name.append(std::to_string(robot_id_)));
-    test_play_sub_ = _node->create_subscription<rj_msgs::msg::OverridePosition>(
+    override_play_sub_ = _node->create_subscription<rj_msgs::msg::OverridePosition>(
         "override_position_for_robot", 1,
         [this](const rj_msgs::msg::OverridePosition::SharedPtr msg) { test_play_callback(msg); });
     _executor.add_node(_node);
@@ -127,11 +127,6 @@ void RobotFactoryPosition::handle_ready() {
 }
 
 void RobotFactoryPosition::update_position() {
-    // if (test_play_position_ == Strategy::OverridingPositions::OFFENSE) {
-    //  SPDLOG_INFO("OFFENSE SELECTED");
-    // set_current_position<Offense>();
-    // return;
-    //}
     bool manual_position_set = check_for_position_override();
     if (manual_position_set) return;
 
@@ -358,17 +353,17 @@ void RobotFactoryPosition::test_play_callback(
     const rj_msgs::msg::OverridePosition::SharedPtr message) {
     if (message->robot_id == this->robot_id_ &&
         message->overriding_position < Strategy::OverridingPositions::LENGTH) {
-        test_play_position_ =
+        override_play_position_ =
             static_cast<Strategy::OverridingPositions>(message->overriding_position);
     }
 }
 
 /**
- * Checks test_play_position_, which automatically updates when an override is set.
+ * Checks override_play_position_, which automatically updates when an override is set.
  * If it is anything but auto, set the current position to that position and return true.
  */
 bool RobotFactoryPosition::check_for_position_override() {
-    switch (test_play_position_) {
+    switch (override_play_position_) {
         case Strategy::OverridingPositions::OFFENSE: {
             set_current_position<Offense>();
             return true;

soccer/src/soccer/strategy/agent/position/robot_factory_position.hpp

@@ -113,11 +113,11 @@ class RobotFactoryPosition : public Position {
     std::unique_ptr<Position> current_position_;
 
     // subscription for test mode - Allows position overriding
-    rclcpp::Subscription<rj_msgs::msg::OverridePosition>::SharedPtr test_play_sub_;
+    rclcpp::Subscription<rj_msgs::msg::OverridePosition>::SharedPtr override_play_sub_;
     rclcpp::executors::SingleThreadedExecutor _executor;
     std::thread _executor_thread;
     void test_play_callback(const rj_msgs::msg::OverridePosition::SharedPtr message);
-    Strategy::OverridingPositions test_play_position_{Strategy::OverridingPositions::AUTO};
+    Strategy::OverridingPositions override_play_position_{Strategy::OverridingPositions::AUTO};
     rclcpp::Node::SharedPtr _node;
 
     std::optional<RobotIntent> derived_get_task(RobotIntent intent) override;

soccer/src/soccer/ui/main_window.cpp

@@ -2,6 +2,7 @@
 
 #include <ctime>
 #include <fstream>
+#include <regex>
 
 #include <QActionGroup>
 #include <QDateTime>
@@ -10,6 +11,7 @@
 #include <QFileDialog>
 #include <QInputDialog>
 #include <QMessageBox>
+#include <QObject>
 #include <QString>
 #include <boost/algorithm/string.hpp>
 #include <google/protobuf/descriptor.h>
@@ -187,7 +189,7 @@ MainWindow::MainWindow(Processor* processor, bool has_external_ref, QWidget* par
     _set_game_settings = _node->create_client<rj_msgs::srv::SetGameSettings>(
         config_server::topics::kGameSettingsSrv);
 
-    test_play_pub_ =
+    override_play_pub_ =
         _node->create_publisher<rj_msgs::msg::OverridePosition>("override_position_for_robot", 1);
 
     _executor.add_node(_node);
@@ -1048,11 +1050,10 @@ void MainWindow::on_actionUse_Multiple_Joysticks_toggled(bool value) {
 
 void MainWindow::on_goalieID_currentIndexChanged(int value) {
     update_cache(_game_settings.request_goalie_id, value - 1, &_game_settings_valid);
-    // This is a convoluted scheme to figure out who the goalie is
-    // May not be necessary later
     QString goalieNum = _ui.goalieID->currentText();
     bool goalieNumIsInt{false};
     int goalieInt = goalieNum.toInt(&goalieNumIsInt);
+    current_goalie_num_ = goalieInt;
     if (goalieNumIsInt)
         setGoalieDropdown(goalieInt);
     else
@@ -1173,7 +1174,11 @@ void MainWindow::updateDebugLayers(const LogFrame& frame) {
 
 /**
  * The following methods are event listeners for the manual position assignments.
- * TODO: implement all of these
+ * In the QT5 event handling system, these methods are automatically connected to the
+ * QObject with the relevant name due to their naming scheme and their labels as Q_SLOT functions.
+ * 
+ * For this reason, all of these methods are REQUIRED by the UI - Consolidating them into one method
+ * produces unnecessary complications.
  */
 void MainWindow::on_positionReset_0_clicked() { onResetButtonClicked(0); }
 
@@ -1207,6 +1212,7 @@ void MainWindow::on_positionReset_14_clicked() { onResetButtonClicked(14); }
 
 void MainWindow::on_positionReset_15_clicked() { onResetButtonClicked(15); }
 
+
 void MainWindow::on_robotPosition_0_currentIndexChanged(int value) {
     onPositionDropdownChanged(0, value);
 }
@@ -1276,23 +1282,18 @@ void MainWindow::onResetButtonClicked(int robot) {
     message.robot_id = robot;
     message.overriding_position = 0;
 
-    test_play_pub_->publish(message);
+    override_play_pub_->publish(message);
     position_reset_buttons.at(robot)->setEnabled(false);
     robot_pos_selectors.at(robot)->setCurrentIndex(0);
 }
 
 void MainWindow::onPositionDropdownChanged(int robot, int position_number) {
-    // This is a convoluted scheme to figure out who the goalie is
-    // May not be necessary later
-    QString goalieNum = _ui.goalieID->currentText();
-    bool goalieNumIsInt{false};
-    int goalieInt = goalieNum.toInt(&goalieNumIsInt);
-    if (!goalieNumIsInt || goalieInt != robot) {
+    if (current_goalie_num_ != robot) {
         rj_msgs::msg::OverridePosition message;
         message.robot_id = robot;
         message.overriding_position = position_number;
 
-        test_play_pub_->publish(message);
+        override_play_pub_->publish(message);
         if (position_number != 0) {
             position_reset_buttons.at(robot)->setEnabled(true);
         } else {

soccer/src/soccer/ui/main_window.hpp

@@ -198,6 +198,7 @@ private Q_SLOTS:
     void on_positionReset_13_clicked();
     void on_positionReset_14_clicked();
     void on_positionReset_15_clicked();
+
 
 Q_SIGNALS:
     // signal used to let widgets that we're viewing a different log frame now
@@ -221,6 +222,8 @@ private Q_SLOTS:
     Processor* const _processor;
     bool _has_external_ref;
 
+    int current_goalie_num_ {0};
+
     // Log history, copied from Logger.
     // This is used by other controls to get log data without having to copy it
     // again from the Logger.
@@ -249,7 +252,6 @@ private Q_SLOTS:
      * Callback when a reset button is clicked
      */
     void onResetButtonClicked(int robot);
-    // TODO: Add a publisher and a subscriber to implement these
 
     // Tree items that are not in LogFrame
     QTreeWidgetItem* _frameNumberItem{};
@@ -296,7 +298,7 @@ private Q_SLOTS:
     rclcpp::Node::SharedPtr _node;
     rclcpp::Client<rj_msgs::srv::QuickCommands>::SharedPtr _quick_commands_srv;
     rclcpp::Client<rj_msgs::srv::SetGameSettings>::SharedPtr _set_game_settings;
-    rclcpp::Publisher<rj_msgs::msg::OverridePosition>::SharedPtr test_play_pub_;
+    rclcpp::Publisher<rj_msgs::msg::OverridePosition>::SharedPtr override_play_pub_;
     rclcpp::executors::SingleThreadedExecutor _executor;
     std::thread _executor_thread;