Allow :class:.SurroundingRectangle to accept multiple Mobjects

[Boris-Filin]

Overview: What does this pull request change?

Adds a secondary constructor to SurroundingRectangle and BackgroundRectangle, which takes a list of objects rather than a single one, outlining them all at once.
The constructor is implemented via classmethod decorator: box = SurroundingRectangle.multiple([...], ...)

Motivation and Explanation: Why and how do your changes improve the library?

This PR resolves issue #3863. The changes are mostly cosmetic, improving the readability of the code by removing the necessity to create an additional VGroup.

Further Information and Comments

In principle, this change only moves the construction of VGroup from the user end to the class constructor. As such, it is a minor addition and might not be necessary for the project.

This is a university-driven PR

Just for context, this PR (same as some other recent ones on this repo) is motivated by a university assignment that requires us to contribute to an Open-Source project. PR merge is not required, however. I figured this information might affect your judgment.

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

[JasonGrace2282]

Hello, and first off, thanks for the PR!

My concern with adding another constructor (albeit as a classmethod) is that it introduces two ways of doing the same thing and could confuse users.

SurroundingRectangle(Square())
SurroundingRectangle.multiple([Square()])

As such, I'd be interested to hear more about the benefits of a separate constructor. Otherwise, I'd suggest just making SurroundingRectangle/BackgroundRectangle take *mobjects in their __init__ and turn them into a Group there.

def __init__(self, *mobjects: Mobject, ...) -> None:
    mobject = Group(*mobjects)
    ...

[Boris-Filin]

Thank you for the quick reply!

Otherwise, I'd suggest just making SurroundingRectangle/BackgroundRectangle take *mobjects in their __init__ and turn them into a Group there.

This was my first thought with this implementation, however, it would not be backward compatible for cases such as
SurroundingRectangle(mobject=Square()) or SurroundingRectangle(Square(), RED) because the argument is renamed, and taking variable-length pos arguments *mobjects leads to colors being interpreted as objects and other bad behaviour.

Here are two other solutions I see to this issue. Both eliminate the need for a separate constructor, but have their drawbacks. Would you consider either one acceptable? If not, perhaps sticking to delegating VGroup creation to user is the best way, then #3863 should be closed.

P.S.: Thank you for your time and input on this. This is my first OSs PR, and I really appreciate your feedback.

Solution 1

def __init__(
        self,
        mobject: Mobject = None,
        color: ParsableManimColor = YELLOW,
        ...
        mobjects: Sequence[Mobject] = None,
        **kwargs,
    ) -> None:
        target = mobject
        if mobjects != None:
                target = VGroup(*mobjects)
        if target is None:
                return None
        ...

The idea is to toggle between single/multiple object mode based on the default value of None for either argument.
This would be fully backwards compatible while allowing the user to use the base constructor for multiple objects as such:
SurroundingRectangle(mobjects=(Square(), Circle()), color=RED)

This solution may be less intuitive because technically both mobject and mobjects can be specified, leading to implied precedent.
SurroundingRectangle(mobject=Triangle(), mobjects=(Square(), Circle()), color=RED) # What would happen?

Solution 2

Perhaps typing the original mobject argument as either a single object or a Collection would be more suitable:

def __init__(
        self,
        mobject: Mobject | Sequence[Mobject],
        color: ParsableManimColor = YELLOW,
        ...
        **kwargs,
    ) -> None:
        target = mobject
        if not isinstance(mobject, Mobject):
                target = VGroup(*mobject)
        ...

This solution is slightly more hacky because it relies on object typing, however, it is way more straightforward to use. As a bonus, it does not require named keywords for multiple objects, and the following syntaxis would work for solution 2 but not solution 1:
SurroundingRectangle([Square(), Circle()], RED)

[JasonGrace2282]

Personally, I think it's probably better to break backwards compatibility in exchange for this (arguably) more intuitive syntax. This is especially true because we're not at 1.0.0 yet, so it's better to work towards a nicer API :)

taking variable-length pos arguments *mobjects leads to colors being interpreted as objects and other bad behaviour.

We could always add a line like if not all(isinstance(mob, Mobject) for mob in mobjects): raise TypeError(...).

P.S. Welcome to OSS!

[JasonGrace2282]

Almost there! I left a few (admittedly pretty nitpicky) comments. Other than that, I think it's good to go! Thanks!

[JasonGrace2282]

Sorry for the late reply, but it looks good to me now! Thanks for implementing this :)