-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Added ConvexHull and LabeledPolygram #3933
base: main
Are you sure you want to change the base?
Conversation
fixed added utils (i.e., Incomplete ordering and Explicit returns mixed with implicit), added :quality: high to docstrings, made ConvexHullExample determined
manim/utils/qhull.py
Outdated
def _recursive_horizon(self, eye, facet, horizon): | ||
# If the eye is visible from the facet... | ||
if np.dot(facet.normal, eye - facet.center) > 0: | ||
# Label the facet as visible and cross each edge | ||
horizon.facets.add(facet) | ||
for subfacet in facet.subfacets: | ||
neighbor = (self.neighbors[subfacet] - {facet}).pop() | ||
# If the neighbor is not visible, then the edge shared must be on the boundary | ||
if neighbor not in horizon.facets and not self._recursive_horizon( | ||
eye, neighbor, horizon | ||
): | ||
horizon.boundary.append(subfacet) | ||
return 1 | ||
else: | ||
return 0 |
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.
It seems CodeQL didn't like the if ... return 1 else return 0
, it didn't realize that all the possible return cases were already covered and there were no implicit (fall through) returns.
One option would be to remove that else, so that instead of
if condition:
# large chunk of code
return 1
else:
return 0
it was straight up
if condition:
# large chunk of code
return 1
return 0
However it doesn't look very nice to have that small dangling return 0
after that large chunk of code. It would be better to do something like this, which handles the small case and returns from it immediately if necessary, and removes one indentation level from the large chunk of code:
if not condition:
return 0
# large chunk of code
return 1
Moreover, those returned ints (1 and 0) are, according to the code, meant to be used as True
and False
, so it's better to stick to that instead:
if not condition:
return False
# large chunk of code
return True
Thus, my suggestion is the following:
def _recursive_horizon(self, eye, facet, horizon): | |
# If the eye is visible from the facet... | |
if np.dot(facet.normal, eye - facet.center) > 0: | |
# Label the facet as visible and cross each edge | |
horizon.facets.add(facet) | |
for subfacet in facet.subfacets: | |
neighbor = (self.neighbors[subfacet] - {facet}).pop() | |
# If the neighbor is not visible, then the edge shared must be on the boundary | |
if neighbor not in horizon.facets and not self._recursive_horizon( | |
eye, neighbor, horizon | |
): | |
horizon.boundary.append(subfacet) | |
return 1 | |
else: | |
return 0 | |
def _recursive_horizon( | |
self, eye: Point3D, facet: Facet, horizon: Horizon | |
) -> bool: | |
eye_is_visible_from_facet = np.dot(facet.normal, eye - facet.center) > 0 | |
if not eye_is_visible_from_facet: | |
return False | |
# If the eye is visible, label the facet as visible and cross each edge | |
horizon.facets.add(facet) | |
for subfacet in facet.subfacets: | |
neighbor = (self.neighbors[subfacet] - {facet}).pop() | |
# If the neighbor is not visible, then the edge shared must be on the boundary | |
if neighbor not in horizon.facets and not self._recursive_horizon( | |
eye, neighbor, horizon | |
): | |
horizon.boundary.append(subfacet) | |
return True |
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.
Ah, that makes sense! I was confused why CodeQL didn't like the original form even though all possible return cases were addressed, so I added the else statement hoping that would make it more explicit. I like your version better!
manim/utils/qhull.py
Outdated
class QuickHull: | ||
def __init__(self, tolerance=1e-5): | ||
self.facets = [] | ||
self.removed = set() | ||
self.outside = {} | ||
self.neighbors = {} | ||
self.unclaimed = None | ||
self.internal = None | ||
self.tolerance = tolerance |
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.
It would be nice if you added docstrings and typings for parameters, returns and attributes (where necessary), both in this file and polylabel.py
.
It allows for people to hover on an attribute or method (in Visual Studio Code, for example) and get hints on what the type of the attribute is supposed to be, what the parameters of a method are expected to be, and what you should expect to get as a result when calling a method.
It makes dev debugging much easier, and it allows our type checkers to detect errors.
I suggest that you take a look at: https://docs.manim.community/en/stable/contributing/docs.html
Although I could add the typings in a subsequent PR, it would be best if you did it, because you know the code better, whereas I would have to guess the types and probably make mistakes.
For example, it could be something like this:
Note 1: Point3D
and Point3D_Array
are custom type aliases which we defined on our manim.typing
module. You need to do from manim.typing import Point3D, Point3D_Array
.
Note 2: I guessed the data types, so maybe I'm wrong on some of these.
class QuickHull: | |
def __init__(self, tolerance=1e-5): | |
self.facets = [] | |
self.removed = set() | |
self.outside = {} | |
self.neighbors = {} | |
self.unclaimed = None | |
self.internal = None | |
self.tolerance = tolerance | |
class QuickHull: | |
"""Description of a QuickHull. | |
Parameters | |
---------- | |
tolerance | |
Description of what the tolerance is. | |
Attributes | |
---------- | |
facets | |
List of facets. | |
removed | |
A set of removed facets. | |
etc... | |
""" | |
def __init__(self, tolerance: float = 1e-5) -> None: | |
self.facets: list[Facet] = [] | |
self.removed: set[Facet] = set() | |
self.outside: dict[Facet, tuple[Point3D_Array | None, Point3D | None]] = {} | |
self.neighbors: dict[SubFacet, Facet] = {} | |
self.unclaimed: Point3D_Array | None = None | |
self.internal: Point3D | None = None | |
self.tolerance = tolerance |
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, I can add docstrings and types to this! Actually, the original code was already strongly typed, so it should be a matter of adding those back with some minor adjustments. Unfortunately, I can't use Point3D
because the algorithm extends to arbitrary dimensions (thought in practice it is very slow beyond 7).
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.
I see. In Manim, Mobjects only use 3D points, so Point3D
could be fine for this anyways... but we can always add PointND
and PointND_Array
to manim.typing
if necessary (we currently do have VectorND
as a type alias, but that's for directions rather than positions).
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.
I see! The only other issue I can think of at the moment is handling the 2 and 3 dimensional cases with Point3D
since the former would take the form of [x y 0]
leading to undesirable behavior. I think the simplest solution is to use np.ndarray
(or a thin wrapper like PointND
) for typing.
manim/utils/qhull.py
Outdated
|
||
|
||
class Facet: | ||
def __init__(self, coordinates, normal=None, internal=None): |
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.
There are two issues with the parameters:
internal
's default value isNone
, but based on the current code, it can never beNone
: it is used in
self.normal = self.compute_normal(internal)
which does the following:
if np.dot(normal, self.center - internal) < 0:
which will crash if internal
is None
, with a ValueError
because you can't subtract None
from a numpy.ndarray
.
I'd suggest not adding a default value to internal
and forcing every initialization of Facet
to supply an explicit value for it... but the name itself (internal
) suggests to me that it might be a special, optional value (an "internal value"?), which is connected to the next point:
normal
is currently not being used anywhere, andFacet
calculates itsnormal
attribute independently viainternal
.
If this is intended, let's remove the normal
parameter.
But it seems to me that the intention could be to let an user add their own normal
, and if they don't supply it, a normal
would be calculated for them. Something like this:
if normal is None:
normal = self.compute_normal(internal)
self.normal = normal
in which case I can understand that internal
's default value might be None
, if a user supplies normal
instead. In that case I'd suggest:
if normal is None:
if internal is None:
raise ValueError("You must supply a non-None value for either 'normal' or 'internal'.")
normal = self.compute_normal(internal)
self.normal = normal
but it all depends on your intention with the code and the actual meaning of internal
(is it an "internal value", or is it an internal something-else?). I'm currently just making wild guesses.
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.
Thank you so much for this (detailed) catch! This is an artifact from a previous version of the code where I computed the initial simplex's normals in batch and supplied them to Facet
directly whereas every other use required an internal point to define the orientation. Since I don't expect users to interact with Facet
in meaningful (do let me know if this is a reasonable expectation) ways, I'll likely drop the normal
parameter as you suggest.
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.
Thanks for your changes!
It is weird that the ReadTheDocs pipeline failed, even when you (and me) successfully ran make html
locally. It sometimes fails randomly. Maybe if you push another commit it will run successfully.
I've taken a look at some of the code and left some suggestions. It's mainly about typing in polylabel.py
and qhull.py
.
7393423
to
3d2ee4a
Compare
3d2ee4a
to
03cc46c
Compare
Overview: What does this pull request change?
This PR introduces four additional features and two utilities.
manim.mobject.geometry.polygram.ConvexHull
which constructs a ConvexHull through a set of points.manim.mobject.three\_d.polyhedra.ConvexHull3D
which constructs a ConvexHull through a set of points.manim.mobject.geometry.labeled.Label
which constructs a boxed label.manim.mobject.geometry.labeled.LabeledPolygram
which constructs a polygram containing a label box at its pole of inaccessibility. This is the point inside a polygon that is farthest from its edges.manim.utilities.qhull
which constructs a ConvexHull through a set of points. It extends to arbitrary dimensions.manim.utilities.polylabel
which computes the pole of inaccessibility for any complex polygon to a given precision.Notably, none of these introduce breaking changes nor require additional dependencies.
Motivation and Explanation: Why and how do your changes improve the library?
The first two are useful in scenarios where the user has a collection of points but does not know what order they should be plotted. In 2D this reduces to sorting by angle bit in 3D the task is more complex and can be handled automagically. The fourth is useful for labeling polygons.
Links to added or changed documentation pages
I am not sure how to format this, but here is what the doc pages look like locally:
Further Information and Comments
Reviewer Checklist