Implemented Chip Pass Modeling #3229

sauravbanna · 2024-06-20T08:25:51Z

Please fill out the following before requesting review on this PR

Description

This PR implements a new class for Chip Passes. It allows us to calculate the route of the chip pass from a start and end point.

Testing Done

Resolved Issues

Length Justification and Key Files to Review

Review Checklist

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

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.
Remove all commented out code
Remove extra print statements: for example, those just used for testing
Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue

itsarune · 2024-06-25T06:18:21Z

src/extlibs/er_force_sim/src/amun/simulator/simball.cpp

+#include "software/logger/logger.h"
+#include "proto/message_translation/tbots_protobuf.h"
+
+// #include "software/logger/logger.h"


remember to re-comment this back in

itsarune · 2024-06-25T06:18:38Z

src/extlibs/er_force_sim/src/amun/simulator/simball.cpp

+ LOG(PLOTJUGGLER) << *createPlotJugglerValue({
+ {"dist", std::sqrt(pow(p.x() - 0, 2) + pow(p.y() - 0, 2))},
+ {"z", p.z()}


remember to remove this when you're done

itsarune · 2024-06-25T06:19:02Z

src/proto/message_translation/ssl_simulation_robot_control.cpp

@@ -79,10 +80,13 @@ std::unique_ptr<SSLSimulationProto::RobotCommand> getRobotCommandFromDirectContr
 float numerator =
 range *
 static_cast<float>(ACCELERATION_DUE_TO_GRAVITY_METERS_PER_SECOND_SQUARED);
- float denominator = static_cast<float>(2.0f * (chip_angle * 2.0f).sin());
+ float denominator = static_cast<float>(2.0f * (chip_angle * 2.0f).22());


itsarune · 2024-06-25T06:19:18Z

src/proto/message_translation/ssl_simulation_robot_control.cpp

+ LOG(PLOTJUGGLER) << *createPlotJugglerValue({
+ {"speed", chip_speed}
+ });


remember to remove when you're done

itsarune · 2024-06-25T06:20:25Z

src/software/ai/hl/stp/tactic/chip/chip_tactic_test.cpp

+ // std::vector<ValidationFunction> terminating_validation_functions = {
+ //  [angle_to_kick_at, tactic](std::shared_ptr<World> world_ptr,
+ //  ValidationCoroutine::push_type& yield) {
+ //  while (!tactic->done())
+ //  {
+ //  yield("Tactic did not complete!");
+ //  }
+ //  ballKicked(angle_to_kick_at, world_ptr, yield);
+ //  }};


remember to comment this back in

itsarune · 2024-06-25T06:21:18Z

src/software/ai/hl/stp/tactic/chip/chip_tactic_test.cpp

 // place the ball directly to the left of the robot
- std::make_tuple(Vector(0, 0.5), Angle::zero()),
- // place the ball directly to the right of the robot
- std::make_tuple(Vector(0, -0.5), Angle::zero()),
- // place the ball directly infront of the robot
- std::make_tuple(Vector(0.5, 0), Angle::zero()),
- // place the ball directly behind the robot
- std::make_tuple(Vector(-0.5, 0), Angle::zero()),
- // place the ball in the robots dribbler
- std::make_tuple(Vector(ROBOT_MAX_RADIUS_METERS, 0), Angle::zero()),
- // Repeat the same tests but kick in the opposite direction
- // place the ball directly to the left of the robot
- std::make_tuple(Vector(0, 0.5), Angle::half()),
- // place the ball directly to the right of the robot
- std::make_tuple(Vector(0, -0.5), Angle::half()),
- // place the ball directly infront of the robot
- std::make_tuple(Vector(0.5, 0), Angle::half()),
- // place the ball directly behind the robot
- std::make_tuple(Vector(-0.5, 0), Angle::half()),
- // place the ball in the robots dribbler
- std::make_tuple(Vector(ROBOT_MAX_RADIUS_METERS, 0), Angle::zero())));
+ // std::make_tuple(Vector(0, 0.5), Angle::zero()),
+ // // place the ball directly to the right of the robot
+ // std::make_tuple(Vector(0, -0.5), Angle::zero())
+ // // place the ball directly infront of the robot
+ std::make_tuple(Vector(0.5, 0), Angle::zero())
+ // // place the ball directly behind the robot
+ // std::make_tuple(Vector(-0.5, 0), Angle::zero()),
+ // // place the ball in the robots dribbler
+ // std::make_tuple(Vector(ROBOT_MAX_RADIUS_METERS, 0), Angle::zero()),
+ // // Repeat the same tests but kick in the opposite direction
+ // // place the ball directly to the left of the robot
+ // std::make_tuple(Vector(0, 0.5), Angle::half()),
+ // // place the ball directly to the right of the robot
+ // std::make_tuple(Vector(0, -0.5), Angle::half()),
+ // // place the ball directly infront of the robot
+ // std::make_tuple(Vector(0.5, 0), Angle::half()),
+ // // place the ball directly behind the robot
+ // std::make_tuple(Vector(-0.5, 0), Angle::half()),
+ // // place the ball in the robots dribbler
+ // std::make_tuple(Vector(ROBOT_MAX_RADIUS_METERS, 0), Angle::zero())
+ ));


remember to revert these when you're done

itsarune · 2024-06-25T06:22:32Z

src/software/ai/passing/base_pass.cpp

+
+double BasePass::length() const 
+{
+ return std::sqrt(std::pow(receiverPoint().x() - passerPoint().x(), 2) + std::pow(receiverPoint().y() - passerPoint().y(), 2));


Suggested change

return std::sqrt(std::pow(receiverPoint().x() - passerPoint().x(), 2) + std::pow(receiverPoint().y() - passerPoint().y(), 2));

return (receiverPoint() - passerPoint()).length()

itsarune · 2024-06-25T06:22:57Z

src/software/ai/passing/base_pass.cpp

+// std::ostream& printHelper(std::ostream& output_stream, const BasePass& pass)
+// {
+// output_stream << "Pass from " << pass.passer_point 
+// << " to: " << pass.receiver_point;
+
+// return output_stream;
+// }


remember to remove this when you're done

itsarune · 2024-06-25T06:23:12Z

src/software/ai/passing/base_pass.h

+// (receive_location_x, receive_location_y)
+static const int NUM_PARAMS_TO_OPTIMIZE = 2;
+
+class BasePass 


missing class docs

itsarune · 2024-06-25T06:24:08Z

src/software/ai/passing/base_pass.h

+ virtual Duration estimatePassDuration() const
+ {
+ return Duration::fromSeconds(0);
+ }


this seems a bit of a useless implementation, why not do virtual Duration estimatePassDuration() const = 0?

itsarune · 2024-06-25T06:24:34Z

src/software/ai/passing/base_pass.h

+ /**
+ // * Implement the "<<" operator for printing
+ // *
+ // * @param output_stream The stream to print to
+ // * @param pass The pass to print
+ // * @return The output stream with the string representation of the class appended
+ // */
+ // friend std::ostream& printHelper(std::ostream& output_stream, const BasePass& pass);


why is this declared as a friend function?

itsarune · 2024-06-25T06:25:00Z

src/software/ai/passing/chip_pass.cpp

+ double last_bounce_range;
+ double length_to_go = length();
+
+ std::cout << "LENGTH: " << length_to_go << std::endl;


delete when done

itsarune · 2024-06-25T06:25:33Z

src/software/ai/passing/chip_pass.cpp

+ std::cout << "LENGTH: " << length_to_go << std::endl;
+
+ // if we cover more than 90% of the pass, stop calculating
+ while(length_to_go >= pass_length * 0.1)


0.1 should be a constant of some kind

itsarune · 2024-06-25T06:25:49Z

src/software/ai/passing/chip_pass.cpp

+ double bounce_height = getBounceHeightFromDistanceTraveled(length_to_go);
+ last_bounce_range = getBounceRangeFromBounceHeight(bounce_height);
+ length_to_go -= last_bounce_range;
+ std::cout << "LENGTH: " << length_to_go << std::endl;


remove std::cout when you're done

itsarune · 2024-06-25T06:26:24Z

src/software/ai/passing/chip_pass.cpp

+
+double ChipPass::getBounceHeightFromDistanceTraveled(double distance_traveled)
+{
+ return 0.0306 * std::exp(-2.04 * (distance_traveled - pass_length));


you should link a picture of the plot for future software members

itsarune · 2024-06-25T06:26:50Z

src/software/ai/passing/chip_pass.cpp

+
+double ChipPass::getBounceRangeFromBounceHeight(double bounce_height)
+{
+ return 4 * bounce_height * (std::cos(ROBOT_CHIP_ANGLE_RADIANS) / std::sin(ROBOT_CHIP_ANGLE_RADIANS));


you should add some comments about where this derivation comes form

itsarune · 2024-06-25T06:27:32Z

src/software/ai/passing/chip_pass.cpp

+
+Duration ChipPass::estimatePassDuration() const
+{
+ return Duration::fromSeconds(0);


do you have plans of estimating the time? or is it intentionally 0?

itsarune · 2024-06-25T06:28:21Z

src/software/ai/passing/chip_pass.h

+
+ double firstBounceRange();
+
+ virtual Duration estimatePassDuration() const;


you should use override so that it checks at compile time that you're overriding the base class implementation corrctly

itsarune · 2024-06-25T06:28:28Z

src/software/ai/passing/chip_pass.h

+#include "software/ai/passing/base_pass.h"
+#include "shared/constants.h"
+
+class ChipPass: public BasePass


missing docs

functions in this class are missing docs

itsarune · 2024-06-25T06:28:59Z

src/software/ai/passing/chip_pass_test.cpp

+
+ EXPECT_EQ(Point(1, 2), p.passerPoint());
+ EXPECT_EQ(Point(3, 4), p.receiverPoint());
+ std::cout << p.firstBounceRange() << std::endl;


remove std::cout when you're done

itsarune · 2024-06-25T06:29:46Z

src/software/ai/passing/pass.h

- * @return An estimate of how long the pass will take, from kicking to receiving
- */
- Duration estimatePassDuration() const;
+ virtual Duration estimatePassDuration() const;


Suggested change

virtual Duration estimatePassDuration() const;

Duration estimatePassDuration() const = override;

itsarune

I know you're still working on the details of this project but I left some interim feedback

itsarune · 2024-06-25T06:31:34Z

is this PR scoped to modelling chips? what's the plan for generating more chip passes?

itsarune · 2024-07-07T19:07:01Z

src/extlibs/er_force_sim/src/amun/simulator/simrobot.cpp

 return std::max((btScalar)0, relBallSpeed.y()) -
 qBound((btScalar)0, (btScalar)0.5 * relBallSpeed.y(),
 (btScalar)0.5 * dirFloor);
 }
 };
 const float speedCompensation = getSpeedCompensation();
- ball->kick(t * btVector3(0, dirFloor * power + speedCompensation, dirUp * power) *
+ ball->kick(t * btVector3(0, dirFloor * power - speedCompensation, dirUp * power) *


why do you need this change?

itsarune · 2024-07-07T19:08:18Z

src/software/ai/passing/base_pass.h

+ return Duration::fromSeconds(0);
+ }
+
+ virtual Duration estimateTimeToPoint(Point& point) const;


itsarune · 2024-07-07T19:09:42Z

src/software/ai/passing/chip_pass.cpp

+ // if we cover more than 90% of the pass, stop calculating
+ while(length_to_go >= pass_length * 0.2)


90% should be pass_length * 0.1

itsarune · 2024-07-07T19:10:43Z

src/software/ai/passing/chip_pass.cpp

+
+Duration ChipPass::estimateTimeToPoint(Point& point) const
+{
+ return Duration::fromSeconds(0);


itsarune · 2024-07-07T19:11:13Z

src/software/ai/passing/chip_pass.h

+ *
+ * @param passer_point the starting point of the pass
+ * @param pass_destination the end point of the pass
+ * @return the ChipPass constructed from the start and end points


we don't usually add @return to constructors

itsarune · 2024-07-07T19:11:22Z

src/software/ai/passing/chip_pass.h

+ */
+ ChipPass(Point passer_point, Point receiver_point);
+
+ double firstBounceRange();


itsarune · 2024-07-07T19:11:41Z

src/software/ai/passing/chip_pass.h

+ double calcFirstBounceRange();
+
+ double getBounceHeightFromDistanceTraveled(double distance_traveled);
+
+ double getBounceRangeFromBounceHeight(double bounce_height);


itsarune · 2024-07-07T19:13:17Z

src/software/ai/passing/cost_function.cpp

+double rateGroundPass(const World& world, const Pass& pass, const Rectangle& zone,
+ TbotsProto::PassingConfig passing_config)
+{
+ return ratePass(world, pass, zone, world.enemyTeam(), passing_config);
+}
+
+double rateChipPass(const World& world, const ChipPass& pass, const Rectangle& zone,
+ TbotsProto::PassingConfig passing_config)
+{
+ return ratePass(world, pass, zone, world.enemyTeam(), passing_config);
+}


if you're using the same implementation, does it make sense to have separate rateGroundPass and rateChipPass methods?

itsarune · 2024-07-07T19:13:46Z

src/software/ai/passing/cost_function.cpp

- ball_time_to_closest_pass_point =
- Duration::fromSeconds(std::numeric_limits<int>::max());
- }
+ Duration ball_time_to_closest_pass_point = pass.estimateTimeToPoint(closest_point_on_pass_to_robot);


you still need to do a check for division by 0 here

itsarune · 2024-07-07T19:14:11Z

src/software/ai/passing/cost_function.cpp

@@ -246,12 +250,6 @@ double ratePassFriendlyCapability(const Team& friendly_team, const Pass& pass,
 return 0;
 }



you still need a check for division by 0

itsarune · 2024-07-07T19:14:24Z

src/software/ai/passing/cost_function.h

+double rateGroundPass(const World& world, const Pass& pass, const Rectangle& zone,
+ TbotsProto::PassingConfig passing_config);
+
+double rateChipPass(const World& world, const ChipPass& pass, const Rectangle& zone,
+ TbotsProto::PassingConfig passing_config)


itsarune · 2024-07-07T19:15:37Z

src/software/ai/passing/pass.cpp

@@ -2,8 +2,7 @@


 Pass::Pass(Point passer_point, Point receiver_point, double pass_speed_m_per_s)


maybe rename this to GroundPass

itsarune · 2024-07-07T19:16:40Z

src/software/ai/passing/pass.h

- * @return true if the Passes are equal and false otherwise
- */
- bool operator==(const Pass& other) const;
+ friend std::ostream& operator<<(std::ostream& output_stream, const Pass& pass);


why do we need a friend here?

itsarune

left a few more comments

itsarune · 2024-07-27T06:40:39Z

src/software/ai/passing/chip_pass.cpp

+ pass_length = length();
+ first_bounce_range_m = calcFirstBounceRange();
+ skip_area = calcSkipArea(); 
+ std::cout << "SKIP: " << skip_area << std::endl;


delete comment

itsarune · 2024-07-27T06:46:44Z

src/software/ai/passing/chip_pass.cpp

+ Point point_on_pass(passer_point.x() + vec_on_pass.x(), passer_point.y() + vec_on_pass.y());
+
+ Vector perpendicular = pass_direction.perpendicular().normalize(1.5);
+ Vector perpendicular_opp = perpendicular.rotate(Angle::fromDegrees(180));


this is just the parallel and the same as pass_direction

itsarune · 2024-07-27T06:47:53Z

src/software/ai/passing/chip_pass.cpp

+ return Polygon({
+ Point(passer_point.x() + perpendicular.x(), passer_point.y() + perpendicular.y()),
+ Point(passer_point.x() + perpendicular_opp.x(), passer_point.y() + perpendicular_opp.y()),
+ Point(point_on_pass.x() + perpendicular.x(), point_on_pass.y() + perpendicular.y()),
+ Point(point_on_pass.x() + perpendicular_opp.x(), point_on_pass.y() + perpendicular_opp.y())


maybe draw an ASCII art diagram for readability

itsarune · 2024-07-27T06:49:19Z

src/software/ai/passing/chip_pass.h

+
+ virtual Duration estimateTimeToPoint(Point& point) const;
+
+ friend std::ostream& operator<<(std::ostream& output_stream, const ChipPass& pass);


why do we need a friend here?

itsarune · 2024-07-27T06:49:35Z

src/software/ai/passing/chip_pass_test.cpp

+// TEST(PassTest2, simple_getters)
+// {
+// ChipPass p(Point(0, 0), Point(2, 0));
+
+// EXPECT_EQ(Point(0, 0), p.passerPoint());
+// EXPECT_EQ(Point(2, 0), p.receiverPoint());
+// std::cout << p.firstBounceRange() << std::endl;
+// }
+
+// TEST(PassTest3, simple_getters)
+// {
+// ChipPass p(Point(0, 0), Point(3, 0));
+
+// EXPECT_EQ(Point(0, 0), p.passerPoint());
+// EXPECT_EQ(Point(3, 0), p.receiverPoint());
+// std::cout << p.firstBounceRange() << std::endl;
+// }


update tests

itsarune · 2024-07-27T06:56:14Z

src/software/ai/passing/cost_function.cpp

+ if (pass.type() == PassType::CHIP_PASS)
+ {
+ std::vector<Robot> enemy_team_not_skipped;
+
+ for (auto &enemy_robot : world.enemyTeam().getAllRobots())
+ {
+ if (!reinterpret_cast<const ChipPass*>(&pass)->isSkipped(enemy_robot.position())) 
+ {
+ enemy_team_not_skipped.push_back(enemy_robot);
+ }
+ }
+ return ratePass(world, pass, zone, Team(enemy_team_not_skipped), passing_config);
+ }
+ else if (pass.type() == PassType::GROUND_PASS)
+ {
+ return ratePass(world, pass, zone, world.enemyTeam(), passing_config);
+ }


maybe better for BasePass to have a function called getIgnoredEnemyRobots(const World& world). I'd be happy to listen to any counter-arguments you have

itsarune · 2024-07-27T06:57:41Z

src/software/ai/passing/pass.h


 /**
 * This class represents a Pass, a receive point with a speed
 */
-class Pass
+class Pass: public BasePass


maybe Pass should be called GroundPass

itsarune · 2024-07-27T07:12:40Z

src/software/ai/passing/cost_function.h

@@ -22,7 +26,7 @@
 * @return A value in [0,1] representing the quality of the pass, with 1 being an
 * ideal pass, and 0 being the worst pass possible
 */
-double ratePass(const World& world, const Pass& pass, const Rectangle& zone,
+double ratePass(const World& world, const BasePass& pass, const Rectangle& zone, const Team& enemy_team,


itsarune · 2024-07-27T07:14:44Z

src/software/ai/passing/pass.h

- * @return The output stream with the string representation of the class appended
- */
- friend std::ostream& operator<<(std::ostream& output_stream, const Pass& pass);
+ virtual Duration estimateTimeToPoint(Point& point) const;


Suggested change

virtual Duration estimateTimeToPoint(Point& point) const;

Duration estimateTimeToPoint(Point& point) const = override;

itsarune · 2024-07-27T07:15:05Z

src/software/ai/passing/pass.h

- * @return true if the Passes are equal and false otherwise
- */
- bool operator==(const Pass& other) const;
+ virtual PassType type() const;


Suggested change

virtual PassType type() const;

PassType type() const = override;

itsarune · 2024-07-27T07:17:00Z

src/software/ai/passing/pass_with_rating.cpp

@@ -3,6 +3,5 @@
 bool operator==(const PassWithRating &lhs, const PassWithRating &rhs)
 {
 return lhs.rating == rhs.rating &&
- lhs.pass.receiverPoint() == rhs.pass.receiverPoint() &&
- lhs.pass.speed() == rhs.pass.speed();
+ lhs.pass.receiverPoint() == rhs.pass.receiverPoint();


why not compare the passes lhs.pass == rhs.pass since we've defined == operators for them already

github-actions · 2024-09-03T02:50:49Z

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

implemented chip modeling

ec25fcc

itsarune reviewed Jun 25, 2024

View reviewed changes

implemented chip pass modeling

e152b94

itsarune reviewed Jul 7, 2024

View reviewed changes

nimazareian added the Robocup-2024 High priority for Rbocup 2024 label Jul 9, 2024

changes

1e10659

itsarune reviewed Jul 27, 2024

View reviewed changes

github-actions bot added the Stale Inactive pull requests label Sep 3, 2024

itsarune removed the Stale Inactive pull requests label Sep 28, 2024

	return std::sqrt(std::pow(receiverPoint().x() - passerPoint().x(), 2) + std::pow(receiverPoint().y() - passerPoint().y(), 2));
	return (receiverPoint() - passerPoint()).length()


		double firstBounceRange();

		virtual Duration estimatePassDuration() const;

	virtual Duration estimatePassDuration() const;
	Duration estimatePassDuration() const = override;

		// if we cover more than 90% of the pass, stop calculating
		while(length_to_go >= pass_length * 0.2)

		@@ -246,12 +250,6 @@ double ratePassFriendlyCapability(const Team& friendly_team, const Pass& pass,
		return 0;
		}

		@@ -2,8 +2,7 @@


		Pass::Pass(Point passer_point, Point receiver_point, double pass_speed_m_per_s)


		virtual Duration estimateTimeToPoint(Point& point) const;

		friend std::ostream& operator<<(std::ostream& output_stream, const ChipPass& pass);

	virtual Duration estimateTimeToPoint(Point& point) const;
	Duration estimateTimeToPoint(Point& point) const = override;

	virtual PassType type() const;
	PassType type() const = override;

Implemented Chip Pass Modeling #3229

Are you sure you want to change the base?

Implemented Chip Pass Modeling #3229

Conversation

sauravbanna commented Jun 20, 2024

Please fill out the following before requesting review on this PR

Description

Testing Done

Resolved Issues

Length Justification and Key Files to Review

Review Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsarune left a comment

Choose a reason for hiding this comment

itsarune commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsarune left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 3, 2024