Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the StrategyRunner/test harness and simulator utilities to better handle misconfiguration and improve runtime robustness, including support for running without a ball (exp_ball=False) and preventing some invalid teleport usage.
Changes:
- Add
exp_ballplumbing fromAbstractStrategy→StrategyRunner→PositionRefiner/GameGater, plus new tests validating behavior. - Improve interrupt handling in
StrategyRunner(SIGINT/ctrl+c) and add a misconfig timeout path inGameGaterfor RSIM. - Add bounds checking to RSIM env teleport APIs and introduce internal “unrestricted” ball teleport/removal helpers.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
utama_core/run/strategy_runner.py |
Adds exp_ball parameter, asserts strategy/runner consistency, removes RSIM ball when needed, and updates refiner initialization + SIGINT handling. |
utama_core/data_processing/refiners/position.py |
Adds exp_ball flag and adjusts ball filtering/imputation logic accordingly. |
utama_core/run/game_gater.py |
Adds exp_ball gate condition and RSIM misconfig timeout behavior. |
utama_core/strategy/common/abstract_strategy.py |
Introduces exp_ball as a strategy-level expectation flag. |
utama_core/rsoccer_simulator/src/ssl/ssl_gym_base.py |
Adds bounds checks for teleport APIs and adds internal ball removal/unrestricted teleport helpers. |
utama_core/rsoccer_simulator/src/ssl/envs/standard_ssl.py |
Updates initial-frame generation to support a “ball exists” toggle. |
utama_core/team_controller/src/controllers/sim/rsim_robot_controller.py |
Tightens PVP manager typing/logic and uses _env consistently. |
utama_core/tests/strategy_runner/test_exp_ball.py |
New test coverage for exp_ball mismatch/agreement and RSIM integration expectations. |
utama_core/tests/strategy_runner/test_runner_misconfig.py |
Updates misconfig assertions to include exp_ball. |
utama_core/tests/strategy_runner/strat_runner_test_utils.py |
Adds exp_ball attribute to DummyStrategy used by runner tests. |
utama_core/tests/abstract_strategy/test_assertions.py |
Adds exp_ball attribute to DummyStrategy used in abstract strategy assertion tests. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
utama_core/team_controller/src/controllers/sim/grsim_controller.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
utama_core/team_controller/src/controllers/common/sim_controller_abstract.py:79
AbstractSimController.set_robot_presencehas parameter order(is_team_yellow, robot_id, ...), but both implementations and call sites in this PR use(robot_id, is_team_yellow, ...). Align the abstract signature (and docstring arg names) with actual usage to avoid confusion and type-checking issues.
@abc.abstractmethod
def set_robot_presence(self, is_team_yellow: bool, robot_id: int, should_robot_be_present: bool) -> None:
"""Sets a robot's presence on the field by teleporting it to a specific location or removing it from the field.
Args:
robot_id (int): The unique ID of the robot.
team_colour_is_blue (bool): Whether the robot belongs to the blue team. If False, it's assumed to be yellow.
should_robot_be_present (bool): If True, the robot will be placed on the field; if False, it will be removed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
utama_core/team_controller/src/controllers/common/sim_controller_abstract.py:79
AbstractSimController.set_robot_presencehas its parameters ordered as(is_team_yellow, robot_id, should_robot_be_present), but both implementations (GRSimController/RSimController) and call sites (e.g.,StrategyRunner._load_sim) use(robot_id, is_team_yellow, ...). This mismatch makes overrides inconsistent and makes it easy to call the method with swapped arguments (especially for callers using positional args). Please reorder the abstract method signature (and its docstring parameter descriptions) to match the concrete controllers and existing usage (robot_id first).
@abc.abstractmethod
def set_robot_presence(self, is_team_yellow: bool, robot_id: int, should_robot_be_present: bool) -> None:
"""Sets a robot's presence on the field by teleporting it to a specific location or removing it from the field.
utama_core/team_controller/src/controllers/common/sim_controller_abstract.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
utama_core/strategy/examples/init.py:18
PointCycleStrategyis no longer re-exported fromutama_core.strategy.examples(it was removed from__init__.py). If external code relies onfrom utama_core.strategy.examples import PointCycleStrategy, this is a breaking change. If the removal wasn’t intentional, consider adding the import back (or documenting the API change).
from utama_core.strategy.examples.defense_strategy import DefenceStrategy
from utama_core.strategy.examples.go_to_ball_ex import GoToBallExampleStrategy
from utama_core.strategy.examples.motion_planning.multi_robot_navigation_strategy import (
MultiRobotNavigationStrategy,
)
from utama_core.strategy.examples.motion_planning.oscillating_obstacle_strategy import (
OscillatingObstacleStrategy,
)
from utama_core.strategy.examples.motion_planning.simple_navigation_strategy import (
SimpleNavigationStrategy,
)
from utama_core.strategy.examples.one_robot_placement_strategy import (
RobotPlacementStrategy,
)
from utama_core.strategy.examples.random_movement_strategy import RandomMovementStrategy
from utama_core.strategy.examples.startup_strategy import StartupStrategy
from utama_core.strategy.examples.two_robot_placement import TwoRobotPlacementStrategy
utama_core/team_controller/src/controllers/common/sim_controller_abstract.py:80
AbstractSimController.set_robot_presencehas parameters ordered as(is_team_yellow, robot_id, ...), but both concrete implementations (e.g.,GRSimController/RSimController) and call sites use(robot_id, is_team_yellow, ...). This mismatch makes keyword-argument calls error-prone and breaks type-checking. Align the abstract method signature with the implementations/call pattern.
@abc.abstractmethod
def set_robot_presence(self, is_team_yellow: bool, robot_id: int, should_robot_be_present: bool) -> None:
"""Sets a robot's presence on the field by teleporting it to a specific location or removing it from the field.
utama_core/team_controller/src/controllers/sim/grsim_controller.py
Outdated
Show resolved
Hide resolved
utama_core/team_controller/src/controllers/common/sim_controller_abstract.py
Outdated
Show resolved
Hide resolved
utama_core/strategy/examples/motion_planning/random_movement_strategy.py
Show resolved
Hide resolved
utama_core/strategy/examples/motion_planning/random_movement_strategy.py
Show resolved
Hide resolved
utama_core/team_controller/src/controllers/common/sim_controller_abstract.py
Outdated
Show resolved
Hide resolved
utama_core/strategy/examples/motion_planning/random_movement_strategy.py
Show resolved
Hide resolved
| if new_ball is None and not self.exp_ball: | ||
| pass | ||
| else: | ||
| # For filtering and vanishing |
There was a problem hiding this comment.
Suggestion: The pass + else pattern is a bit hard to parse at a glance. Consider inverting the condition:
if new_ball is not None or self.exp_ball:
# For filtering and vanishing
if self.filtering and self._filter_running:
new_ball = self.kalman_filter_ball.filter_data(
new_ball,
game_frame.ball,
time_elapsed,
)
elif new_ball is None:
# If none, take the ball from the last frame of the game
new_ball = game_frame.ballThis removes the empty pass branch and makes the intent clearer: "run the filter logic unless there's no ball and we don't expect one."
| This method creates a command for teleporting the ball and sends it to the simulator. | ||
| """ | ||
| ... | ||
| if self.exp_ball is False: |
There was a problem hiding this comment.
Nit: is False works here since it's always a bool, but not self.exp_ball is more Pythonic and consistent with the rest of the PR (e.g., not self.exp_ball is used in position.py:159).\n\npython\nif not self.exp_ball:\n
exp_ballfield to parameterise if ball should be present on field or not. Only throw error if strat requires it and exp_ball is False on the runner.Note for 3, ball or robot should NEVER be teleported off the pitch using this method. That is what
set_robot_presenceandremove_ballis for now it confuses the system. As a general requirement, we should not have any changes to the number of robots or balls on the pitch during execution. This is unexpected behaviour and will cause the filters to hallucinate.