-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Command length and general user convenience #65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
I concur. We don't want to make manim as complicated as something like ffmpeg |
Besides, how does having shortcuts hurt @eulertour? |
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 |
I agree with @leotrs . 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. |
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. |
Well,
I have never seriously used
Preset flags and manual specification flags. Sure that'd be fine.
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. |
Let me try to gather the current action items.
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. |
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. |
@XorUnison thanks, ammended. |
This would be in the realm of #7 though, right?
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.
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.
More like, do we know one we need to specifically address?
Now that would be quite a lot of overhead. If possible we should just have something like |
#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
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 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.
This is what I meant. Just have a catch-all argument and dump it into ffmpeg. No need to process anything on our end. |
I think that this is the simplest way. EDIT: sorry for the close-reopen. Missclick :/ |
That is exactly the purpose. The workflow would be:
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.) |
@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.
I think this pretty bad too, and the
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 |
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.
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. 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. |
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.
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.
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.
We can discuss this offline if you like.
I don't really know what to say regarding this other than that writing good software can just be challenging sometimes.
If you voice your concerns when they occur to you I can address them quickly.
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. |
@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. |
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) That's my opinion |
I think the gist is
|
We're fine as far as I think. Talked it over on Discord, and as I said there, no offense was meant by me.
Yeah. |
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.
|
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. |
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:
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?
The text was updated successfully, but these errors were encountered: