-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add referee display layer #3337
base: master
Are you sure you want to change the base?
Changes from 8 commits
4d608ca
de95603
07f17cb
b369777
8f58631
878112a
160d347
ef0a030
f39a399
0d242ce
db3f208
709db29
ecd314d
b7642cf
4f3d93c
947428d
70ea39f
63ff6ed
494b833
f403762
e08edd0
8b1a616
2daae4f
207eae2
4f94227
7368f8c
7f097b0
afaf9a9
6b4fb32
1f91332
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
from PyQt6.QtGui import QFont, QColor | ||
from pyqtgraph.opengl.GLGraphicsItem import GLGraphicsItem | ||
from pyqtgraph.Qt import QtCore, QtGui | ||
|
||
from typing import Optional | ||
|
||
from software.thunderscope.gl.graphics.gl_painter import GLPainter | ||
from software.thunderscope.constants import Colors | ||
|
||
|
||
class GLLabel(GLPainter): | ||
"""Displays a 2D text label on the viewport""" | ||
|
||
def __init__( | ||
self, | ||
parent_item: Optional[GLGraphicsItem] = None, | ||
font: QFont = QFont("Roboto", 8), | ||
text_color: QColor = Colors.PRIMARY_TEXT_COLOR, | ||
offset: tuple[int, int] = (0, 0), | ||
text: str = "", | ||
) -> None: | ||
"""Initialize the GLLabel | ||
|
||
:param parent_item: The parent item of the graphic | ||
:param font: The font using to render the text | ||
: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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a huge deal tho There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
the viewport right edge. | ||
If y is negative then the y offset is |y| pixels from | ||
the viewport bottom edge. | ||
:param text: The optional title to display above the legend | ||
""" | ||
super().__init__(parent_item=parent_item) | ||
|
||
self.text_pen = QtGui.QPen(text_color) | ||
self.font = font | ||
self.offset = offset | ||
self.text = text | ||
|
||
self.add_draw_function(self.draw_label) | ||
|
||
def draw_label(self, painter: QtGui.QPainter, viewport_rect: QtCore.QRect) -> None: | ||
"""Draw the label | ||
|
||
:param painter: The QPainter to perform drawing operations with | ||
:param viewport_rect: The QRect indicating the viewport dimensions | ||
""" | ||
# calculate width and height of the label | ||
painter.setFont(self.font) | ||
bounds = painter.boundingRect( | ||
QtCore.QRectF(0, 0, 0, 0), | ||
QtCore.Qt.AlignmentFlag.AlignLeft | QtCore.Qt.AlignmentFlag.AlignVCenter, | ||
str(self.text), | ||
) | ||
|
||
width = round(bounds.width()) | ||
height = round(bounds.height()) | ||
|
||
# Determine x and y coordinates of the label | ||
if self.offset[0] < 0: | ||
x = viewport_rect.right() + self.offset[0] - width | ||
else: | ||
x = viewport_rect.left() + self.offset[0] | ||
if self.offset[1] < 0: | ||
y = viewport_rect.bottom() + self.offset[1] - height | ||
else: | ||
y = viewport_rect.top() + self.offset[1] | ||
|
||
if self.text: | ||
painter.drawText(QtCore.QPoint(x, y), self.text) | ||
|
||
def set_text(self, new_text: str) -> None: | ||
"""Update the text being displayed | ||
:param new_text: new text being displayed | ||
""" | ||
self.text = new_text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly enough, it seems that our
constants.h
file is wildly inconsistent with its use ofstatic const
vsconstexpr
. From my knowledge, there is no real performance difference here aside fromconstexpr
potentially preventing others in the future from assigning a non-compile time constant value to these variables.TLDR: ask the others about as I am not sure why this is the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the info I have learned just then,
constexpr
might be used in this case, but I will confirm with others.static const
seems to be used more in class-level constants. If we useconstexpr
, it allows complie-time optimization.If confirmed and refactor needed, do you mind if I do this refactor in a separate ticket, so we don't mess up other features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, sounds good.
IMO
constexpr
andstatic const
in this case should not technically have a difference in optimization in this case as most of thestatic const
values we have can be determined at compile-time which results in the optimization process being carried through anyways (to my understanding). This would mean the main advantage of switching would purely be to deter others in the future from using non-compile time derivable values (asconstexpr
would throw an error). You may want to refer to the link below for a clearer explanation:https://stackoverflow.com/questions/41125651/constexpr-vs-static-const-which-one-to-prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could open a new issue to make all these constants
constexpr
. The main benefit would be so that we can use these constants in otherconstexpr
variables and use them in template args, array declarations, etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, fixed all 3 constants included in this PR with
constexpr
. Will send another PR on refactoring others after this is merged.