ManimCommunity / manim

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

The second parameter should not be named with a fixed literal string "dt" #3197

Open pstricks-fans opened 1 year ago

pstricks-fans commented 1 year ago

Usually function parameters are dummy variables that can be named anything. Forcing us to use dt just adds unnecessary confusion to users.

https://github.com/ManimCommunity/manim/blob/117563e1f01adafe675f011ad425a257753ec9f4/manim/mobject/mobject.py#L975

MrDiver commented 1 year ago

Or it enforces readable code. If it is a time based updater the parameters should be named accordingly.

pstricks-fans commented 1 year ago

It makes less readable because using other names do not work until we open the source code and figure out the suggested name. One more thing, if it is considered as a by-design principle, why is it not adopted throughout the code base? Mostly other callbacks do not adopt the enforcing-readability principle. For example, the first argument can be mob, mobject, m, whatever. Only the second argument must be dt to enforce readability while the first one can be any name?

behackl commented 1 year ago

It makes less readable because using other names do not work until we open the source code

Technically this is also mentioned in the documentation, albeit the wording is not necessarily strong enough to make it clear that it will not work with other parameter names.

a by-design principle

That's not what I would call it -- it is something that has been implemented by Grant like that at some point. I haven't asked him about his thoughts when doing so, but I'd guess that the motivation was being able to pass updater functions with multiple (keyword) arguments and have a distinct way of letting Manim infer whether it should be called every frame, or only when a dependent Mobject updates. I don't think that readability was part of the argument.

While I don't have a particularly strong opinion on the naming per se, I strongly dislike the current implementation where the arguments of the passed function have to be inspected. I would like to get rid of that -- the easiest way to do so would be to cleanly separate between time-based and other updaters, and implementing something along the lines of a Mobject.add_time_updater(func: Callable[[Mobject, float], None], ...) while deprecating the code that inspects the arguments being passed to Mobject.add_updater.

It is worth noting that while 3b1b/manim has introduced neat type hints for the two updater types (time-based and non-time-based), he still uses the dt-mechanism to decide which category a passed function belongs to.


TL;DR: No opinion on the naming enforcement per se, but I dislike the inspection-based implementation and would rather have split add_updater, add_time_updater methods. I am unsure, whether it is worth breaking all existing updater code for this though.

pranav-ravuri commented 1 year ago

So, in the meanwhile should we update the documentation, just change the wording or add it as a gauche. If so, I would like to help.

MrDiver commented 10 months ago

So, in the meanwhile should we update the documentation, just change the wording or add it as a gauche. If so, I would like to help.

Yes!