Skip to content

Conversation

@behackl
Copy link
Member

@behackl behackl commented Aug 28, 2020

List of Changes

  • Refactor ArrowTip in geometry.py to be a (actually quite concrete) abstract base class so that different arrow tip shapes can be implemented by inheriting from it.
  • Add ArrowTriangleTip (the current default tip), and two new tips ArrowCircleTip, ArrowSquareTip.
  • Add keyword tip_shape to pass the custom shape to the Arrow constructor.

Motivation

This is part of #272.

Explanation for Changes

This PR changes the class hierarchy for arrow tips a little in order to allow the (easy) implementation of custom tips. Details are given in the List of Changes.

Testing Status

As an example, rendering the ArrowTest scene in the Snippet

import numpy as np

from manim import Scene, Arrow, ShowCreation, FadeOut,\
                  ArrowTriangleTip, ArrowSquareTip, ArrowCircleTip


class ArrowTest(Scene):
    def construct(self):
        a11 = Arrow(np.array([-2,3,0]), np.array([2, 3, 0]))
        a12 = Arrow(np.array([-2,2,0]), np.array([2, 2, 0]),
                    tip_shape=ArrowTriangleTip,
                    tip_style={'fill_opacity': 0, 'stroke_width': 3})
        a21 = Arrow(np.array([-2,1,0]), np.array([2, 1, 0]), tip_shape=ArrowSquareTip)
        a22 = Arrow(np.array([-2,0,0]), np.array([2, 0, 0]), tip_shape=ArrowSquareTip,
                    tip_style={'fill_opacity': 0, 'stroke_width': 3})
        a31 = Arrow(np.array([-2,-1,0]), np.array([2, -1, 0]), tip_shape=ArrowCircleTip)
        a32 = Arrow(np.array([-2,-2,0]), np.array([2, -2, 0]), tip_shape=ArrowCircleTip,
                    tip_style={'fill_opacity': 0, 'stroke_width': 3})
        self.play(ShowCreation(a11), ShowCreation(a12), ShowCreation(a21), 
                  ShowCreation(a22), ShowCreation(a31), ShowCreation(a32))
        self.wait(1)
        self.play(*[FadeOut(obj) for obj in self.mobjects])

yields

ArrowTest

Acknowledgement

@huguesdevimeux huguesdevimeux added the new feature Enhancement specifically adding a new feature (feature request should be used for issues instead) label Aug 28, 2020
@huguesdevimeux
Copy link
Member

Thanks for your contribution !
As this PR is still WIP; I'm marking it as draft.

@huguesdevimeux huguesdevimeux marked this pull request as draft August 28, 2020 13:52
@leotrs leotrs added the enhancement Additions and improvements in general label Aug 28, 2020
@leotrs
Copy link
Contributor

leotrs commented Aug 28, 2020

WOWOWOW This is beautiful thanks so much! I'll review in a sec, but thank you!

Copy link
Contributor

@leotrs leotrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff here!

One more suggestion: I think the current way of creating filled/hollow tips is a bit awkward. Why not add a filled class to each? In this way this code

a12 = Arrow(np.array([-2,2,0]), np.array([2, 2, 0]),
                    tip_shape=ArrowTriangleTip,
                    tip_style={'fill_opacity': 0, 'stroke_width': 3})

becomes this

a12 = Arrow(np.array([-2,2,0]), np.array([2, 2, 0]),
                    tip_shape=ArrowTriangleFilledTip)

@behackl
Copy link
Member Author

behackl commented Aug 28, 2020

Thank you for this lightning-fast review! I was just typing that you don't need to stress yourself, as I don't know how soon I can add documentation and tests. 😄

I'll look at your comments a bit later -- thanks again!

@huguesdevimeux
Copy link
Member

huguesdevimeux commented Aug 28, 2020

Speaking of tests, I think we should wait for #335 to be merged before adding new tests, so it will be easier.

@behackl
Copy link
Member Author

behackl commented Aug 29, 2020

One more suggestion: I think the current way of creating filled/hollow tips is a bit awkward. Why not add a filled class to each? In this way this code

a12 = Arrow(np.array([-2,2,0]), np.array([2, 2, 0]),
                    tip_shape=ArrowTriangleTip,
                    tip_style={'fill_opacity': 0, 'stroke_width': 3})

becomes this

a12 = Arrow(np.array([-2,2,0]), np.array([2, 2, 0]),
                    tip_shape=ArrowTriangleFilledTip)

Great idea! It really was rather clunky before. I've added some convenience classes for filled/hollow tips in e8d076c. :-)

@leotrs
Copy link
Contributor

leotrs commented Aug 29, 2020

Great great stuff. BTW, it was extremely helpful of you to leave the commit link in each of your comments. Super easy to review!

@behackl
Copy link
Member Author

behackl commented Aug 29, 2020

Great great stuff. BTW, it was extremely helpful of you to leave the commit link in each of your comments. Super easy to review!

Thank you for the kind words and your reviews! The thing with the commit hashes in the comments is what I like most when I have to review stuff, glad you like it as well. 😄

I'll finish this here up by writing some docstrings, then the draft status of the PR can be removed again. As @huguesdevimeux suggested above, I'll wait with adding tests until #335 is ready (or take care of that in a follow-up PR). 👍

@leotrs
Copy link
Contributor

leotrs commented Aug 29, 2020

Sounds like a plan

@behackl behackl changed the title [WIP] Custom Arrow Tips Custom Arrow Tips Aug 30, 2020
leotrs
leotrs previously requested changes Aug 30, 2020
@behackl
Copy link
Member Author

behackl commented Aug 30, 2020

Formatting inconsistencies are fixed in e0acdeb.

Changing how tips are added to arrows also slightly changed the visuals of DoubleArrow, which is why the corresponding doctest has to be fixed in 48acf99.
Before:
image

After:
image

(Notice that the tips were not equally large before, this is fixed now.)

@behackl behackl marked this pull request as ready for review August 30, 2020 23:12
@leotrs leotrs dismissed their stale review August 30, 2020 23:25

outdated

@behackl behackl mentioned this pull request Aug 31, 2020
1 task
@huguesdevimeux huguesdevimeux linked an issue Sep 1, 2020 that may be closed by this pull request
@huguesdevimeux huguesdevimeux added the test requested Implementation of tests are requested label Sep 1, 2020
@leotrs
Copy link
Contributor

leotrs commented Sep 2, 2020

@behackl can you please solve the conflicts and make sure that the tests still pass? We just merged #335 which completely rehauled the test suite

@behackl
Copy link
Member Author

behackl commented Sep 2, 2020

There was no actual conflict, I only needed to merge master locally. And of course, black still fails.

Unfortunately, the tests revealed another inconvenience: due to different architectures, numerics are slightly different between operating systems, which leads to numpy yielding -0.0 in macos and 0.0 otherwise. 🙄

I'll come up with a fix for this, or replace the faulty doctests.

@behackl
Copy link
Member Author

behackl commented Sep 2, 2020

Ready to be reviewed again!

::
>>> arrow = Arrow(np.array([0, 0, 0]), np.array([2, 0, 0]), buff=0)
>>> arrow.tip.base.round(2)
>>> arrow.tip.base.round(2) + 0. # add 0. to avoid negative 0 in output
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, I did not know about this hack

@leotrs
Copy link
Contributor

leotrs commented Sep 2, 2020

Can we get one more approval here? This is super ready @huguesdevimeux @Aathish04

Copy link
Member

@Aathish04 Aathish04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contributions!

Copy link
Member

@huguesdevimeux huguesdevimeux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM !

@leotrs leotrs merged commit dc268f7 into ManimCommunity:master Sep 2, 2020
@leotrs
Copy link
Contributor

leotrs commented Sep 2, 2020

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Additions and improvements in general new feature Enhancement specifically adding a new feature (feature request should be used for issues instead) test requested Implementation of tests are requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support different arrow tips / annotation options

4 participants