Add referee display layer #3337
Conversation
Lmh-java
requested review from
itsarune,
williamckha and
sauravbanna
as code owners
Lmh-java
requested review from
a team,
Mr-Anyone and
AadityaSuri
and removed request for
a team
There was a problem hiding this comment.
🔥
Would you also be able to incorporate #3289 into this PR? There's already another PR that never got merged which implements it.
| if not referee_proto: | ||
| return | ||
|
|
||
| referee_msg_dict = MessageToDict(referee_proto) |
There was a problem hiding this comment.
You actually don't need MessageToDict (I'm not sure why we use it in some of our code). You can just access the fields directly e.g. referee_proto.stage
There was a problem hiding this comment.
After testing, I think in this case I might want to keep MessageToDict, since directly acessing referee_proto.stage only gives me the index number (instead of the text)
Yes, sure. 👍 |
|
Just a little bit clarification here before I get started, is ball placement visulization supposed to be in the same layer with referee info? In #3289, that's what being said, but in the associated PR, it seems to be added to a different layer. |
I would put the ball placement visualization in the Referee Info layer you created |
| :param text_color: The color for rendering the text. | ||
| :param offset: The offset (x, y) from the viewport left and top edge | ||
| to use when positioning the label. | ||
| If x is negative then the x offset is |x| pixels from |
There was a problem hiding this comment.
nit: is this necessary? feels like its a bit clearer to just require the offsets to be positive numbers always. from the perspective of someone using this method in the future, the sign flipping feels a bit roundabout
There was a problem hiding this comment.
Lol I followed the design of other components on the same level for consistency. But sure, I will clean this up to make it more understandable.
There was a problem hiding this comment.
After some testing here, I found out that the easier way to place the label on the top right corner is just to keep this wired sign flipping, because it takes some code to fetch the width of the viewport before passing into this function. Instead, we can get viewport by whatever calls this function, so we probably need to use a negative sign to denote starting from the right side.
| return | ||
| self.cached_referee_info = referee_msg_dict | ||
|
|
||
| if not self.gamestate_type_text: |
There was a problem hiding this comment.
nit: these 2 if statements are pretty identical, could probably pull them out to a new method
There was a problem hiding this comment.
These two if statements are writting to class fields, I can parametrize relevant parameters, but I don't know how to parametrize class level field so everytime we call this method we write to a different file. It seems taking a lot of code...
There was a problem hiding this comment.
you can probably parametrize a class-level member with a lambda but that might be overkill since there's only 2
|
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. |
|
@itsarune @williamckha Merged with master, manual tested and everything LGTM. Please review |
|
Thanks William! 🍻 |
Description
Add a referee info layer to display game state and command.
Testing steps:
Testing Done
Resolved Issues
resolves #3333
resolves #3289
Demo
Dropdown menu to enable layer
Game state and command display
Ball placement visual (ball is not placed correctly)
Ball placement visual (ball is placed correctly)
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue