ManimCommunity / manim

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

Command length and general user convenience #65

Closed XorUnison closed 4 years ago

XorUnison commented 4 years ago

I think we'll have to drag this issue in the open and properly discuss it, rather sooner than later.

Right now the code supports a wide range of arguments for some desired parameters. For instance using medium quality. -m -r medium -r 720,1280 (although this would default to 60FPS, not 30) --medium_quality --resolution medium

Personally I'm fine with that. It's not like this is adding over hundred lines of code, when reading the code it's easy to isolate and skip these areas, and the performance overhead is almost certainly not even anywhere near being measurable.

Now we have people like @huguesdevimeux who are for cutting down some of that (#64), with which I could live. Although I think that's not really necessary.

We also have @eulertour with statements like... this here on my current PR:

At some point these should probably be merged into a single --render_quality flag which takes 4 arguments.

Let's also make properly clear what's at stake with decisions like these. Currently, and particularly with my pending constants.py changes we can drive manim like this: manim project scene -psm I like it that way. When working on my projects I usually have to re-render stuff a lot, and for very different reasons, so using it like this I often switch the m for an l, or remove it, or, now with the new changes, add a k. I can quickly get manim to get me what I need.

On the other hand it sounds like @eulertour is more in favor of enforcing people to have to use manim like this: manim C:\FullProjectFolderName\project.py scene --media_dir D:\SomeOtherProbablyNotSuperShortAbsolutePath --preview --save_last_frame --render_quality medium

Now I don't know about you, but for me, that there is just ugly and panic inducing in the extreme. I unironically estimate that if I always had to use manim like this then my recent video would've probably included a couple real extra hours of work, solely dedicated to writing out and copypasting commands. Please, for the love of deer goats, let's not go there. Yes?

If anything, manim is tool. It's code is there for us to create something that is as powerful and convenient to use as possible. We should never sacrifice even the tiniest bit of either of those unless there's substantial gains for the other we can't get otherwise. I'd even argue we should add the ability to define FPS in the arguments as opposed to having it only tethered to predefined qualities. And maybe add an addition to not just start at a specific animation but also end at one as opposed to always rendering through to the end.

Thoughts and opinions?

kilacoda-old commented 4 years ago

I concur. We don't want to make manim as complicated as something like ffmpeg

kilacoda-old commented 4 years ago

Besides, how does having shortcuts hurt @eulertour?

leotrs commented 4 years ago

-m -r medium -r 720,1280 (although this would default to 60FPS, not 30) --medium_quality --resolution medium

IMO, this is confusing. A new user will not necessarily know that these are all available, and may (understandably) think that these are doing different things. I think this should change.

On the larger issue of accepting long/many arguments. I think there is a notion here of options that should always apply for a project, vs options that I will change from execution to execution even within the same project. For example, for a single project, I am unlikely to change the media_dir more than a few times. But I am likely to render in different resolutions while working on the same project. Concretely, I think the resolution flag will be used much more frequently than the media_dir flag. I suggest that we separate all of the less-frequently used options and put them in a .cfg file. The more frequently options should stay, and they should be easy to change, which means having single-letter flags.

huguesdevimeux commented 4 years ago

I agree with @leotrs . Aside, I think the flags aren't organized well. In my opinion, we should have 4 sorts of flags : The first one would be something with related to quality in general as it is currently (--low_quality, etc ..) that would contain a preset of settings (pixel_height, pixel_width, frame_rate). I think this is enough for most of the users. But, the 3 others flags would be used to customize these settings: -d for the definition, -f for the framerate.

PS : We should pay attention to the fact that resolution != definition. Resolution is in Pixels per one inch and definition is the size of the video (1920 x 1080). I wanted to do a PR about that but it would have been useless since we are going to rebuild the whole thing.

leotrs commented 4 years ago

We don't want to make manim as complicated as something like ffmpeg

But manim does use ffmpeg. Is there a way to grab extra CLI arguments and send them to ffmpeg? This would save us a lot of implementation and just rely on what ffmpeg already provides.

XorUnison commented 4 years ago

IMO, this is confusing. A new user will not necessarily know that these are all available, and may (understandably) think that these are doing different things. I think this should change.

Well, -h is dropping information about what these do, although honestly it's really lacking. I've added brief mentions of render time to the different qualities in my PR, because it's everything but obvious that high quality is actually below production quality. In the first place, if you don't look into the code you never either hear of this so called production quality, nor are you going to find out what resolution and FPS it is until the very moment you inspect a finished file. This is kinda bad. When I go -h on a cmd tool I expect to find out what's up in a concise, but adequately verbose manner. Neither as a user, nor as a dev do I always want to be forced to go into the code to understand what everyday arguments do. So I agree, it is confusing. If we actually want to take options away we'll want to carefully decide which ones. The shortest commands should absolutely stay, because as I said, convenience of use. And we'll want to rework -h to explain stuff better, not worse.

On the larger issue of accepting long/many arguments. I think there is a notion here of options that should always apply for a project, vs options that I will change from execution to execution even within the same project. For example, for a single project, I am unlikely to change the media_dir more than a few times. But I am likely to render in different resolutions while working on the same project. Concretely, I think the resolution flag will be used much more frequently than the media_dir flag. I suggest that we separate all of the less-frequently used options and put them in a .cfg file. The more frequently options should stay, and they should be easy to change, which means having single-letter flags.

I have never seriously used --media_dir until the moment I had to work with it to test my PR. I have one HDD, and on it one folder where I want my stuff to go, and that's it. In vanilla manim I did that by using the DIR variables in constants.py, which my PR moves to it's own file, because of course every time constants.py was updated that spelt trouble. I'm also all for a .cfg file. The dirs.py in my PR is basically a prototype for that exact paradigm. Something the user can set once and forget, and that (after we're done with it) won't have to be changed again, or only very, very rarely.

I agree with @leotrs . Aside, I think the flags aren't organized well. In my opinion, we should have 4 sorts of flags : The first one would be something with related to quality in general as it is currently (--low_quality, etc ..) that would contain a preset of settings (pixel_height, pixel_width, frame_rate). I think this is enough for most of the users. But, the 3 others flags would be used to customize these settings: -d for the definition, -f for the framerate.

Preset flags and manual specification flags. Sure that'd be fine. Small nitpick, when we introduce a framerate flag it's gonna have to be something else since -f is used. Like I had to use -e for high quality because any and all other proper letters were taken.

PS : We should pay attention to the fact that resolution != definition. Resolution is in Pixels per one inch and definition is the size of the video (1920 x 1080). I wanted to do a PR about that but it would have been useless since we are going to rebuild the whole thing.

I think you're SoL with that point. Even when you go into the Windows system settings and look for definition to change your "definition" you won't even find the setting. It's called resolution. Even Wikipedia defines resolution first as the number of pixels, not density, before it's throwing the caveats into the discussion. If we pick up the torch of definition versus resolution we're just going to confuse the living daylights out of users. Best to just drop it and move on.

leotrs commented 4 years ago

Let me try to gather the current action items.

  1. Identify which things should be constants (never change), go on config files (things that almost never change, like project-wide settings), and which things may change frequently (like resolution).
  2. The first kind of things should go to a constants file, which already exists. The second kind should go on a .cfg file which does not yet exist. The third kind should be kept as cli arugments.
  3. The cli arguments should have a short-hand (one letter) version, and a succinct but clear description, to be displayed when showing the help (with the -h flag).
  4. Decide which cli argument will have a default value, and what that value should be.
  5. Decide how much flexibility we want our cli arguments to have. For example, do we want to expose every single argument supported by ffmpeg?
  6. Regarding "production quality": we need to either expose it to the user, or delete it from the code base. This is the default (so it is exposed to the user), but it is not documented anywhere. It is also not clear whether this equals any of the others (low, medium, high). We should document it better, include in the description output by -h.

Please feel free to continue discussing these, they are not set in stone. I just don't want things to fall out of the radar.

XorUnison commented 4 years ago
6. Regarding "production quality": we need to either expose it to the user, or delete it from the code base.

Just a quick and immediate interjection here, "production quality" is the preset manim uses when you pass no resolution arguments at all, so deleting it from the code base would be... bad. As such it's also "exposed" since it's the default. We should just have -h stating the resolution somewhere.

leotrs commented 4 years ago

@XorUnison thanks, ammended.

XorUnison commented 4 years ago

Let me try to gather the current action items.

1. Identify which things should be constants (never change), go on config files (things that almost never change, like project-wide settings), and which things may change frequently (like resolution).

This would be in the realm of #7 though, right?

2. The first kind of things should go to a constants file, which already exists. The second kind should go on a .cfg file which does not yet exist. The third kind should be kept as cli arugments.

I can't currently think of any important user defined base constants outside of the dirs right now. I mean we could include a preferred base resolution, but we already have single character shorthands for all resolutions. So I'm not sure this really would be valuable. Then again... while I and many others always render stuff in 16:9 format for YT or something, there are also those cases where people might want to render in other formats such as 1080x1080... If so we'd have to expose "production quality" though. Up for more discussion I guess.

3. The cli arguments should have a short-hand (one letter) version, and a succinct but clear description, to be displayed when showing the help (with the `-h` flag).

I should probably bite the apple and add this to my PR, as well. Although it's so many commits it's getting a bit unsightly.

4. Decide which cli argument will have a default value, and what that value should be.

More like, do we know one we need to specifically address?

5. Decide how much flexibility we want our cli arguments to have. For example, do we want to expose every single argument supported by ffmpeg?

Now that would be quite a lot of overhead. If possible we should just have something like --ffmpeg "string of arguments and stuff" that's passed along.

leotrs commented 4 years ago

This would be in the realm of #7 though, right?

7 is about replacing CONFIG with dataclasses, no? Are any of these options being stored in CONFIG somehow? I think these are different things... In any case, I think there's a new issue that needs to be opened in terms of cleaning up the constants.py file.

I can't currently think of any important user defined base constants outside of the dirs right now. I mean we could include a preferred base resolution, but we already have single character shorthands for all resolutions. So I'm not sure this really would be valuable. Then again... while I and many others always render stuff in 16:9 format for YT or something, there are also those cases where people might want to render in other formats such as 1080x1080... If so we'd have to expose "production quality" though. Up for more discussion I guess.

There's nothing wrong with moving some things to a .cfg file... and then providing a default config file. It's just a different way of exposing different options. It sounds like this config file would contain (at least) the directories, and the default resolution.

It can also be a system where the .cfg file can be used to set the project-wide default value of any of the cli arguments. If the user provides any argument on the command line, it would override what's in the .cfg file. This saves us the step of deciding which options go where.

Now that would be quite a lot of overhead. If possible we should just have something like --ffmpeg "string of arguments and stuff" that's passed along.

This is what I meant. Just have a catch-all argument and dump it into ffmpeg. No need to process anything on our end.

huguesdevimeux commented 4 years ago

It can also be a system where the .cfg file can be used to set the project-wide default value of any of the cli arguments. If the user provides any argument on the command line, it would override what's in the .cfg file. This saves us the step of deciding which options go where.

I think that this is the simplest way. There is still a problem that I think can be very annoying for some people. With this functioning, one would have to re-enter the same arguments over and over, if the values that one wants are not the one set by default as long as one works on a project. Maybe would for he user can for that purpose directly modify the .cfg (the .cfg could be generated in the project directory, idk (that supposes that everyone always create a project directory, which .. is not the case)) to have user-set default values or we can add a flag to save a configuration (so when running manim without any arguments it would take the arguments set by the user). In fact the question is, where should be the .cfg ?

EDIT: sorry for the close-reopen. Missclick :/

leotrs commented 4 years ago

Maybe would the user can for that purpose directly modify the .cfg (the .cfg could be generated in the project directory)

That is exactly the purpose. The workflow would be:

  1. download/install
  2. decide what options are appropriate for the project, and put them in a .cfg file
  3. start running manim

We would of course provide a default .cfg file in case users do not want or need to .cfg file. If the default .cfg file is powerful enough, then step number 2 would be unnecessary in most cases. (There are easy ways of setting up default configuration files without littering the user's folders.)

eulertour commented 4 years ago

On the other hand it sounds like @eulertour is more in favor of enforcing people to have to use manim like this: manim C:\FullProjectFolderName\project.py scene --media_dir D:\SomeOtherProbablyNotSuperShortAbsolutePath --preview --save_last_frame --render_quality medium

@XorUnison There's no need for pointed remarks like this, just voice your concerns if you have them. I'll just be as transparent as possible; if it keeps happening I'm likely to remove you from the org.

Right now the code supports a wide range of arguments for some desired parameters. For instance using medium quality. -m -r medium -r 720,1280 (although this would default to 60FPS, not 30) --medium_quality --resolution medium

I think this pretty bad too, and the -r flag should probably just take two numbers. Additionally, the versions taking a string are broken at the time of writing.

Currently, and particularly with my pending constants.py changes we can drive manim like this: manim project scene -psm I like it that way.

This is valid, however manim has covered 17 of the 26 single-letter flags available to it and the number covered by resolutions is growing. Not only that, but as we continue to work on it the number of features is growing, which will inevitably lead to even more flags. That's why I think consolidating the ones related to quality could be useful. This doesn't have to drastically change the length of the command line, as it could always be something like manim module scene -ps -q[l|m|h|p|k].

XorUnison commented 4 years ago

@XorUnison There's no need for pointed remarks like this, just voice your concerns if you have them. I'll just be as transparent as possible; if it keeps happening I'm likely to remove you from the org.

That wasn't meant in a personal manner, just to be clear. The reason I displayed this dramatic case is simply that if we occasionally purge convenience tools from the code base it will add up quickly. And that's just really how it can look, the code would already work like that. One of the purposes of this issue is to see if we can agree on not purging any convenience, verbosity or usability unless we really can't help it. Also you didn't quote the more pointed part there, (I'd think that's how I called this convention, ugly and scary) because that part you quoted isn't pointed at all, it's genuinely what you made me believe. You have suggested the removal of convenience and making more obscure UI to me several times, and in very clear words. I mean, I can tell you it isn't meant personal as much as I can, but the thing is you suggested first making the use of --media_dir a necessary thing, you suggested to make -h less verbose than it already is (and as I said, it's already totally unclear what this production quality is and how it relates to high quality, short of either testing out everything manually or sifting the code [I only learned it through testing]), and then you suggested removing all resolution shorthands in favor of something that would explode minimum cli command length by roughly 100% of what it usually is, and something up to 200% for long dir names. I'm not saying that out of resentment, that's just how you presented your priorities to me. If that's not what you meant we've got to work on our communication. I feel it's important we talk about this clearly and openly with everything that's at stake. And if you take offense at my packaging I can apologize, at least if you tell me what exactly rubbed you the wrong way. But I must also ask you to not take that personal because it isn't. I've put many, many more hours into the constants.py rework, almost an order of magnitude more than I initially planned, specifically to accommodate your requests. If I had personal pickings with you I wouldn't have done that. So. Burry that hatchet?

I think this pretty bad too, and the -r flag should probably just take two numbers. Additionally, the versions taking a string are broken at the time of writing. Sounds like we have a rather one-sided agreement on this then.

This is valid, however manim has covered 17 of the 26 single-letter flags available to it and the number covered by resolutions is growing. Not only that, but as we continue to work on it the number of features is growing, which will inevitably lead to even more flags. That's why I think consolidating the ones related to quality could be useful. This doesn't have to drastically change the length of the command line, as it could always be something like manim module scene -ps -q[l|m|h|p|k].

Right, but this wasn't the suggestion that alarmed me. If we run into this issue and want to rework the character flags this is a sensible way of doing that. Edit: Kind of an obvious point but if we do this, old ways of driving manim will cease working. Not a real backwards compatibility issue, but still something to keep in mind.

On a side note... Do we have any idea what we'd need so many more character flags for? I mean they're only needed for fairly specific settings. If you got something in mind I'm curious.

eulertour commented 4 years ago

you suggested first making the use of --media_dir a necessary thing

This isn't true, I always suggested allowing the --media_dir flag and defaulting to the current directory otherwise. Until we have a persistent config file that's the best the library can do, and as I also said when I mentioned it, you can edit your copy of the repo to do this automatically if you want.

you suggested to make -h less verbose than it already is (and as I said, it's already totally unclear what this production quality is and how it relates to high quality...

I suggested removing the comment about render speed from the flag for --low_quality, and not adding 3 more comments all saying roughly the same thing. In hindsight I even agreed with you that leaving the comment for only --low_quality probably makes sense. The additions you were adding didn't address the discrepancy between high quality and production quality at all.

then you suggested removing all resolution shorthands in favor of something that would explode minimum cli command length by roughly 100% of what it usually is, and something up to 200% for long dir names.

These are two different things, the --media_dir flag and the --render_quality one I proposed. I addressed your concern about the quality flag in my last comment reiterated the --media_dir one here.

And if you take offense at my packaging I can apologize, at least if you tell me what exactly rubbed you the wrong way.

We can discuss this offline if you like.

I've put many, many more hours into the constants.py rework, almost an order of magnitude more than I initially planned, specifically to accommodate your requests.

I don't really know what to say regarding this other than that writing good software can just be challenging sometimes.

Right, but this wasn't the suggestion that alarmed me.

If you voice your concerns when they occur to you I can address them quickly.

Do we have any idea what we'd need so many more character flags for?

It isn't that they're needed for anything now, it's that they're growing quickly and will eventually force us to use to either use long flags or single letters that are unintuitive, like -e for high quality.

leotrs commented 4 years ago

@XorUnison @eulertour I saw this conversation was moved to Discord (which is great!). Could either of you two please briefly summarize here what is the current status of this issue i.e. what needs further discussion? The current conversation is a bit hard to follow. Thanks.

PgBiel commented 4 years ago

XorUnison There's no need for pointed remarks like this, just voice your concerns if you have them.

I gotta admit, when I read it I did feel it was kinda aggressive... but glad to see things are working out between you two... I think?

Anyway, about all of this... (there's just too much to quote one part) I'm in favor of reducing them all into one -r flag. We can just have something like -r m and that's it. That's adding 2 characters, and might even be clearer than before. --render_quality would just be an alias. (We need at least one alias. Come on!)

That's my opinion

eulertour commented 4 years ago

I think the gist is

  1. Flags that are specified often such as the ones for controlling the render quality should always have a short option, it seems like we're okay with -q[l|m|h|p|k]
  2. For --media_dir which is specified often but may have a long path name that we can't control, we need to implement a .cfg file
  3. It doesn't seem like anyone is opposed to getting rid of the --video_dir, --tex_dir, etc files, so they can be handled automatically from --media_dir
  4. A --production_quality flag should be added to show that it's the default, and the help for the other options should give their p string in their help message
XorUnison commented 4 years ago

I gotta admit, when I read it I did feel it was kinda aggressive... but glad to see things are working out between you two... I think?

We're fine as far as I think. Talked it over on Discord, and as I said there, no offense was meant by me.

Anyway, about all of this... (there's just too much to quote one part) I'm in favor of reducing them all into one -r flag. We can just have something like -r m and that's it. That's adding 2 characters, and might even be clearer than before. --render_quality would just be an alias. (We need at least one alias. Come on!)

That's my opinion

Yeah. -r x wouldn't make any real difference to the shortest use I can think of and if anything would make it easier because we'd have all 26 letters free for resolutions after an -r (or q or whatever, though I'd stick with r). The more I thought about this, the more I shifted towards being in favor of that. We could also see about making some of the letter assignments more fitting maybe, if we completely remove m/l/e/k for resolutions and instead refer to -r for that.

leotrs commented 4 years ago

I'm forking this into separate issues for simpler tracking of the implementation of what was discussed here. If I got anything wrong, please let's discuss there.

  1. Implement -q[l|m|h|p|k] to specify render quality. Also, a --production_quality flag should be added.

  2. The --media_dir problem will be fixed once #98 is merged. There is already a way of determining media_dir (and others) in the current state of the PR.

  3. Get rid of --video_dir, --tex_dir.

leotrs commented 4 years ago

Closing this now. if there's anything that the new issues don't cover from this conversation, please open a new issue specifically for that instead of reopening this one.