Remove all instances of MessageToDict #3405

AmyKawa · 2024-11-09T20:12:52Z

Please fill out the following before requesting review on this PR

Description

Removed MessageToDict import, as well as all instances of MessageToDict
Replaced variables accessing a dictionary into accessing values from .GetOptions()

Testing Done

n/a

Resolved Issues

#3341

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

…into 1

itsarune · 2024-11-11T10:37:43Z

src/software/thunderscope/gl/layers/gl_tactic_layer.py

        """
-        tactic_assignments = play_info_dict["robotTacticAssignment"]
+        tactic_assignments = play_info.robotTacticAssignment()


this doesn't quite work. The correct way access the robot tactic assignment field is: play_info.robot_tactic_assignment().

This will return a MessageMapContainer which has this API: https://googleapis.dev/python/protobuf/3.17.0/google/protobuf/internal/containers.html#google.protobuf.internal.containers.MessageMap

The function call will match the field from the definition of the proto parameters:

Software/src/proto/play_info_msg.proto

Line 18 in 10bded3

map<uint32, Tactic> robot_tactic_assignment = 1;

itsarune · 2024-11-11T10:39:43Z

src/software/thunderscope/gl/layers/gl_tactic_layer.py

@@ -38,20 +36,17 @@ def __init__(self, name: str, buffer_size: int = 5) -> None:
    def refresh_graphics(self) -> None:
        """Update graphics in this layer"""
        self.cached_world = self.world_buffer.get(block=False)
-        play_info = self.play_info_buffer.get(block=False)
-        play_info_dict = MessageToDict(play_info)
+        play_info = self.play_info_buffer.get(block=False).GetOptions()


you don't need to call GetOptions() here. I believe you'll only need this if you define specific bounds for the proto definitions (like if you defined this proto:

Software/src/proto/parameters.proto

Line 61 in 10bded3

required double ball_is_kicked_m_per_s_threshold = 1

and you needed to access the "bounds" values). We don't use this pattern in most of other protos, including this one.

itsarune · 2024-11-11T10:40:03Z

src/software/thunderscope/play/playinfo_widget.py

-        playinfo = self.playinfo_buffer.get(block=False, return_cached=False)
+        playinfo = self.playinfo_buffer.get(
+            block=False, return_cached=False
+        ).GetOptions()


you don't need to call GetOptions here

itsarune · 2024-11-11T10:42:25Z

src/software/thunderscope/play/playinfo_widget.py

        robot_ids = []
        tactic_fsm_states = []
        tactic_names = []
        play_name = []

-        if "robotTacticAssignment" not in play_info_dict:
+        if "robotTacticAssignment" not in playinfo:


I believe you can do if not playinfo.robot_tactic_assignment()

Maybe try if not playinfo.robot_tactic_assignment if that doesn't work. I may be wrong, but I feel like robot_tactic_assignment would return a type of dict which is not callable?

itsarune · 2024-11-11T10:44:14Z

src/software/thunderscope/play/playinfo_widget.py

+            len(playinfo.robotTacticAssignment()),
+            len(playinfo.play.playState()),
        )

        # setting table size dynamically
        self.play_table.setRowCount(num_rows)

-        for state in play_info_dict["play"]["playState"]:
+        for state in playinfo.play().playstate():
            play_name.append(state)

-        for robot_id in sorted(play_info_dict["robotTacticAssignment"]):
+        for robot_id in sorted(playinfo.robotTacticAssignment()):
            robot_ids.append(robot_id)
            tactic_fsm_states.append(
-                play_info_dict["robotTacticAssignment"][robot_id]["tacticFsmState"]
+                playinfo.robotTacticAssignment().robot_id().tacticFsmState()
            )
            tactic_names.append(
-                play_info_dict["robotTacticAssignment"][robot_id]["tacticName"]
+                playinfo.robotTacticAssignment().robot_id().tacticName()


these function calls should be updated. The field names should match the proto field names.

You should take a look at the proto definition:

Software/src/proto/play_info_msg.proto

Line 5 in 10bded3

message PlayInfo

And take a look at the Python generated code reference guide: https://protobuf.dev/reference/python/python-generated/

for your reference, it should be something like this:

for robot_id in sorted(playinfo.robot_tactic_assignment): robot_ids.append(robot_id) tactic_fsm_states.append( playinfo.robot_tactic_assignment[robot_id].tactic_fsm_state ) tactic_names.append( playinfo.robot_tactic_assignment[robot_id].tactic_name )

playinfo has type is a google protobuf type. playinfo.robot_tactic_assignment returns a dictionary. So to access this we need an id like robot_id. playinfo.robot_tactic_assignment[robot_id] is this type:

Software/src/proto/play_info_msg.proto

Lines 12 to 16 in 10bded3

message Tactic

{

string tactic_name = 1;

string tactic_fsm_state = 2;

}

To get the tactic_name we do playinfo.robot_tactic_assignment[robot_id].tactic_name

itsarune · 2024-11-11T10:44:38Z

src/software/thunderscope/play/refereeinfo_widget.py

@@ -36,15 +35,13 @@ def __init__(self, buffer_size: int = 1) -> None:

    def refresh(self) -> None:
        """Update the referee info widget with new referee information"""
-        referee = self.referee_buffer.get(block=False, return_cached=False)
+        referee = self.referee_buffer.get(block=False, return_cached=False).GetOptions()


you don't need to call GetOptions here

itsarune · 2024-11-11T10:46:06Z

src/software/thunderscope/play/refereeinfo_widget.py

-
-        if not referee_msg_dict:
+        if not referee:
            return


this check is duplicated above, you can delete this now

itsarune · 2024-11-11T10:47:02Z

src/software/thunderscope/play/refereeinfo_widget.py

-        referee_msg_dict = MessageToDict(referee)
-
-        if not referee_msg_dict:
+        if not referee:
            return

        stage_time_left_s = (


you need to update:
referee_msg_dict["stageTimeLeft"]
referee_msg_dict["packetTimestamp"]

itsarune · 2024-11-11T10:48:26Z

src/software/thunderscope/play/refereeinfo_widget.py

-                blue.append(referee_msg_dict["blue"][info])
-                yellow.append(referee_msg_dict["yellow"][info])
+                blue.append(referee.blue().info())
+                yellow.append(referee.yellow().info())

        set_table_data(
            {


you also need to update the implementation of parse_yellow_card_times I believe for the team_info object

itsarune

Good start. I recommend running ./tbots.py run thunderscope_main --enable_autoref and check the console to see if errors are being spammed. If everything is working properly, you should see the "Play Info" and "Referee Info" widget constantly update.

Mr-Anyone · 2024-11-12T03:24:48Z

src/software/thunderscope/play/playinfo_widget.py

            return

        num_rows = max(
-            len(play_info_dict["robotTacticAssignment"]),
-            len(play_info_dict["play"]["playState"]),
+            len(playinfo.robotTacticAssignment()),


I think you meant len(playinfo.robot_tactic_assignment)

Mr-Anyone · 2024-11-12T03:27:27Z

src/software/thunderscope/play/playinfo_widget.py

-            len(play_info_dict["robotTacticAssignment"]),
-            len(play_info_dict["play"]["playState"]),
+            len(playinfo.robotTacticAssignment()),
+            len(playinfo.play.playState()),


I think this should be len(playinfo.play.play_state), you should check these two comments.

Mr-Anyone · 2024-11-12T03:52:40Z

src/software/thunderscope/play/refereeinfo_widget.py

@@ -54,27 +51,27 @@ def refresh(self) -> None:
            f"Packet Timestamp: {round(float(referee_msg_dict['packetTimestamp']) * SECONDS_PER_MICROSECOND, 3)}\n"
            + f"Stage Time Left: {int(stage_time_left_s / SECONDS_PER_MINUTE):02d}"
            + f":{int(stage_time_left_s % SECONDS_PER_MINUTE):02d}\n"
-            + f"Stage: {referee_msg_dict['stage']}\n"
+            + f"Stage: {referee.stage()}\n"


I don't think you need a (), so just referee.stage

Mr-Anyone · 2024-11-12T03:52:57Z

src/software/thunderscope/play/refereeinfo_widget.py

            + "Command: "
-            + referee_msg_dict["command"]
+            + referee.command()


Mr-Anyone · 2024-11-12T03:53:06Z

src/software/thunderscope/play/refereeinfo_widget.py

            + "\n"
-            + f"Blue Team on Positive Half: {referee_msg_dict['blueTeamOnPositiveHalf']}\n"
+            + f"Blue Team on Positive Half: {referee.blueTeamOnPositiveHalf()}\n"


here as well

Mr-Anyone · 2024-11-12T03:53:36Z

src/software/thunderscope/play/refereeinfo_widget.py

            if info == "yellowCardTimes":
-                blue.append(self.parse_yellow_card_times(referee_msg_dict["blue"]))
-                yellow.append(self.parse_yellow_card_times(referee_msg_dict["yellow"]))
+                blue.append(self.parse_yellow_card_times(referee.blue()))
+                yellow.append(self.parse_yellow_card_times(referee.yellow()))
            elif info == "remainingTimeouts":
-                blue.append(referee_msg_dict["blue"]["timeouts"])
-                yellow.append(referee_msg_dict["yellow"]["timeouts"])
+                blue.append(referee.blue().timeouts())
+                yellow.append(referee.yellow().timeouts())
            elif info == "goalkeeperID":
-                blue.append(referee_msg_dict["blue"]["goalkeeper"])
-                yellow.append(referee_msg_dict["yellow"]["goalkeeper"])
+                blue.append(referee.blue().goalkeepers())
+                yellow.append(referee.yellow().goalkeeper())
            else:
-                blue.append(referee_msg_dict["blue"][info])
-                yellow.append(referee_msg_dict["yellow"][info])
+                blue.append(referee.blue().info())
+                yellow.append(referee.yellow().info())


you actually don't need the () here.

Mr-Anyone · 2024-11-12T04:00:00Z

src/software/thunderscope/common/proto_parameter_tree_util.py

 from thefuzz import fuzz
+from proto.import_all_protos import *


 def __create_int_parameter_writable(key, value, descriptor):


it would be nice to add type annotation

def __create_int_parameter_writable(key, value, descriptor): to def __create_int_parameter_writable(key:str, value:int, descriptor:FieldDescriptor):

Also, FieldDescriptor could be found here: from google.protobuf.descriptor import FieldDescriptor

Mr-Anyone · 2024-11-12T04:07:45Z

looking good. just need some final touches.

Mr-Anyone · 2024-11-12T04:43:17Z

FYI:

Say we have the following protobuf file person.proto:

message Person{
  required string first_name = 1;
  required string last_name = 2;
}

We use a protobuf compiler, called protoc, to generate the python file. This would then be turned into some python file under some black magic process.

To set the field, we do something like this;

person = Person()
person.first_name =  "Someone" 
person.last_name = "Someone's Lastname"

To access the field, we do something like this:

print(f"their first name  is {person.first_name}")
print(f"their first name  is {person.last_name}")

Since from the .proto above, we know that .first_name is a string and .last_name is a string. We cannot do something like the following:

person.first_name()

This would imply that person.first_name is callable, which it is not.

see here for the entire code:

https://github.com/Mr-Anyone/Thunderbot_Software/tree/proto_example/src/proto_example

You might have to do:

git remote add vincent [email protected]:Mr-Anyone/Thunderbot_Software.git 
git switch -c proto_example vincent/proto_example
./tbots.py run main

AmyKawa added 2 commits November 9, 2024 12:01

Removed all instances of messagetodict

7df478c

Merge branch 'master' of https://github.com/UBC-Thunderbots/Software …

99e7f6a

…into 1

AmyKawa requested review from itsarune, williamckha and sauravbanna as code owners November 9, 2024 20:12

AmyKawa changed the title 1 Remove all instances of MessageToDict Nov 9, 2024

[pre-commit.ci lite] apply automatic fixes

8aa2dd0

itsarune reviewed Nov 11, 2024

View reviewed changes

Mr-Anyone reviewed Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all instances of MessageToDict #3405

Remove all instances of MessageToDict #3405

AmyKawa commented Nov 9, 2024 •

edited

Loading

itsarune Nov 11, 2024

itsarune Nov 11, 2024

itsarune Nov 11, 2024

itsarune Nov 11, 2024

Mr-Anyone Nov 12, 2024

itsarune Nov 11, 2024

Mr-Anyone Nov 12, 2024

itsarune Nov 11, 2024

itsarune Nov 11, 2024

itsarune Nov 11, 2024

itsarune Nov 11, 2024

itsarune left a comment

Mr-Anyone Nov 12, 2024

Mr-Anyone Nov 12, 2024

Mr-Anyone Nov 12, 2024

Mr-Anyone Nov 12, 2024

Mr-Anyone Nov 12, 2024

Mr-Anyone Nov 12, 2024

Mr-Anyone Nov 12, 2024

Mr-Anyone commented Nov 12, 2024

Mr-Anyone commented Nov 12, 2024

	message Tactic
	{
	string tactic_name = 1;
	string tactic_fsm_state = 2;
	}

Remove all instances of MessageToDict #3405

Are you sure you want to change the base?

Remove all instances of MessageToDict #3405

Conversation

AmyKawa commented Nov 9, 2024 • edited Loading

Please fill out the following before requesting review on this PR

Description

Testing Done

Resolved Issues

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

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

Mr-Anyone commented Nov 12, 2024

Mr-Anyone commented Nov 12, 2024

AmyKawa commented Nov 9, 2024 •

edited

Loading