ManimCommunity / manim

A community-maintained Python framework for creating mathematical animations.
https://www.manim.community
MIT License
24.33k stars 1.7k forks source link

Removing FadeInFromDown and FadeOutAndShiftDown #112

Closed huguesdevimeux closed 4 years ago

huguesdevimeux commented 4 years ago

In my opinion they are completly useless since when FadeInFromand FadeOutAndShiftare called direction = DOWN is set by default.

(And FadeInFrom(mobject, direction = DOWN) , FadeOutAndShift(mobject, direction = DOWN) are equivalent)

EDIT : I mixed up FadeInand FadeInFrom. I'm talking about the latter; FadeInis fine

leotrs commented 4 years ago

Paging @XorUnison, what do you think about this in terms of convenience?

PhilippImhof commented 4 years ago

From a user's perspective, it is strange to have specific functions for fading from one direction, but not from others. Even though I am too small a sample to be representative, I would prefer a generic function with the direction given as a parameter and a reasonable default.

XorUnison commented 4 years ago

I mean, there's also the factor that manim used to have this whole convoluted process of mixing animations. Now we can just do:

self.play(mobject.rotate,np.pi,
          mobject.shift,DOWN)

And call it a day.

That's because manim can now mash and mix all properly coded methods of mobjects such as shift and rotate. Works with custom stuff too.

So what we should do is probably nuke those archaic methods, and instead add mobject.fade_out and mobject.fade_in. Then we can just go the much better way of:

self.play(mobject.fade_out,np.pi,
          mobject.shift,DOWN)

And any other combination we may want.

(And FadeIn(mobject, direction = DOWN) , FadeOutAndShift(mobject, direction = DOWN) are equivalent)

This... however doesn't seem to be the case on my end? FadeIn is FadeIn for me, direction passed or not. I see no way for FadeIn to digest a direction into anything meaningful either.

PgBiel commented 4 years ago

Now we can just do:

Hol' up, have we fixed the multiple animations on one mobject issue already? On which PR?

XorUnison commented 4 years ago

Which issue exactly? I see no such open issue and this is currently possible in vanilla manim, and I've not been able to find any issues with it either. What's the problem?

PgBiel commented 4 years ago

Oh... I was thinking of the "bug" that doesn't let you do

self.play(Rotating(mobj), FadeIn(mobj))

(for e.g.)

XorUnison commented 4 years ago

Right, this has been a thing for a good long while, and I'm not even sure what the issue is, nor if it can and should be fixed. But as I said, the syntax I highlighted above does now work. But as far as I know it requires mobject.methods to work with.

leotrs commented 4 years ago

So it seems that there's agreement on removing FadeInFromDown and FadeOutAndShiftDown and other similar methods.

Now, do we want to just nuke them or deprecate them (ie issue a DeprecationWarning for now and delete them later)?

PhilippImhof commented 4 years ago

I prefer the latter. It is always frustrating when stuff disappears without prior notice. If nobody else is currently on it, I might be able to prepare a PR to solve this issue.

leotrs commented 4 years ago

@huguesdevimeux, or anyone else, are there any volunteers to write a PR that deprecates FadeInFromDown, FadeOutAndShiftDown, and other similar methods?

huguesdevimeux commented 4 years ago

@PhilippImhof Feel free to submit a PR :)

PhilippImhof commented 4 years ago

@PhilippImhof Feel free to submit a PR :)

Haha, j'allais dire la même chose.... 😊 I'll have a look at it, but I 'm not sure whether I got all of it right.

leotrs commented 4 years ago

but I 'm not sure whether I got all of it right.

That's what code reviews are for! Looking forward to your PR.