Bug Fix Closed Loop #3522
Bug Fix Closed Loop #3522
Conversation
Mr-Anyone
requested review from
Andrewyx,
sauravbanna and
williamckha
as code owners
| :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: whether the GLPolygon is closed or not |
There was a problem hiding this comment.
I think this naming or doc can be improved a bit to be more specific as to the fact that by "closed" we mean include the final segment from the last point to the initial.
There was a problem hiding this comment.
Not sure if this is good enough:
:param closed: If true, connect the first and the last point to form a
closed shape. Otherwise, the shape is left open.
| 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) | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
I am not sure if there is a easy fix without sending a patch to pyqtgraph.
CC: @williamckha
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| ) | ||
|
|
||
| self.set_points(points) | ||
| self.is_closed = closed |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Mr-Anyone
force-pushed
the
closed_loop_bug_fix
branch
from
815d520 to
200b606
Compare
What I see on my end. You can see closed and open shapes. |
| if not self.points: | ||
| return |
There was a problem hiding this comment.
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.
|
I'm not sure I understand what exactly an 'open' shape is. I don't see how it exists in the screenshot sent. Do we even need to support something like this? |
|
EDIT: @GrayHoang I'm pretty sure its solving this issue where paths for the robots are rendered as a closed shape. 
|
|
I left another comment above, but I think this should be a separate class instead of implementing "open shapes" for the GLPolygon class |
5 participants
I'll work on a description later.
Description
Testing Done
Resolved Issues
Length Justification and Key Files to Review
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