Direction methods (VMobject)

[XorUnison]

Partial implementation of #210

Added shoelace method to space_ops.py
Added orientation methods to vectorized_mobject.py, uses shoelace

For test code and a result gif of the added feature, see #210, Test1.

[behackl]

On a first glance, this looks pretty good already! I'll review this in more detail tomorrow or so.

Just as a remark: please avoid creating new branches on the ManimCommunity repository and rather push and PR from your own fork. This helps to keep the main repository clean (#590).

[XorUnison]

I see. I'm not entirely sure how that works, but if I know GitHub as much as I do it's not gonna look different than the fork/contribution interface currently. I'll keep it in mind.

[huguesdevimeux]

It's perfect and all but i'm just worried about the used of strings "CW" and "CCW" for this case. Don't you think there is a better way to do this rather than using string ?

I don't know if it is possible to do better in Python, but one solution is to use booleans instead.
I would like to hear of the other's opinion on thisl.

[leotrs]

Thanks @XorUnison ! Will review at length later but to answer @huguesdevimeux question: maybe you can use an Enum.

[behackl]

Another option would be to return +1 or -1 for positive (counterclockwise) or negative (clockwise) orientation.

[XorUnison]

It's perfect and all but i'm just worried about the used of strings "CW" and "CCW" for this case. Don't you think there is a better way to do this rather than using string ?

maybe you can use an Enum.

Another option would be to return +1 or -1 for positive (counterclockwise) or negative (clockwise) orientation.

Sure, those are all options, but I am actually rather partial to strings in this case. While those options might be marginally simpler, and there definitely is a correct way of interpreting the sign to mean one or the other direction, my concern with using these shortened strings lies in how it's going to be used and viewed by end users. Pretty much everyone and their dog knows how clockwise and counterclockwise work, and will find it likewise extremely easy to write and see code like obj.force_direction("CW")/obj.force_direction("CCW") and pretty much instantly know what's going on, or maybe have a cursory look into it once and understand it for the future. Right there it quickly reads "force this object's direction clockwise/counterclockwise". To me this strikes a balance of both short and readable.
Most people however will have no idea about the shoelace method and according signed area. obj.force_direction("-1") isn't anywhere near as obvious, and even I after having implemented this method would likely have to brush up on what that means if I haven't seen it for a week. Not to mention that - on a number line is associated with going left, while - from the top of a clock actually means going right, which isn't helping this case.
And while I suppose this could be done with Enum somehow, I don't see that being worth it. So unless there's a specific example of a different implementation that retains code readability in the final product I'd rather stick with the abbreviated strings.

[leotrs]

I think strings are superior to +1/-1, but I still think an Enum is superior to strings.

However, since in this case there are only two options, I think strings are fine.

@XorUnison would it be too much trouble to ask you to add tests? :)

[XorUnison]

@XorUnison would it be too much trouble to ask you to add tests? :)

@leotrs Unless I know where those tests would go with the new structure, it is trouble, heh. The necessary test for direction is of graphical nature, so it has no business in test_vectorized_mobject.py. But test_geometry.py isn't really the place either. So uh... you tell me where it goes, I'll see about adding it.

[behackl]

This looks very good to me! Added a bunch of tiny suggestions concerning formatting of documentation, one suggestion for a doctest, and a non-blocking wish for a rendered example.

[huguesdevimeux]

I think a classical unit test id fine for this, no?

[leotrs]

I'd rather have more tests than have them organized. So feel free to create a new file test_orientation.py or something like that.

[leotrs]

How is this looking? Is this ready for final review?

Also, @XorUnison do you think we could close #210 ? I understand this is only a partial implementation of it, but whatever future work should be done in a new PR, not in #210, right?

[XorUnison]

How is this looking? Is this ready for final review?

Well, I wish. Like I expected the amount and type of change requests is holding this up again. Expect it to be held up for a few more days again or even a week.

Also, @XorUnison do you think we could close #210 ? I understand this is only a partial implementation of it, but whatever future work should be done in a new PR, not in #210, right?

I suppose so. I have the issue bookmarked and will open new ones as I get the stuff ready, just expect that to take a lot of time too.

[leotrs]

No worries, just wanted an update, didn't want to be pushy :)

…

On Wed, Nov 4, 2020 at 9:41 PM XorUnison ***@***.***> wrote: How is this looking? Is this ready for final review? Well, I wish. Like I expected the amount and type of change requests is holding this up again. Expect it to be held up for a few more days again or even a week. Also, @XorUnison <https://github.com/XorUnison> do you think we could close #210 <#210> ? I understand this is only a partial implementation of it, but whatever future work should be done in a new PR, not in #210 <#210>, right? I suppose so. I have the issue bookmarked and will open new ones as I get the stuff ready, just expect that to take a lot of time too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#647 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAILYACNU5J5HDEAENUWTHLSOIGFTANCNFSM4TFSTJWQ> .

-- www.leotrs.com | [email protected] PhD candidate at the Network Science Institute, Northeastern University

[XorUnison]

No worries, just wanted an update, didn't want to be pushy :)

Didn't take it as being pushy, no worries there either. I'll have to deal with the manim_directive.py or whatever it was first to make the sample animation, but I also want that in there myself. That's going to be most of the delay.

[behackl]

That's going to be most of the delay.

Adding a rendered example to the docs is easy: prepare a scene to render and put it inside the docstring inside a .. manim:: directive. For example, I'd suggest something like

.. manim:: ChangeOfDirection

    class ChangeOfDirection(Scene):
        def construct(self):
            square_ccw = Square()
            square_ccw.shift(LEFT)
            square_cw = Square()
            square_cw.shift(RIGHT).reverse_direction()

            self.play(ShowCreation(square_ccw), ShowCreation(square_cw))
            

And this block is put directly into the docstring; I would put this in the examples section of the docstring of reverse_direction. The directive is documented at https://docs.manim.community/en/latest/reference/manim_directive.html#module-manim_directive if you are interested in details.

[behackl]

Looks great, and the example renders and displays correctly. Good to go!

[XorUnison]

I'd rather have more tests than have them organized. So feel free to create a new file test_orientation.py or something like that.

I agree there should be a test for these direction methods. However the only real meaningful test for this is whether or not the array, and thus the drawing direction gets inverted cleanly. While in principle a picture test could be done by snapshotting halfway into the animation, adding a proper simple video test once these are ready to go strikes me as the decidedly better path.

[leotrs]

I agree there should be a test for these direction methods. However the only real meaningful test for this is whether or not the array, and thus the drawing direction gets inverted cleanly. While in principle a picture test could be done by snapshotting halfway into the animation, adding a proper simple video test once these are ready to go strikes me as the decidedly better path.

In that case please add it to #193