Added ConvexHull and LabeledPolygram

[JayGupta797]

Overview: What does this pull request change?

This PR introduces four additional features and two utilities.

1. manim.mobject.geometry.polygram.ConvexHull which constructs a ConvexHull through a set of points.
2. manim.mobject.three\_d.polyhedra.ConvexHull3D which constructs a ConvexHull through a set of points.
3. manim.mobject.geometry.labeled.Label which constructs a boxed label.
4. 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.
5. manim.utilities.qhull which constructs a ConvexHull through a set of points. It extends to arbitrary dimensions.
6. 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:

1. manim/docs/build/html/reference/manim.mobject.geometry.labeled.LabeledPolygram.html
2. manim/docs/build/html/reference/manim.mobject.geometry.labeled.Label.html
3. manim/docs/build/html/reference/manim.mobject.geometry.polygram.ConvexHull.html
4. manim/docs/build/html/reference/manim.mobject.three_d.polyhedra.ConvexHull3D.html

Further Information and Comments

Reviewer Checklist

• The PR title is descriptive enough for the changelog, and the PR is labeled correctly
• If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
• If applicable: newly added functions and classes are tested

[chopan050]

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:

Suggested change

	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

[JayGupta797]

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!

[chopan050]

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.

Suggested change

	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

[JayGupta797]

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).

[chopan050]

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).

[JayGupta797]

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.

[chopan050]

There are two issues with the parameters:

• internal's default value is None, but based on the current code, it can never be None: 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, and Facet calculates its normal attribute independently via internal.

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.

[JayGupta797]

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.

[chopan050]

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.