-
Notifications
You must be signed in to change notification settings - Fork 2.2k
t values issue that causes the animations to not be finished entirely. #183
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
So there are two problems here. Correct me if I'm wrong.
If fixing 2 is too costly, then we can just fix 1 and open a new issue for 2. I understand this interacts with the tests, so we should try to fix this asap. |
See these lines : l678 scene.py if file_writer_config['skip_animations'] and not override_skip_animations:
times = [run_time]
else:
step = 1 / self.camera.frame_rate
times = np.arange(0, run_time, step)
time_progression = ProgressDisplay(
times, total=n_iterations,
leave=file_writer_config['leave_progress_bars'],
ascii=False if platform.system() != 'Windows' else True
) When -s is called, On second thought the problem does not come from the progress bar but from np.arrange that yield 15 values from 0 to 0.93333 (although we should change that). number_frames = int(self.camera.frame_rate * run_time) #arbitrray rounded down, maybe should be round this up ?
times = np.linspace(0, run_time, number_frame) With that we would have a correct repartition of all the t values. And I completely agree with 2. And btw it does not interact with the tests. We can merge #185 before or after that, it does not matter :) |
Let's see,
While
One uses |
I think since we are changing this logic we could think of a more general refactoring, and do what you suggested in you second point. I already have 2 PR to take care of, so I won't be able to PR this right now. Feel free yo do it if you have time ;D |
What is "free time"? |
An interesting concept that only high school students under lockdown seem to benefit from lol |
We need to round this as np.linspace only takes integrers as third parameter. I missed that when I read your answer |
Oh I see now. I'd say we should round up! |
Keep in mind that if we do this then the last frame of animation n and the first frame of animation n+1 will always be the same, which would cause an unavoidable pause between animations which would be especially noticeable at low frame rates. It wouldn't be possible to create the effect where animation 1 runs from 0s to 1s and animation 2 runs from 1s to 2s. This isn't technically possible now, since playing an animation for 1s at a given frame rate generates <frame_rate> frames, which is only (frame_rate-1)/frame_rate seconds of video. If you did want to do this, you'd probably want to increase the number of samples to 1 more than the frame rate so that each animation played for 1s rather than (frame_rate-1)/(frame_rate) seconds as it does now. Consecutive calls to play() would then generate video lengths of 1s, (2 + 1/frame_rate)s, (3 + 2/frame_rate)s, etc. However, I think the proper solution is just to add a terminating frame at the end of the last animation, so that consecutive calls to play() would generate video with frame counts of 16, 31, 46, etc., which corresponds to video lengths of 1s, 2s, 3s, etc. at 15fps. |
This comment has been minimized.
This comment has been minimized.
@eulertour I didn’t think about that. This is a very good point.
So seeing your note this is IMO the best solution. @kilacoda Did you implement number_frame variable as I did in on of my previous answer ? |
Welp. My stupidity strikes me again. Adding the number_frames variable worked. Looks like I overlooked it😅 |
@huguesdevimeux could you please update us here? I thought this was fixed. |
Well, it is not fixed. I'm working on it ! |
Update on this : Given that we don't have a solution for #214 , I don't see how to fix it without altering the number of frames. So in my opinion fixing this would require a big rewrite of the code, that is not worth in my opinion, so I'm partial to not fix this at all. I will close this in a few days in no one objects. |
I'd actually leave this one open since it's causing so much problems. We can revisit in the future. |
Coming back to this issue because I need it fixed before I can make some desired changes to the config system. @eulertour said
I agree with this. If I understand correctly, @huguesdevimeux says that this cannot be done unless we are able to count the number of play calls beforehand (because the number of frames per animation has to be set dynamically -- it will be different only for the last animation). Is it possible to do it at the end, for example in the Also, it should be very easy to write a test for this (render a simple scene with one animation with run_time = 1.0 and check the number of frames generated equals 15). |
@leotrs I've been thinking about this and I think t'it's impossible to count the number of play calls. The only solution is to do what @eulertour proposed, but in this case it would mean that 1s at 15fps generate ... 16 frames, which I don't think good. |
@huguesdevimeux Why don't you think that's good? 16 frames at 15 fps is 1 second of video. |
I mean, for two reasons :
But maybe I'm nitpicking. |
People usually aren't trying to generate a specific number of frames when they render animations, only a specific length of video. But if they were, 16 would be the correct number of frames for 1 second of video. |
I'm not sure to follow you. Are you saying that 16 frames for 1 sec at 15 FPS is correct ? Why ? |
Only the time between frames is counted toward the animation, so there is no additional 1/15 seconds added by the last frame. |
This is where I disagree. I don't think the additional time at the end of the video is the problem.
One second at 1fps contains one frame, it is basically be an image on a video. The end frame would be also the start frame. Your idea would work perfectly but I disagree. Manim being something supposed to be precise, I don't think adding such a thing is a good idea. Furthermore, this issue isn't really important (it is purely graphical) so I would be partial to not fix it. Any opinion on that @ManimCommunity/core ? (or anyone else) |
I don't think it's correct to say that if you add a frame at the end, then the video automatically becomes 2fps. 1fps means that a frame lasts for 1s. If you have a video that is 1s long and it shows one frame for 99.99% of the time, and then at the last instant changes to another frame, then that video is still 1fps because the first frame was shown for (almost exactly) 1s. The second frame would have been shown for 1s as well, if the video had lasted for 2s. So it seems to me fps really refers to the duration of each frame on screen, not the number of frames you show in the span of of a 1s window. The difference is that the first and/or last frames may be on screen for a different amount of time. Also, I'm not sure I understand when you say "this issue isn't really important (it is purely graphical)". I think we should care if there are graphical issues, since manim is designed to generate video, no? |
Leo you are correct on this. That is why some video standards have
fractions in their fps counts. In the US for example the standard for TV is
59.94 FPS or exactly 1% slower than the 60Hz electricity there.
…On Mon, Sep 7, 2020, 3:38 PM Leo Torres ***@***.***> wrote:
I don't think it's correct to say that if you add a frame at the end, then
the video automatically becomes 2fps.
1fps means that a frame lasts for 1s. If you have a video that is 1s long
and it shows one frame for 99.99% of the time, and then at the last instant
changes to another frame, then that video is still 1fps because the first
frame was shown for (almost exactly) 1s. The second frame *would have
been* shown for 1s as well, if the video had lasted for 2s.
So it seems to me fps really refers to the duration of each frame on
screen, not the number of frames you show in the span of of a 1s window.
The difference is that the first and/or last frames may be on screen for a
different amount of time.
Also, I'm not sure I understand when you say "this issue isn't really
important (it is purely graphical)". I think we should care if there are
graphical issues, since manim is designed to generate video, no?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#183 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEDJKY6OU2DY3T45OAXCQFTSETO65ANCNFSM4OUNQ4UA>
.
|
Very interesting. Then okay, let's do that.
yes, but what I meant is that this issue is minor, as I'm sure a lot of people didn't even notice. But this of course better to fix :D |
OK so the fix is to
I think the first one is easy. Can we do the second one by using |
Wait, I think this is a bad idea, as @eulertour pointed out
|
@huguesdevimeux I agree with what @leotrs said; I don't think he's describing the same situation I was talking about in your second quote. |
Yes, it was the wrong quote lol.
Sorry if I miss something, but if each frame is shown excaltly 1/fps, we will have this problem, no ? |
No, why would we? Only the last frame of the scene will have the terminating frame. |
But, this is not what @leotrs said. I mean, currently, each frame as for t value 1/(fps - 1), so the frame with EDIT : just to be clear, I'm only talking about |
@huguesdevimeux The suggestion is just to keep everything the way it is currently, but also add a terminating frame at the end of the scene. So no animation should reach t=1 except for the very last one. |
Alright then, I might have misunderstood. Anyway; we will discuss this deeper when a PR will be open. |
y values are generated with those lines : (progress_through_animations, l883, scene.py).
get_animation_time_progression
returns a ProgressBarDisplay object, from the library tdqm that we use to display the progress bar.When doing -s, skip_animations is enabled to t value is set to 1.0 (no intermediary values as we just need the last frame).
When running normally, eg with 15 fps, there are logically 15 t values, that are :
As you may see, 1 is never reached. In other terms, the animations is stopped at 93%). Not that 1.0 change depending on the rn_time : if the run_time was let's say 3, last t value would be 2.93.
We can see this issue with a single pixel that should be here :

To give an idea of a fix, I will use the words of @leotrs :
The text was updated successfully, but these errors were encountered: