Move mobject positioning methods into separate class - #4994
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
GniLudio
added
documentation
|
The docs include the new class now: Positionable.html |
|
I spotted a regression that the library doesn't currently test for: On the main branch, But in this PR, But more generally, I'm now concerned that this PR breaks other behavior elsewhere without anyone knowing because of insufficient regression tests. I would strongly suggest that before this PR is merged, proper tests should be created for all the methods touched by this PR (and probably some others which depend on them). |
I think the known kwargs should definitely be delcared specificly. For unknown kwargs it's probably a good idea to have a |
I think there are many valid choices here. One way to start might be to look at which methods shouldn't receive a |
|
Question: upon closer review of the new class, I've got some concerns that I'd love to hear your thoughts on. Some of the major changes introduced by this PR are:
Overall, I think this is a good change - everything is much more generalized now and it's clear that many of the methods are variations on the same operation. However, IMO this increase in abstraction leads to some method semantics that don't make that much sense. A few examples:
I don't think any of these are huge blockers, but it's always easier to add more functionality than to remove it later once people have started using it, so I think it's at least worth mentioning. I have suggestions for all of these, but before I do that, I just wanted to hear your thoughts on these things. Did you have any particular design goal or considerations in mind when making these changes? I'm entirely willing to be wrong on this :) |
That was already the case with When passing another object to the value of a setter method, it always uses the property of that object as the value. So Alternatively, having separate
I think the base assumption should be that the shape of an object isn't modified unless specified. Otherwise, even
I renamed the
I know that |
|
Additionally, the old method names (e.g. |
This I don't agree with -
I think that's a wonderful rule in theory, but in my view it clashes too heavily with the existing conventions for getters/setters in Python and other languages. Using
Disagree for the same reason as my first point! "Move" is basically always analogous to "shift" or "translate".
Generally agree! See below for my naming ideas.
100000% agree, I think "critical point" is a horrible term. But I would also challenge the notion of a "position" here; given that a mobject currently doesn't have a single origin point that its points are defined relative to, I find it difficult to say for sure that a mobject has a single position. You could maybe make the argument that its position is the center of its bounding box?
Definitely open to this (though I think "edge" is also tricky language when more than half of the critical points aren't really edges, haha)! In the suggestions below I float the idea of the term "anchor", maybe that could work? SuggestionsIf I was king of Manim, here's what I would do:
I look forward to hearing your thoughts on all this! It might also be relevant to hear the opinion of some additional core developers since this is a reasonably big design choice. |
I meant that
Agree.
I just wanted to make the point that it isn't hard to understand that other operations like
I wouldn't be against it. The same argument could be used for removing the getter methods as well: They can all be replicated using
Your assumption is correct. I didn't like having multiple setter methods for the same attribute. That just leads to more methods (and I would say there are already quite a lot to remember). I would favor dropping them, but that would be quite a breaking change.
I can understand that, but just as a note: Currently, the |
|
Sounds like we're roughly aligned! I've only got a few comments.
I actually think it's completely fine to keep
Here I'm assuming you're referring to the various
Yeah, I totally see how it could for example be useful to express any point inside the bounding box of a mobject with a tuple point = interpolate(minimum_corner, maximum_corner, (x, y, z))or similar. This would effectively be some Positionable-specific version of |
|
Added the discussion points to the PR description. |
|
I just realized that returning references to Here an example: def construct(self) -> None:
circle = Circle()
all_points = circle.get_all_points()
all_points += 3 * RIGHT
self.add(circle)All operations should thus return refresh copies. Also it could make sense to make constants read-only to avoid unexpected behavior like this. class TestScene(Scene):
def construct(self):
point = ORIGIN
point += RIGHT + UP
circle = Circle().move_to(ORIGIN)
self.add(circle) |
|
I'm fine with always returning copies, but I don't necessarily think it's wrong for Manim to not do so. Sometimes a live view of the returned points is even what you want! This example: def construct(self) -> None:
circle = Circle()
all_points = circle.get_all_points()
all_points += 3 * RIGHT
self.add(circle)actually seems to be a perfectly legitimate use case for the user (who is after all perfectly capable of calling In many cases, it's also possibly an unnecessary performance hit. Take for example def get_center_of_mass(self) -> Point3D:
return np.apply_along_axis(np.mean, 0, self.get_all_points())We're just iterating over I'm completely on board with your new change of returning |
You are right. It's quite a performance difference for mobject with only 1 member which has points. And I noticed that the old behavior also returned a reference in that case. So I reverted it for |
There are two variants of what Making the variable
|
|
I'm not particularly in love with either option; in my mind, the risk (to the extent there is any) is not actual overwriting of the constant, but rather unintentional in-place modification a la your example in the previous comment. I suppose the For what it's worth, when the user writes |
|
I renamed |
There was a problem hiding this comment.
General notes on testing (I realize much of this is currently left out on purpose, so see it more as a wishlist!):
- I think a helper method to build a Positionable would be very useful. Something simple like:
def positionable_with_points(points): return Positionable().set_points(points)
- For any method which takes a
directionorabout_edgeor similar as parameter (meaning a vector representing an anchor point), the test should be parametrized to try at least all the constant-valued directions such asUP,DR,OUT,ORIGIN, etc. Might be useful to save these as adirection_constantslist at the top of the file. It could also be built aslist(itertools.product([-1, 0, 1], repeat=3)).- A similar thing goes for methods with a
dimparameter; we should take care to test all three when it makes sense.
- A similar thing goes for methods with a
- Methods with optional parameters should be tested with and without arguments for those parameters when it makes sense.
- Some methods have mutually exclusive parameters. For example,
apply_points_functionwill choose its pivot point in this order:This means that a) tests should take care to actually test all those branches and b) there should probably be tests to confirm that it is possible to pass mutually exclusive arguments but that behavior stays as expected.if about_point is not None: choose about_point if about_edge is not None: choose self.get_critical_point(about_edge) choose self.get_critical_point(ORIGIN) - Most of the methods which modify a Positionable could use a test for the case where the operation does nothing (shifting by 0, calling
apply_functionwithlambda x: x, etc.) Similarly, we could probably test for operations where we pass in empty lists of points and so on. - Standard boundary-case stuff. Can we pass a
buff=0? Does a negativebuffbehave the way we expect?
I'll add more if I think of them :)
There was a problem hiding this comment.
- Getter methods should always be tested with 0 points.
3 participants
Overview: What does this pull request change?
Moves the
Mobjectpositioning methods into their own class.Motivation and Explanation: Why and how do your changes improve the library?
Changes
coor_maskparameter frommove_toandnext_to(breaking)submobject_to_alignandindex_of_submobject_to_alignfromnext_to(breaking)**kwargsto all settersDiscussion Points
set_x(other)match_x(other)(current choice)set_x(other.get_x())get_anchor,get_critical_point,get_positionget_anchor**kwargs?get_all_pointsorget_points_defining_boundary?get_all_pointswidth,heightanddepthproperties for the(set|get)_(width|height|depth)methodsStructure
Here is the full structure
Benchmark
I used randomized testing with 10-100k points (without submobjects) to benchmark and validate the changes.
Here are the benchmark results
To run the tests yourself, switch to the main branch and execute the following commands.
Future
Here are a few things that aren't addressed with this PR, but should definitely be discussed and/or changed in the future.
apply_function_to_submobject_positions,space_out_submobjects,arrange,arrange_in_gridPositionablefor opengl@deprecated_paramsonly supports renaming keyword-only parametersLinks to added or changed documentation pages
Positionable.html, Mobject.html
Reviewer Checklist