-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix assertions and improve error messages when adding submobjects #3756
Conversation
for more information, see https://pre-commit.ci
…050/manim into improve-submobject-errors
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.
Changes look good to me!
Think you missed one or two places though
https://github.com/chopan050/manim/blob/0046b8496d1736ecf65386a719ba7a6971eb5ac3/manim/mobject/opengl/opengl_vectorized_mobject.py#L1691-L1692
Thanks for highlighting those places! I fixed them, as well as some other places I missed. |
Closes #3755.
Initially this PR only improved 2 things:
When adding into a
Mobject
a value which is not aMobject
, or there's an attempt to add a mobject to itself, the error messages are now more descriptive, indicating:Example:
Mobject._assert_valid_submobjects()
method which makes this check, and calling it on every method which required this assertion, which areMobject.add()
,Mobject.insert()
andMobject.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 theadd()
method to be more restrictive (by checking that the submobjects are all instances ofVMobject
), theVMobject
class itself didn't do that, which causes the issue #3755: you could add aMobject
to aVMobject
such asCircle
, and aMobject
could be part of the family of aVGroup
without being a direct child of it.Plus, the other methods
insert()
andadd_to_back()
weren't overridden, so one could still do this:So, this PR also fixes that.
VMobject
, not onlyVGroup
s, now asserts onadd()
,insert()
andadd_to_back()
that all the submobjects are instances ofVMobject
. To do that, it suffices to override the_assert_valid_submobjects()
method inVMobject
. It is not necessary to actually touch theadd()
,insert()
andadd_to_back()
methods. Because theVGroup.add()
docstrings have an useful example, I decided to leave that override in the code, even if it now contains only areturn super().add(*vmobjects)
line.Finally:
OpenGLMobject
andOpenGLVMobject
to avoid breaking tests, and for more consistency.Reviewer Checklist