manim icon indicating copy to clipboard operation
manim copied to clipboard

Allow 3DArrows to utilize GrowArrow()

Open RishabhSaini opened this issue 2 years ago • 7 comments

Overview: What does this pull request change?

Adds a scale function to the Arrow3D Class in three_dimensions to prevent it from falling back to the default implemented in mobject which lacks implementation for handling the scaling of the conical tip of the arrow. Similar to the 2D version, it handles scaling the arrow.

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

Resolves: #3481 Allows self.play(GrowArrow(Arrow3D(LEFT, RIGHT)))

Links to added or changed documentation pages

Further Information and Comments

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

RishabhSaini avatar Dec 03 '23 04:12 RishabhSaini

I forgot to mention a final thing:

Make sure to document Arrow3D.scale() (and potentially Arrow3D._set_cylinder_diameter_from_length() or whatever you call it) when you're done! It is always important to document new features.

P.S: have you experienced extremely long render times when using Arrow3D? It shouldn't be the case at all, that could be another issue to publish and/or even a PR :eyes:

chopan050 avatar Dec 04 '23 02:12 chopan050

Thanks for the comment. I am making the above requested changes. Upon completion I will mark this Draft PR to ready

RishabhSaini avatar Dec 05 '23 18:12 RishabhSaini

@chopan050 How does this implementation look now? I have addressed the scaling of tip as well as modifying the GrowingArrow function to take care of 3D vs 2D arrows

RishabhSaini avatar Dec 06 '23 02:12 RishabhSaini

I tried rendering the following scene, but the tip doesn't move properly:

class Simple(ThreeDScene):
    def construct(self):
        arrow = Arrow3D(resolution=(1, 12))
        self.add(arrow)
        self.wait()
        self.play(arrow.animate.scale(3), run_time=5)
        self.wait()
        arrow.scale(1/3)
        self.wait()
        arrow.scale(2)
        self.wait()
        arrow.scale(1/2)
        self.wait()

https://github.com/ManimCommunity/manim/assets/49853152/95f28c6d-3d7e-4efd-8d85-c420806f0f5a

  • The length of the arrow scales fine.
  • I don't think that the actual width of the arrow line should change, for the reasons mentioned before (unless some specific conditions apply, the same conditions as for Arrow).
  • The tip of the arrow is not scaling, which by itself is sort of fine, but it's not moving correctly. Also, the width of the line should be consistent with this behavior of not scaling.

chopan050 avatar Dec 08 '23 22:12 chopan050

  • I don't think that the actual width of the arrow line should change, for the reasons mentioned before (unless some specific conditions apply, the same conditions as for Arrow).

Scaling a 2D arrow without scaling_tips also results in a behavior where the width of the arrow increases and the tips also increase

https://github.com/ManimCommunity/manim/assets/10206278/498e1003-fd43-47b1-a925-4ecd4b7eab49

RishabhSaini avatar Dec 09 '23 16:12 RishabhSaini

Hence it looks like this issue stems from the 2D arrow not scaling properly when the scale_tips is set to true. Hence this issue blocks on the correct implementation for the add_tip (rotation fails when the tips are not scaled) on the 2D arrow.

RishabhSaini avatar Dec 09 '23 18:12 RishabhSaini

Hello, sorry for not commenting for that long.

It's true that the tip and width do scale with GrowArrow or scale_tips=True, as you show in the animation.

However, I was talking about the case where you use scale() with the default parameters. Try running

class Test(Scene):
    def construct(self):
        self.play(Arrow().animate.scale(3), run_time=3)

and see what happens, and compare with the previous video I sent with Arrow3D. https://github.com/ManimCommunity/manim/assets/49853152/062cd9e7-4513-4da2-8dda-e7d39734f628

chopan050 avatar Dec 15 '23 20:12 chopan050

Closed due to inactivity

JasonGrace2282 avatar Jul 13 '24 18:07 JasonGrace2282