Skip to content

Bug Fix Closed Loop #3522

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.

Sign up for GitHub

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

Open
Mr-Anyone wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Bug Fix Closed Loop #3522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a02fab8
Bug Fix Closed Loop
Mr-Anyone Aug 25, 2025
200b606
Address comments
Mr-Anyone Nov 15, 2025
d820548
[pre-commit.ci lite] apply automatic fixes
pre-commit-ci-lite[bot] Nov 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/software/thunderscope/gl/graphics/gl_polygon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ def __init__(
outline_color: QtGui.QColor = Colors.DEFAULT_GRAPHICS_COLOR,
fill_color: Optional[QtGui.QColor] = None,
line_width: float = LINE_WIDTH,
closed: bool = False,
) -> None:
"""Initialize the GLPolygon

Expand All @@ -30,6 +31,8 @@ def __init__(
:param outline_color: The color of the polygon's outline
:param fill_color: The color used to fill the polygon, or None if no fill
:param line_width: The line width of the polygon's outline
:param closed: If true, connect the first and the last point to form a
closed shape. Otherwise, the shape is left open.
"""
super().__init__(
parent_item=parent_item,
Expand All @@ -38,6 +41,7 @@ def __init__(
line_width=line_width,
)

self.is_closed = closed
Copy link
Contributor

@StarrryNight StarrryNight Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit confused by why the initialization of is_closed. Have we used the constructor parameter "closed" at all, and is there a reason why we set is_closed after calling set_points?

Copy link
Contributor Author

@Mr-Anyone Mr-Anyone Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think you just discovered a bug that is not caught!

I will move self.is_closed one line above self.set_points(points).

My guess is that set_points is called after the construction of this class so this bug is never discovered.

self.set_points(points)

def set_points(self, points: list[tuple[float, float]]) -> None:
Expand All @@ -62,7 +66,10 @@ def _update_shape_data(self) -> None:
if not self.points:
return
Comment on lines 66 to 67
Copy link
Contributor

@Andrewyx Andrewyx Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts on logging something here as an error/warning? I think this error could easily be buried otherwise if we just silently return early.


vertices = [(point[0], point[1], 0) for point in self.points + [self.points[0]]]
render_points = (
self.points + [self.points[0]] if self.is_closed else self.points
)
vertices = [(point[0], point[1], 0) for point in render_points]
self.setData(pos=vertices)

Copy link
Contributor

@StarrryNight StarrryNight Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should vertices contain the duplicate self.points[0] when the polygon is closed? I thought vertices means independent vertices so it should copy from points instead of render_points.

Copy link
Contributor Author

@Mr-Anyone Mr-Anyone Nov 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should vertices contain the duplicate self.points[0] when the polygon is closed?

I believe the answer is yes.

The vertices for the following must be [A, B, D, C, A]

A -- B 
|     |
C -- D

If the vertices are [A, B, D, C], it will render the following:

A -- B 
      |
C -- D

Copy link
Contributor Author

@Mr-Anyone Mr-Anyone Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I have done a bit more digging, you did bring up a fairly interesting point.

The inheritance diagram works like the following:

GLPolygon -> GLShape ->GLLinePlotItem

The fundamental problem here is that we probably want a GL_LINE_LOOP instead of a GL_LINE_STRIP:

https://github.com/pyqtgraph/pyqtgraph/blob/8385bd7e1dd88a007d5f50438afe388fe164c047/pyqtgraph/opengl/items/GLLinePlotItem.py#L90-L93

Image

I am not sure if there is a easy fix without sending a patch to pyqtgraph.

CC: @williamckha

Copy link
Contributor

@Andrewyx Andrewyx Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you are getting me thinking about this class, I feel like there should be a clearer definition for what is a "polygon" especially since we can form an "open looped" shape. The notion of an "open" polygon feels a bit strange and leaves some ambiguity about how this class may handle lists of points with <3 points in them (since geometrically, that cannot form a polygon.

I think we should either be more explicit with the fact that this class can support any number of points greater than 0, and guard against cases where a closed loop is used with fewer than 3 points (since this can have unintended rendering consequences for the caller).

Otherwise, the more software-y approach here would be to abstract the class into some GL Line class (unless this is what GLLinePlotItem is, but I haven't looked into that class yet). Hence, we can clearly define polygons as ALWAYS closed loops and have their rendering inherit from GL Line.

Copy link
Member

@nycrat nycrat Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I agree that this should be a different class that better describes a series of line segments, cause my first thought of rendering for example the path of a robot would definitely not be a polygon

if self.fill_graphic:
Expand Down
4 changes: 2 additions & 2 deletions src/software/thunderscope/gl/layers/gl_draw_polygon_obstacle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def __init__(self, name: str, friendly_io: ProtoUnixIO) -> None:
self.rendering_polygons = ObservableList(self._graphics_changed)

# The current polygon being edited (not visible yet)
self.current_polygon = GLPolygon(parent_item=self, line_width=2)
self.current_polygon = GLPolygon(parent_item=self, line_width=2, closed=True)

self.can_double_click = False

Expand Down Expand Up @@ -75,7 +75,7 @@ def __push_polygon_to_list(self):
self.points.clear()

self.rendering_polygons.append(self.current_polygon)
self.current_polygon = GLPolygon(parent_item=self, line_width=2)
self.current_polygon = GLPolygon(parent_item=self, line_width=2, closed=True)

def __add_one_point(self, point: Point) -> None:
"""Adding one points to a polygon
Expand Down