Skip to content

Patch/pytest#110

Merged
energy-in-joles merged 26 commits intomainfrom
patch/pytest
Mar 1, 2026
Merged

Patch/pytest#110
energy-in-joles merged 26 commits intomainfrom
patch/pytest

Conversation

@energy-in-joles
Copy link
Member

@energy-in-joles energy-in-joles commented Feb 27, 2026

  1. Fix bug that was causing ctrl+c to not stop pytest execution
  2. add exp_ball field 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.
  3. prevent misuse of teleport_ball or teleport_robot function being used to remove robots off pitch by bounds checking
  4. fix infinite loop issue in run_test when game is misconfigured and GameGater waits indefinitely
  5. Move all motion_planning strategies out of test and into strategy folder
  6. Set all tests that do not expect a ball to be exp_ball=False

Note for 3, ball or robot should NEVER be teleported off the pitch using this method. That is what set_robot_presence and remove_ball is 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.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_ball plumbing from AbstractStrategyStrategyRunnerPositionRefiner / GameGater, plus new tests validating behavior.
  • Improve interrupt handling in StrategyRunner (SIGINT/ctrl+c) and add a misconfig timeout path in GameGater for 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>
Copilot AI review requested due to automatic review settings February 27, 2026 00:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.

Copilot AI review requested due to automatic review settings February 27, 2026 01:10
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

@energy-in-joles energy-in-joles enabled auto-merge (squash) February 27, 2026 01:26
Copilot AI review requested due to automatic review settings February 27, 2026 10:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_presence has 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.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_presence has 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.

Copilot AI review requested due to automatic review settings February 27, 2026 12:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • PointCycleStrategy is no longer re-exported from utama_core.strategy.examples (it was removed from __init__.py). If external code relies on from 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_presence has 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.

Copilot AI review requested due to automatic review settings February 27, 2026 13:17
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 32 changed files in this pull request and generated 3 comments.

Copilot AI review requested due to automatic review settings February 27, 2026 13:40
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 32 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings February 27, 2026 14:08
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 34 changed files in this pull request and generated no new comments.

if new_ball is None and not self.exp_ball:
pass
else:
# For filtering and vanishing
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ball

This 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@energy-in-joles energy-in-joles merged commit f9b58bd into main Mar 1, 2026
2 checks passed
@energy-in-joles energy-in-joles deleted the patch/pytest branch March 1, 2026 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:minor Minor changes to main

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants