Skip to content
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

Fix assertions and improve error messages when adding submobjects #3756

Merged
merged 22 commits into from
May 21, 2024

Conversation

chopan050
Copy link
Contributor

@chopan050 chopan050 commented May 12, 2024

Closes #3755.

Initially this PR only improved 2 things:

  1. When adding into a Mobject a value which is not a Mobject, or there's an attempt to add a mobject to itself, the error messages are now more descriptive, indicating:

    • what's the value being added
    • what's its type
    • at which index is the issue happening
    • into which Mobject you're attempting to add the value

    Example:

>>> img = ImageMobject("cat.jpg")
>>> img.add(Circle(), 3)
# TypeError: Only values of type Mobject can be added as submobjects of ImageMobject, but the value 3 (at index 1) is of type int.
>>> img.add(img)
# ValueError: Cannot add ImageMobject as a submobject of itself (at index 0).
  1. Refactor the code by creating a Mobject._assert_valid_submobjects() method which makes this check, and calling it on every method which required this assertion, which are Mobject.add(), Mobject.insert() and Mobject.add_to_back(). Previously, those methods duplicated the code for checking the submobjects, but in different ways, with very different error messages, and occasionally not always checking the types of the submobjects.

But I noticed that, while VGroup overrided the add() method to be more restrictive (by checking that the submobjects are all instances of VMobject), the VMobject class itself didn't do that, which causes the issue #3755: you could add a Mobject to a VMobject such as Circle, and a Mobject could be part of the family of a VGroup without being a direct child of it.

Plus, the other methods insert() and add_to_back() weren't overridden, so one could still do this:

>>> VGroup().add_to_back(Mobject())
VGroup(Mobject)

So, this PR also fixes that.

  1. Any VMobject, not only VGroups, now asserts on add(), insert() and add_to_back() that all the submobjects are instances of VMobject. To do that, it suffices to override the _assert_valid_submobjects() method in VMobject. It is not necessary to actually touch the add(), insert() and add_to_back() methods. Because the VGroup.add() docstrings have an useful example, I decided to leave that override in the code, even if it now contains only a return super().add(*vmobjects) line.
>>> Circle().add(Mobject())
# TypeError: Only values of type VMobject can be added as submobjects of Circle, but the value Mobject (at index 0) is of type Mobject. You can try adding this value into a Group instead.

Finally:

  1. Repeat all the previous steps for OpenGLMobject and OpenGLVMobject to avoid breaking tests, and for more consistency.
  2. Update the tests to take into account the more descriptive error messages.

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 chopan050 added pr:bugfix Bug fix for use in PRs solving a specific issue:bug refactor Refactor or redesign of existing code pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements and removed pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! labels May 12, 2024
Copy link
Contributor

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

tests/module/mobject/mobject/test_mobject.py Outdated Show resolved Hide resolved
@chopan050
Copy link
Contributor Author

Thanks for highlighting those places! I fixed them, as well as some other places I missed.

@chopan050 chopan050 merged commit 0d67dcd into ManimCommunity:main May 21, 2024
18 checks passed
@chopan050 chopan050 deleted the improve-submobject-errors branch May 23, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements pr:bugfix Bug fix for use in PRs solving a specific issue:bug refactor Refactor or redesign of existing code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

VGroup cannot have a Mobject as a child, but VMobject can
2 participants