Marking coordinator by petergarud · Pull Request #2446 · RoboJackets/robocup-software

petergarud · 2025-11-03T00:19:07Z

Allows marking with multiple robots by coordinating who to mark using a danger score calculation

changes

…hine. Still some issues with it actually going to mark, because it seems to idle

rishiso

Few initial comments will look more later

install/setup.zsh

src/rj_strategy/include/rj_strategy/agent/position/defense.hpp

rishiso · 2025-11-03T16:23:19Z

src/rj_strategy/include/rj_strategy/agent/position/free_kicker.hpp

 public:
    FreeKicker(int r_id);
+    FreeKicker(int r_id, std::shared_ptr<ClientHandles> clientHandles);
+    FreeKicker(const Position& other, std::shared_ptr<ClientHandles> clientHandles);


Should we consider making clientHandles part of Position instead of adding to every specific position? If we are doing it this way, would it make sense to only pass the clients it actually needs rather than a struct with all of them?

I tried doing this in the past and remember encountering a lot of weird errors. I think there are a lot of places that end up expecting to have some definition of clienthandles (b/c included in position) but it's hard to pass one in. I tried again just now and still getting a lot of weird errors. I can try to debug but I think it would just take a long time and this approach already works so I would prefer to move on.

Just got a fix for this working, i can either add it to this PR or just make a new one after this gets merged, up to you @petergarud

All that is left for this PR is rebasing based off Waller Coordinator once that is merged in

rishiso · 2025-11-03T16:26:04Z

src/rj_strategy/include/rj_strategy/agent/position/robot_factory_position.hpp

            // Position&)
            current_position_->die();
-            current_position_ = std::make_unique<Pos>(*current_position_);
+            current_position_ = std::make_unique<Pos>(*current_position_, clientHandles_);


If you wanted to do conditional passing of handles based on position, you could use template specializations here, but I personally think client handles should just be added to Position itself

src/rj_strategy/include/rj_strategy/coordinator/marking.hpp

src/rj_strategy/include/rj_strategy/coordinator/marking_client.hpp

src/rj_strategy/src/coordinator/kicker_picker_client.cpp

petergarud · 2025-11-10T02:02:09Z

Pretty much everything should be resolved aside from potentially adding clientHandles to Position which does not feel necessary right now.

automated style fixes Co-authored-by: petergarud <petergarud@users.noreply.github.com>

sanatd33

looks good for now, i have some ideas on how to simplify the client_handles_ thing but i think that can be done after this is merged?

rishiso

some more comments

rishiso · 2025-11-11T03:04:28Z

src/rj_strategy/include/rj_strategy/agent/position/defense.hpp

@@ -45,8 +45,7 @@ class Defense : public Position {

 private:
    // static constexpr int kMaxWallers{6};


Remove this out-of-date comment with kMaxWallers = 6

rishiso · 2025-11-11T03:05:15Z

src/rj_strategy/include/rj_strategy/agent/position/defense.hpp

-    Marker marker_;
+    bool sent_join_marking_group_request_ = false;
+    RJ::Time request_time_;
+    RJ::Seconds kMarkingGroupJoinTimeout{2.0};


Make kMarkingGroupJoinTimeout static constexpr

rishiso · 2025-11-11T03:12:24Z

src/rj_strategy/include/rj_strategy/coordinator/marking.hpp

+
+    std::array<uint8_t, kNumShells> marking_list_{};
+    std::array<double, kNumShells> danger_score_{};
+    std::array<uint8_t, kNumShells> enemey_to_friends_{};


Nit: enemy mispelled

I think we noticed that and left it because it's kind of funny but we can change that if necessary and if it could cause future issues

Just changed it to enemy to prevent future headaches

rishiso · 2025-11-11T03:15:01Z

src/rj_strategy/include/rj_strategy/coordinator/marking.hpp

+    std::array<uint8_t, kNumShells> marking_list_{};
+    std::array<double, kNumShells> danger_score_{};
+    std::array<uint8_t, kNumShells> enemey_to_friends_{};
+    std::vector<uint8_t> queue_;


Nit: Can we use a more descriptive name for the queue (ie. available_markers)?

Changed to unassigned_markers_queue to be more descriptive. Kept the queue in there so people know it's a queue.

rishiso · 2025-11-11T03:17:37Z

src/rj_strategy/include/rj_strategy/coordinator/marking_client.hpp

+    struct Result {
+        bool am_i_member{false};  // Whether this robot is currently a member of the kicker group.
+        std::optional<bool> am_i_marking{false};
+        std::optional<uint8_t> who_i_am_marking{kInvalidRobotId};  // ID of Kicker id


Is there a reason to make these optional variables? Seems like we never set them to nullopt anywhere

Update: Looking at this more closely, can we just remove this struct altogether and pass a single bool?

I think we still need to know if we are marking someone along with if we are member. Removed am_i_marking and just kept who_am_i_marking (changed variable name for consistency). This is an optional now (got rid of the default value) and if it is set then we treat it as they are marking someone, if it is empty they are not marking someone.

rishiso · 2025-11-11T04:10:52Z

src/rj_strategy/src/coordinator/marking.cpp

+    uint8_t most_dangerous = kInvalidRobotId;
+    double min = std::numeric_limits<double>::infinity();
+    for (size_t i = 0; i < danger_score_.size(); ++i) {
+        // don't include marked robots or the guy with the ball in most dangerous calculation


Pretty sure this logic is repeated elsewhere in the file. Probably should make this a helper function. In general, some refactoring might be nice here considering these functions are pretty long

Made helper function to find most dangerous robot

rishiso · 2025-11-11T04:14:13Z

src/rj_strategy/src/coordinator/marking.cpp

+            onOurSide = robot.pose.position().y() > field_center.y();
+        }
+
+        if (onOurSide) {


Use our_half constant instead of this. Shapes have a .contains() method

rishiso · 2025-11-11T04:14:40Z

src/rj_strategy/include/rj_strategy/coordinator/marking.hpp

+    static constexpr int kMaxMarkers = 2;
+    // this is the threshold value for switching
+    // you can play with these constants if the current results aren't good enough
+    static constexpr double kSuperDangerSub = 3.1415926535;


Don't we have a PI constant?

Technically this is just a random number and not pi, but I'll change it to 3.2 so no one else gets confused

rishiso · 2025-11-11T04:15:26Z

src/rj_strategy/src/coordinator/marking.cpp

+
+    // for (size_t i = 0; i < 6; ++i) {
+    //     SPDLOG_INFO("Robot {} has danger score {}", i, danger_score_[i]);
+    // }


I kept that one in because it's really easy to uncomment for debugging purposes but can remove if it is clutter

src/rj_strategy/src/coordinator/marking.cpp

automated style fixes Co-authored-by: sanatd33 <sanatd33@users.noreply.github.com>

…lines 50-58)

Squid5678

Logic makes sense to me, though this PR requires changing a significant number of our classes to incorporate this functionality. While they are minor changes, I do think it's worth documenting somewhere what these changes are and why we made them for future reference.

Squid5678 · 2025-11-03T02:47:29Z

src/rj_strategy/include/rj_strategy/agent/position/defense.hpp

    // static constexpr int kMaxWallers{6};
-    static constexpr int kMaxWallers{
-        static_cast<int>(kNumShells)};  // This effectively turns off marking
+    static constexpr int kMaxWallers{2};


can this be changed into a constant

Squid5678 · 2025-11-03T16:58:59Z

src/rj_strategy/include/rj_strategy/agent/position/defense.hpp

-    Marker marker_;
+    bool sent_join_marking_group_request_ = false;
+    RJ::Time request_time_;
+    RJ::Seconds kMarkingGroupJoinTimeout{2.0};  // 2 seconds


make this a constant as well in case needs to be tuned later

Squid5678 · 2025-11-12T00:27:23Z

src/rj_strategy/include/rj_strategy/agent/position/defense.hpp

 class Defense : public Position {
 public:
    Defense(int r_id);
+    Defense(int r_id, std::shared_ptr<ClientHandles> clientHandles);


ClientHandles seems to play a very large part in the implementation of coordinators going forward. I'm in agreement with Sanat here that we could find a solution in the future, but I am also on the side that if this works, we should not change it and finish off the coordinator design.

It would be nice if we could have a doc created on the purpose of these ClientHandles. Nothing too crazy, just explaining the purpose and where they should be used.

Squid5678 · 2025-11-16T13:39:45Z

src/rj_strategy/src/coordinator/marking.cpp

+        danger_score_[i] = danger_score;
+    }
+
+    // for (size_t i = 0; i < 6; ++i) {


Remove comments; in the future if you want to include something for debugging purposes, just add to clickup description, easier for everyone to access.

Squid5678 · 2025-11-16T13:40:10Z

src/rj_strategy/src/coordinator/marking_client.cpp

+                [this](const rj_msgs::msg::Marking::SharedPtr msg) {  // callback=std::move(callback)
+                    selected_robot_marking_id_ = msg->mark_robot_ids[robot_id_];
+                    am_i_marking_ = (selected_robot_marking_id_ != kInvalidRobotId);
+                    // callback(Result{true, selected_robot_marking_id_});


automated style fixes Co-authored-by: shourikb <shourikb@users.noreply.github.com>

…robocup-software into marking-coordinator

automated style fixes Co-authored-by: petergarud <petergarud@users.noreply.github.com>

sanatd33

nothing blocking but if you need to make changes anyway go fix all the small stuff me and rishi commented on

sanatd33 · 2025-12-03T01:48:02Z

src/rj_strategy/include/rj_strategy/coordinator/marking.hpp

+    static constexpr int kMaxMarkers = 2;
+    // this is the threshold value for switching
+    // you can play with these constants if the current results aren't good enough
+    static constexpr double kSuperDangerSub = 3.2;


maybe some more info on what each param is?

shourikb and others added 30 commits September 28, 2025 20:29

first steps

8b1aa38

added required messages for marking

ef5bc01

more setup

74e7956

fixed some stuff

a5487ae

added a dangerous score array to keep track of valid targets

dfe621e

made some progress

01e4436

danger score calcs

6301a5b

more marking stuff

c85f542

update naming

e83d402

changes

can leave marking

b501fe6

fixed linker error

ded69ac

started client

20b41d0

marking client should be done

787f99b

started on danger score calculation

32f8860

marking coordaintor should be done'

64ef7d4

Struct of coordinators: compiles but fails to run at all

15f1b61

Fix issue with not running

267f9bd

starting implementing coordaintor with defense

30e54f6

added some more stuff to implement marking into the defense state mac…

86e538a

…hine. Still some issues with it actually going to mark, because it seems to idle

some debugging done. marks now, but sim crashes

c740b5e

stuff

68ce933

fixed it so it compiles

245cfd4

Stuff

1d6c90d

rebase to ros2 to have colcon

a6c207a

stuff

b653fd7

New danger score parameters

5477bf5

Merge from ros2

e2b8c4e

Danger angle change

f070f8a

doesn't mark the guy with the ball anymore

2f1354a

completed marking coordinator

ac18cc7

This comment was marked as outdated.

Sign in to view

rishiso reviewed Nov 3, 2025

View reviewed changes

Resolve comments

9207a0c

github-actions bot mentioned this pull request Nov 10, 2025

Fix Code Style On marking-coordinator #2452

Merged

Fix Code Style On marking-coordinator (#2452)

d916b5a

automated style fixes Co-authored-by: petergarud <petergarud@users.noreply.github.com>

sanatd33 approved these changes Nov 10, 2025

View reviewed changes

rishiso self-requested a review November 11, 2025 03:00

rishiso requested changes Nov 11, 2025

View reviewed changes

fix client handles

8de5768

github-actions bot mentioned this pull request Nov 12, 2025

Fix Code Style On marking-coordinator #2454

Merged

github-actions bot and others added 2 commits November 11, 2025 20:06

Fix Code Style On marking-coordinator (#2454)

0ce1e25

automated style fixes Co-authored-by: sanatd33 <sanatd33@users.noreply.github.com>

addressed most comments but still missing some callback stuff (check …

e79fb8e

…lines 50-58)

github-actions bot mentioned this pull request Nov 12, 2025

Fix Code Style On marking-coordinator #2457

Merged

Squid5678 requested changes Nov 16, 2025

View reviewed changes

github-actions bot and others added 6 commits November 16, 2025 08:47

Fix Code Style On marking-coordinator (#2457)

1fc4831

automated style fixes Co-authored-by: shourikb <shourikb@users.noreply.github.com>

fixed some more comments

22d0b1b

Merge branch 'marking-coordinator' of https://github.com/RoboJackets/…

88daaec

…robocup-software into marking-coordinator

More fixes

7b1c468

addressed all comments

498f016

Add client handles implementation

dd5a402

github-actions bot mentioned this pull request Nov 24, 2025

Fix Code Style On marking-coordinator #2472

Merged

Fix Code Style On marking-coordinator (#2472)

5e2cc4d

automated style fixes Co-authored-by: petergarud <petergarud@users.noreply.github.com>

sanatd33 requested review from Squid5678, rishiso and sanatd33 November 24, 2025 02:20

sanatd33 approved these changes Dec 3, 2025

View reviewed changes

last few fixes

7c729e4

github-actions bot mentioned this pull request Feb 4, 2026

Fix Code Style On marking-coordinator #2485

Open

minor bug fix

4d7e53a

		@@ -45,8 +45,7 @@ class Defense : public Position {

		private:
		// static constexpr int kMaxWallers{6};

Conversation

petergarud commented Nov 3, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

rishiso left a comment • 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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

petergarud commented Nov 10, 2025

Uh oh!

sanatd33 left a comment

Choose a reason for hiding this comment

Uh oh!

rishiso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rishiso Nov 11, 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!

shourikb Nov 12, 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!

rishiso Nov 11, 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!

rishiso Nov 11, 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!

shourikb Nov 12, 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!

Uh oh!

Squid5678 left a comment

Choose a reason for hiding this comment

rishiso left a comment •

edited

Loading

rishiso Nov 11, 2025 •

edited

Loading

shourikb Nov 12, 2025 •

edited

Loading

rishiso Nov 11, 2025 •

edited

Loading

rishiso Nov 11, 2025 •

edited

Loading

shourikb Nov 12, 2025 •

edited

Loading