OffensePlayTest checking that there is Never Excessive Dribbling #3536

adrianchan787 · 2025-11-30T11:45:32Z

Description

Added a validation status check in offense play test that checks if the friendly team has excessively dribbled
Changed the validation status check (NeverExcessivelyDribbles and EventuallyStartsExcessiveDribbles) to look at the dribble_displacement field in World instead of the previous "brute force" method; this (theoretically) aligns more with the AutoRef's implementation of excessive dribbling
Added a testing file for the validation status check

Testing Done

- Reran all pytests that used NeverExcessivelyDribbles; some of them were flaky for unrelated reasons e.g. pass_defender_test had a robot enter region validation error first time around
- Created a testing file that tests never excessively and eventually starts dribbling, using the dribble_tactic. (since the main dribbling done in offense_play_test is in dribble_tactic). These tests typically pass but are also flaky; I'll expand more in the comments at the end.

Resolved Issues

Resolves #3452

Length Justification and Key Files to Review

Review Checklist

It is the reviewers responsibility to also make sure every item here has been covered

[x ] Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
[ x] Remove all commented out code
[ x] Remove extra print statements: for example, those just used for testing
[ x] Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue

Additional Comments

Not really an addition and more of a few observations and questions. Wanted to still make this PR since I feel like it's not directly related to my current ticket (and also because I've dragged this on for too long lol).
Was looking at how the Autoref implemented dribble distance, and from what I understood it seems to compare initial position of the robot to the final position of the ball (see the code snippet below, taken from https://github.com/TIGERs-Mannheim/AutoReferee/tree/53063578e38ac4818849df3196b32a856a5fa41d). If I'm mistaken, please tell me and I'll edit the comments in my branch lol

However, because they use robot to ball and not e.g. ball to ball, the dribble displacement field is extremely inconsistent. For example, in my tests, if you start the robot in the same location as the ball, you get a different dribble displacement than if you started the robot a meter way. (which I think makes sense? since the orientation with which you start dribbling will probably be different).
Similarly, that's why the maximum distance between the initial point location of the ball and its location after dribbling is greater than 1 m. From tests I got up to 1.03 m. However, this maximum limit was pretty flaky for me, and fluctuated between 1.03 and 1.01 m. (which is why there's a decent chance my code won't pass my own tests).
So I was wondering if this is an issue? On one hand, (I think?) if this mirrors the Autoref's behaviour, it doesn't matter how inconsistent it is. On the other, I know William said the 1 meter max dribbling was a pretty strict limit, so would it be worthwhile to be conservative and use like 0.98 max distance in testing and tactics?

Sorry if that was a bit unclear, please let me know if it was. Thanks!

adrianchan787 · 2025-11-30T11:53:46Z

(oops really sorry not sure if i was supposed to resolve the merge conflict)

…/adrianchan787/Software into adrian/offense_play_test_dribble

nycrat

Added some comments, just some refactoring changes recommended but the new implementation of excessive_dribbing validation seems good and the test are thorough

nycrat · 2025-12-02T18:17:37Z

src/software/ai/hl/stp/tactic/dribble/excessive_dribble_test.py

+def test_excessive_dribbling(
+    initial_location,
+    dribble_destination,
+    final_dribble_orientation,
+    allow_excessive_dribbling,
+    simulated_test_runner,
+):


Perhaps instead of having a separate function for testing excessive dribbling, you could pass in a parameter such as "should_excessively_dribble" which would control if the test expects excessive dribbling or not.

This would reduce the repeated code between the two test functions

nycrat · 2025-12-02T18:17:40Z

src/software/simulated_tests/excessive_dribbling.py

    create_validation_geometry,
    create_validation_types,
 )
-from typing import override


The @override decorators should be kept, which I think was removed during your merge conflict. Just add these back.

nycrat · 2025-12-02T18:22:35Z

src/software/ai/hl/stp/tactic/dribble/excessive_dribble_test.py

+    initial_location,
+    dribble_destination,
+    final_dribble_orientation,
+    allow_excessive_dribbling,


allow_excessive_dribbling might not be a necessary parameter, since all the tests use True. Is there a scenario where allow_excessive_dribbling should be set to False?

nycrat · 2025-12-02T18:27:26Z

src/software/ai/hl/stp/play/offense/offense_play_test.py

 import software.python_bindings as tbots_cpp
 from proto.play_pb2 import Play, PlayName
+
+from software.simulated_tests.excessive_dribbling import NeverExcessivelyDribbles


Small nit, I would just use the wildcard import * for consistency with the rest of the validation imports.

But actually there are instances of importing specific names in python in our codebase. This might be another refactoring issue to make our python code more consistent

I would leave it as is, using the module imports. It is a better practice to only import what you need so as not to pollute the namespace. Additionally, just as a fun fact, try never to import a raw file as it can lead to some unpredictable behaviour during the import process: https://stackoverflow.com/questions/5748946/pythonic-way-to-resolve-circular-import-statements

Yea I also agree with just importing the required functions, should this be a different issue to get rid of all wildcard imports from our python code ?

Andrewyx · 2025-12-04T04:27:20Z

src/software/ai/hl/stp/play/offense/offense_play_test.py

 import software.python_bindings as tbots_cpp
 from proto.play_pb2 import Play, PlayName
+
+from software.simulated_tests.excessive_dribbling import NeverExcessivelyDribbles


I would leave it as is, using the module imports. It is a better practice to only import what you need so as not to pollute the namespace. Additionally, just as a fun fact, try never to import a raw file as it can lead to some unpredictable behaviour during the import process: https://stackoverflow.com/questions/5748946/pythonic-way-to-resolve-circular-import-statements

Andrewyx · 2025-12-04T04:32:11Z

src/software/ai/hl/stp/tactic/dribble/excessive_dribble_test.py

+from software.simulated_tests.robot_enters_region import *
+from software.simulated_tests.ball_enters_region import *
+from software.simulated_tests.ball_moves_in_direction import *
+from software.simulated_tests.friendly_has_ball_possession import *
+from software.simulated_tests.ball_speed_threshold import *
+from software.simulated_tests.robot_speed_threshold import *
+from software.simulated_tests.excessive_dribbling import *


See comment above regarding module imports. It makes it easier for reviewers to catch unused imports if exact macro names are stated.

Andrewyx · 2025-12-04T04:35:39Z

src/software/simulated_tests/excessive_dribbling.py

+        max_dribble_length: float = 1.00,
+        max_dribble_error_margin: float = 0.05,


nit: these are magic numbers, is there some way we can make these constants somewhere? Better yet, they should co-align with whatever thresholds we might already have in our logic.

Andrewyx · 2025-12-04T06:19:51Z

src/software/simulated_tests/excessive_dribbling.py

+        if world.HasField("dribble_displacement"):
+            dribble_disp = world.dribble_displacement
+            dist = tbots_cpp.createSegment(dribble_disp).length()
+            if dist > (max_dribble_length - max_dribble_error_margin):


Should this be (dist-max_dribble_length) > max_dribble_error_margin?

I think its correct right now since it states earlier in the file that this is a margin for a conservative estimate for allowed dribble distance, so it should make the allowed distance shorter to make sure we dont go over the strict limit.

StarrryNight · 2025-12-10T07:01:34Z

src/software/simulated_tests/excessive_dribbling.py

    """Checks if any friendly robot is excessively dribbling the ball, i.e. for over 1m."""

    def __init__(self):
        self.continous_dribbling_start_point = None


Is this line still useful? The usage of continous_dribbling_start_point seems to be completely removed in this PR.

adrianchan787 added 6 commits November 1, 2025 12:24

Added NeverExcessivelyDribbles functionality into OffensePlayTest

ec7010a

intermediate commit

b690a45

intermediate change

c339388

Switched to world proto subscription measurement

a4f6bc0

Added excessive dribbling functionality and testing

544bc55

Reverted verbosity change which was for print testing previously

d928b5d

adrianchan787 requested review from Andrewyx, sauravbanna and williamckha as code owners November 30, 2025 11:45

Merge branch 'master' into adrian/offense_play_test_dribble

a3929cf

[pre-commit.ci lite] apply automatic fixes

32cc42d

adrianchan787 marked this pull request as draft November 30, 2025 22:10

adrianchan787 added 2 commits November 30, 2025 14:49

Added error margin of 0.05 to max dribbling distance

5b0b98c

Merge branch 'adrian/offense_play_test_dribble' of https://github.com…

2fb03f8

…/adrianchan787/Software into adrian/offense_play_test_dribble

adrianchan787 marked this pull request as ready for review November 30, 2025 23:01

[pre-commit.ci lite] apply automatic fixes

dff8853

nycrat requested changes Dec 2, 2025

View reviewed changes

Andrewyx reviewed Dec 4, 2025

View reviewed changes

StarrryNight reviewed Dec 10, 2025

View reviewed changes

		max_dribble_length: float = 1.00,
		max_dribble_error_margin: float = 0.05,

OffensePlayTest checking that there is Never Excessive Dribbling #3536

Are you sure you want to change the base?

OffensePlayTest checking that there is Never Excessive Dribbling #3536

Uh oh!

Conversation

adrianchan787 commented Nov 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing Done

Resolved Issues

Length Justification and Key Files to Review

Review Checklist

Additional Comments

Uh oh!

adrianchan787 commented Nov 30, 2025

Uh oh!

nycrat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nycrat Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adrianchan787 commented Nov 30, 2025 •

edited

Loading

nycrat Dec 4, 2025 •

edited

Loading