Fixed Arrow3D.put_start_and_end_on() to use the actual end of the arrow

[SirJamesClarkMaxwell]

Overview: What does this pull request change?

I have fixed the issue #3655
the problem was that in the put_start_and_end_on method we got the not true end of the arrow, but the end of the cylinder. What I did, was I added the invisible point attached to the tip of the cone, and in the get_end method I returned the coordinates of this point -> the length of the vector was wrong the saling factor was alwasy larger than 1.

In the new implementation, I also changed a little bit of the Cone class: the base is always added but with a different opacity, 0 or 1.

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

Links to added or changed documentation pages

#3655

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

[nathanielatom]

I noticed a potential drawback with simply setting the opacity for Cone.base_circle and Cone.tip: if a user later sets the opacity on a composite object like an Arrow3D then both the tip Dot3D and base circle become visible.

Is there any way to hide mobjects without adjusting their opacity? I'm becoming more familiar with manim but not yet super familiar with internals. As a workaround, I'm just manually resetting arrow.cone.tip.set_fill(opacity=0) every time I change the overall arrow opacity.

Also, on a related topic, any ideas why the base circle of the purple arrow in behind shows up as drawn overtop of the pink arrow in the foreground despite the pink arrow being closer to the camera?

[nathanielatom]

Could a ValueTracker somehow be used instead for Cone.tip?

[SirJamesClarkMaxwell]

Could you send me the code? I will try to figured it out. On discord i have the same nick-name

[nathanielatom]

Here's a somewhat minimal and reasonably fast snippet for jupyter notebook cells:

from manim import *
import numpy as np

def shrink(arrow, *, small=1e-2, animate=False):
    hawkeye = arrow.animate if animate else arrow
    if not getattr(arrow, 'shrunk', False):
        arrow.shrunk = True
        vect = normalize(arrow.get_end() - arrow.get_start())
        return hawkeye.put_start_and_end_on(arrow.get_start(), vect * small + arrow.get_start())
    else:
        return hawkeye.set_opacity(0)

%%manim -ql -v WARNING TestArrow

class TestArrow(ThreeDScene):

    def construct(self):
        arrow = Arrow3D(ORIGIN, np.array([1, 1, 0]))
        self.add(arrow)

        for _ in range(3):
            self.play(shrink(arrow, animate=True), rate_func=linear, run_time=0.5)

        arrow.set_opacity(1)
        # arrow.cone.tip.set_opacity(0)
        arrow.shrunk = False
        self.play(
            arrow.animate.put_start_and_end_on(ORIGIN, np.array([-1, 1, 0])),
            rate_func=linear,
            run_time=0.5)

Maybe a Point3D could also be used instead of a Dot3D?

[nathanielatom]

Some of the code is also trying to sort out another Arrow3D quirk: arrows can never have zero length or it raises an error about trying to move the endpoints of a closed loop.

What would you think about updating Arrow3D's (or maybe better to do Cylinder or Line3D's) put_start_and_end_on method in this PR? Maybe something similar to this, which first shrinks the arrow to a tiny but non-zero length (keeping the previous direction), and then sets it's opacity to zero (to prevent the arrow from shrinking exponentially and leading to numerical issues on subsequent calls).

def put_start_and_end_on(arrow, start, end, *, animate=False, small=1e-2):
    hawkeye = arrow.animate if animate else arrow
    if np.array_equal(start, end):
        if not getattr(arrow, 'shrunk', False):
            arrow.shrunk = True
            vect = unit_vector(arrow.get_end() - arrow.get_start())
            return hawkeye.put_start_and_end_on(start, vect * small + start)
        else:
            return hawkeye.set_opacity(0)
    else:
        arrow.shrunk = False
        arrow.set_opacity(1)
        arrow.cone.tip.set_opacity(0)
        arrow.cone.base_circle.set_opacity(0)
        return hawkeye.put_start_and_end_on(start, end)

Sorry I hardly ever go on discord.

[SirJamesClarkMaxwell]

I will check this out after Monday, tommorow I will not have enough time. Thanks for the code 🙂

[lambdadotjoburg]

Cool -Awesome Change - Makes the System more ROBUST - the scaling factors for Manim is a bit tricky

Now that somebody has taken care of the 3D Objects for the General MANIM CE, the Axes-System in ManimLib seems more improved than the before.

[SirJamesClarkMaxwell]

I noticed a potential drawback with simply setting the opacity for Cone.base_circle and Cone.tip: if a user later sets the opacity on a composite object like an Arrow3D then both the tip Dot3D and base circle become visible.

Is there any way to hide mobjects without adjusting their opacity? I'm becoming more familiar with manim but not yet super familiar with internals. As a workaround, I'm just manually resetting `arrow.cone.tip.set_fill(opacity=0)` every time I change the overall arrow opacity.

Also, on a related topic, any ideas why the base circle of the purple arrow in behind shows up as drawn overtop of the pink arrow in the foreground despite the pink arrow being closer to the camera?

[SirJamesClarkMaxwell]

Okay, now I have a little time to think about this, but I just started the project on quantum computing...
Back to the main topic: a couple of things:

1. If we want to hide an object on the scene, we shouldn't add it to the scene. But this is the thing: if we don't add this object (in this case, base_circle and tip), manim, after some manipulations, will return the old tip, and the code will not work correctly.
2. You proposed Point3D instead of Dot3D, but Point3D is just a fancy name for np.array with three elements. It is not a Mobject and can't be added to Group or VGroup.
3. We could create a new group of elements inside the Cone, but we will end up with not the same issue as I described in the first point. Those objects/coordinates will not be updated after some operations.
4. We could override all methods for moving, rotating, setting color, filling, etc. -> This code will no longer be good, clean, and easily extensible.
5. I see one; this is a terrible solution in terms of performance; I could add an updator for the tip, it will always be visible if a user deletes this updator. Why is this solution not a good idea? Because it will increase the number of computations ...
6. For some reason, your code is not working; it worked but has stopped. I have no idea why. I copied it several times...
7. I can't figure out why you get the purple arrow without the code

Could a ValueTracker somehow be used instead for Cone. Tip?

8. Could you elaborate on this? How would you like to put the ValueTracker here?
9. The issue with a 0 length of the arrow is known. I (or another brave man/woman) have to fix it after this PR. But I don't know
   when I find the time, maybe after death...

I assume that our discussion about the changes and this PR, has to stay here.

[SirJamesClarkMaxwell]

1. If we want to hide an object on the scene, we shouldn't add it to the scene. But this is the thing: if we don't add this object (in this case, base_circle and tip), manim, after some manipulations, will return the old tip, and the code will not work correctly.

To addition: maybe I have found a solution, but eee i am not sure. What if we in get_end() method we could create new cone based on the visible one, add Dot3D to VGroup, move new cone into the old one and then return the position of the dot? It should work...

[nathanielatom]

Thanks for taking a look!

1-2. I see. Perhaps simply a smaller Dot3D? Or maybe we could override the set_opacity method for the cone.tip instance to not do anything: cone.tip.set_opacity = lambda *args, **kwargs: None. That way it would stay hidden.
3-5. Yeah a lot of trade-offs for each of these; I don't have a clear quantification of what the performance hit would be, but certainly in 3D manim's cairo backend already struggles in terms of performance. Having a general purpose way of keeping attributes of mobjects might be useful - I also found that I needed to manually update the Arrow3D.direction attribute during animation.
6. Oh what was the error/issue? The code still works reliably for me, creating a fresh notebook or a script (removing the %%manim magic line). The code runs with the version of manim from this PR.
7. Sorry that isn't the code that produces the purple arrow: that takes hours to render, so I created a minimal example that shows the same issue with the cone tip. For reference the full code is available here: https://github.com/nathanielatom/asl-bloch-sim/blob/main/asl_bloch_sim/backends/manim_cairo.py
8. I might have been misunderstanding the purpose of a value tracker, I was basically thinking of it as a mobject whose position would be updated from transformations but that wouldn't actually display anything.
9. Fair enough, I found the workaround in my put_start_and_end_on function above to be sufficient: it just avoids setting the length to zero, in a way that still shows naturally for animations.

All that said, I think the opacity of the arrow tip is still a fairly minor issue unlikely to affect many users, and this PR brings significant value to anyone using Arrow3D so I don't think any of this discussion should block merging!!

[SirJamesClarkMaxwell]

 I also found that I needed to manually update
the Arrow3D.direction attribute during animation.

Why? I mean, what is the unexpected behavior?

[SirJamesClarkMaxwell]

1. We should not override the working methods. If something is not working after our changes this is our fault....
   I know that, especially in Python, there is no clean code, but what I know is that if we overwrite the set_opacity method for Cone or Arrow3D, our changes will not be merged. I would not merge these changes even if I could lol.
2. My error is as follows

  File "d:\Programing\Manim\ManimContributing\manim\ThreeDArrowIssue.py", line 45, in <module>
    TestArrow().render()
  File "d:\Programing\Manim\ManimContributing\manim\manim\scene\scene.py", line 229, in render
    self.construct()
  File "d:\Programing\Manim\ManimContributing\manim\ThreeDArrowIssue.py", line 24, in construct
    self.play(shrink(arrow, animate=True), rate_func=linear, run_time=0.5)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "d:\Programing\Manim\ManimContributing\manim\ThreeDArrowIssue.py", line 10, in shrink
    return hawkeye.put_start_and_end_on(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "d:\Programing\Manim\ManimContributing\manim\manim\mobject\mobject.py", line 3040, in update_target
    method(*method_args, **method_kwargs)
  File "d:\Programing\Manim\ManimContributing\manim\manim\mobject\mobject.py", line 1739, in put_start_and_end_on  
    raise Exception("Cannot position endpoints of closed loop")
Exception: Cannot position endpoints of closed loop

8. Yeah, this is the purpose of the value tracker, but using valueTracker requires to use of updaters, and this is what want to avoid, because of performance

[SirJamesClarkMaxwell]

This is my proposition, for cone class
self.height = height <- in constructor

       def get_end(self) -> np.ndarray:
        direction = self.direction / np.linalg.norm(self.direction)
        cone_base = self.base_circle.get_center()
        return cone_base + direction * self.height

[chopan050]

LGTM!

[behackl]

Can the tripled reference to start and end cause any harm here? (Modifying any of the entries will modify all three of them in the same way.)

[SirJamesClarkMaxwell]

I think that it must be the same point three times. Otherwise we will get a 3d curve wich would be visible, and we don't want this. My solution based on merge #3358 or #3718. I am not sure why we might have problem here, because we are calculating everything internally based on user start and end points. Maybe I missunderstood you question

[JasonGrace2282]

I believe he is talking about this behavior

x = [1]
y = [x]*3
x.append(2)
print(y)

Which results in

[[1, 2], [1, 2], [1, 2]]

Because y simply stores a list of 3 "pointers" to x.

[behackl]

Exactly, this is what I was referring to!

[SirJamesClarkMaxwell]

Okay, but should we care about it? They are local variables, as long as direction, get_center and new_height will return propper types we will not have a problem here. I could add type checking but I am not sure that we need it here. Why? Because if direction won't have a proper type cone and cilinder will through the error same for height, if get_center will have invalid type all manim will be broken. I could add it but IMO if the type of the variables used in this method other part of the code will through the error ealier.

[behackl]

I don't want to cause any worries or block this for no reason, I just find the construction here a bit odd. Can you explain why these mobjects here are paths that visit the same point three times? I'm just curious about why [start]*3 is necessary.

[SirJamesClarkMaxwell]

I'm sorry for not getting back to you sooner. I checked we could use VectorizedPoint, and it will work. I just thought we need at least three points to construct any manim object. I can't run the tests locally. There are some problems with the .toml file, but everything works correctly when I run the standard manim script.

[chopan050]

Hello!
I'd also want this to get merged soon.

Regarding the initial concern, VMobject.set_points_as_corners() creates a new NumPy array from scratch using the given points, so that VMobject.points no longer contains 3 references to the same point:

>>> vmob = VMobject().set_points_as_corners([UP] * 3)
>>> vmob.points
array([[0., 1., 0.],
       [0., 1., 0.],
       [0., 1., 0.],
       [0., 1., 0.],
       [0., 1., 0.],
       [0., 1., 0.],
       [0., 1., 0.],
       [0., 1., 0.]])
>>> vmob.points[0][2] = 5
>>> vmob.points
array([[0., 1., 5.],
       [0., 1., 0.],
       [0., 1., 0.],
       [0., 1., 0.],
       [0., 1., 0.],
       [0., 1., 0.],
       [0., 1., 0.],
       [0., 1., 0.]])

So it's OK, you don't have to worry about that :)

[chopan050]

So, will the implementation be changed to a VectorizedPoint or similar, or will it stay the same? 👀

[SirJamesClarkMaxwell]

I will push today the VectorizedPoint version.