manim icon indicating copy to clipboard operation
manim copied to clipboard

Fix graph updater to respect vertex boundaries

Open noamzaks opened this issue 3 years ago • 4 comments

Overview: What does this pull request change?

The graph's updater now uses the underlying _set_start_and_end_attrs method of Line-based mobjects so that it respects the boundaries of the start/end mobjects.

Before:

https://user-images.githubusercontent.com/63877260/175780272-67605e57-7d61-47ae-a06a-af90fd61d56f.mp4

After:

https://user-images.githubusercontent.com/63877260/175780277-537a99d5-ba48-40c7-ba3c-09ed2139e443.mp4

For the snippet

class Testing(Scene):
    def construct(self):
        g = Graph([0, 1], [(0, 1)], edge_type=Arrow, labels=True)
        self.add(g)
        self.wait()
        self.play(g.animate.shift(DOWN))

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

While temporarily calling clear_updaters() helps, in any cases where there are moving vertices the animation must currently still not be continuous because while the constructor respects the start/end mobjects' boundaries, the updater doesn't. This fix makes everything 'smooth' and continuous (and also, respect the mobjects' boundaries which is more visually pleasing).

Further Information and Comments

Closes #2843.

  • Unfortunately, this breaks edge cases where the edge_type doesn't have a _set_start_and_end_attrs method (which is possible only if it doesn't inherit from Line)
  • Therefore, it might make sense to abstract the _set_start_and_end_attrs into another place, and perhaps even rename it.

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

noamzaks avatar Jun 25 '22 15:06 noamzaks

I'm guessing the file tests/test_graphical_units/control_data/threed/MovingVertices.npz needs to be replaced accordingly, but how does one create the appropriate .npz?

noamzaks avatar Jun 25 '22 15:06 noamzaks

[...] but how does one create the appropriate .npz?

Please refer to the contribution guide here in the AddingTests section.

First check if the test really needs to be replaced.

MrDiver avatar Jun 25 '22 16:06 MrDiver

[...] but how does one create the appropriate .npz?

Please refer to the contribution guide here in the AddingTests section.

First check if the test really needs to be replaced.

Thank you! I believe the test makes sure the frames of the animation in the ThreeD scene are as expected. Thus, if we change the behavior of the updater (which changes the edges' position) the resulting frames are different and should be different

noamzaks avatar Jun 25 '22 18:06 noamzaks

It seems this change, accounting for the vertices' radiuses, looks worse in 3D scenes. Perhaps there is an underlying issue with get_boundary_point.

Here's the 3D scene from the test, with the new change:

https://user-images.githubusercontent.com/63877260/177015982-ea5a3e14-6ffa-4cf8-8b48-64767d9c4671.mp4

Here's it without doing so (the current state):

https://user-images.githubusercontent.com/63877260/177015992-1005fb65-5e3e-4a6f-b60c-5dfc8bcc494f.mp4

In any case, my so-called "solution" is not elegant at all; Looking for thoughts about how to improve on it (perhaps detect a 3D scene automatically).

noamzaks avatar Jul 02 '22 20:07 noamzaks