Skip to content
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

t values issue that causes the animations to not be finished entirely. #183

Open
huguesdevimeux opened this issue Jul 8, 2020 · 35 comments
Assignees
Labels
issue:bug Something isn't working... For use in issues

Comments

@huguesdevimeux
Copy link
Member

y values are generated with those lines : (progress_through_animations, l883, scene.py).

        for t in self.get_animation_time_progression(animations):
            dt = t - last_t
            last_t = t
            for animation in animations:
                animation.update_mobjects(dt)
                alpha = t / animation.run_time
                animation.interpolate(alpha)
            self.update_mobjects(dt)
            self.update_frame(moving_mobjects, static_image)
            self.add_frames(self.get_frame())

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 :

0.0
0.06666666666666667
0.13333333333333333
0.2
0.26666666666666666
0.3333333333333333
0.4
0.4666666666666667
0.5333333333333333
0.6
0.6666666666666666
0.7333333333333333
0.8
0.8666666666666667
0.9333333333333333

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 :
image

To give an idea of a fix, I will use the words of @leotrs :

I've never used tqdm myself so my question is: is it orthodox/expected/good practice to determine the course of a program by querying the progress bar itself? I'm not sure how this works at all.

It'd be much more reasonable I think for manim to determine its own course of action and then tell the progress bar to reflect that, instead of the other way around

@huguesdevimeux huguesdevimeux added the pr:bugfix Bug fix for use in PRs solving a specific issue:bug label Jul 8, 2020
@huguesdevimeux huguesdevimeux changed the title t values issue that cause the animations to not be finished entirely. t values issue that causes the animations to not be finished entirely. Jul 8, 2020
@leotrs
Copy link
Contributor

leotrs commented Jul 8, 2020

So there are two problems here. Correct me if I'm wrong.

  1. progress_through_animations gives a range of t values that never reaches 1.0. I don't think this has anything to do with the config system right? In any case, can we modify progress_through_animations? It should be as easy to ask it to partition the interval in one more chunk (as right now it ends 1/15 short of 1.0).

  2. I still don't understand why the t values come from the progress bar. It should be the other way around: manim decides what t values it wants the progress bar to show.

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.

@huguesdevimeux
Copy link
Member Author

huguesdevimeux commented Jul 8, 2020

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, skip_animations is True as well and the progressbar is set up with only one step that has the length of the run_time. That's why there is only one single t value equal to 1 (in the case where run_time = 1).

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).
I would suggest just changing times = np.arange(0, run_time, step) by

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 :)

@leotrs
Copy link
Contributor

leotrs commented Jul 8, 2020

Let's see, arange does the following

In [7]: np.arange(0, 1.0, 1/15)
Out[7]: 
array([0.        , 0.06666667, 0.13333333, 0.2       , 0.26666667,
       0.33333333, 0.4       , 0.46666667, 0.53333333, 0.6       ,
       0.66666667, 0.73333333, 0.8       , 0.86666667, 0.93333333])

While linspace returns

In [8]: np.linspace(0, 1.0, 15)
Out[8]: 
array([0.        , 0.07142857, 0.14285714, 0.21428571, 0.28571429,
       0.35714286, 0.42857143, 0.5       , 0.57142857, 0.64285714,
       0.71428571, 0.78571429, 0.85714286, 0.92857143, 1.        ])

One uses 1/15 and the other uses 15. So I'm not sure why the need to round anything. But in any case, yes it seems like using linspace is going to do the trick. PR?

@huguesdevimeux
Copy link
Member Author

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

@leotrs
Copy link
Contributor

leotrs commented Jul 8, 2020

What is "free time"?

@huguesdevimeux
Copy link
Member Author

huguesdevimeux commented Jul 8, 2020

What is "free time"?

An interesting concept that only high school students under lockdown seem to benefit from lol

@huguesdevimeux
Copy link
Member Author

huguesdevimeux commented Jul 8, 2020

One uses 1/15 and the other uses 15. So I'm not sure why the need to round anything. But in any case, yes it seems like using linspace is going to do the trick.

We need to round this as np.linspace only takes integrers as third parameter.
Example : run_time = 2.5 seconds, at 15fps. How many frames will be generated ? 37 or 38 ?

I missed that when I read your answer

@leotrs
Copy link
Contributor

leotrs commented Jul 8, 2020

Oh I see now. I'd say we should round up!

@eulertour
Copy link
Member

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.

@kilacoda-old

This comment has been minimized.

@huguesdevimeux
Copy link
Member Author

@eulertour I didn’t think about that. This is a very good point.

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.

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 ?

@kilacoda-old
Copy link
Contributor

kilacoda-old commented Jul 12, 2020

Welp. My stupidity strikes me again. Adding the number_frames variable worked. Looks like I overlooked it😅

@leotrs
Copy link
Contributor

leotrs commented Aug 2, 2020

@huguesdevimeux could you please update us here? I thought this was fixed.

@huguesdevimeux
Copy link
Member Author

@huguesdevimeux could you please update us here? I thought this was fixed.

Well, it is not fixed. I'm working on it !

@huguesdevimeux
Copy link
Member Author

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.
My initial idea was to change the repartition of the t values to a regular one only for the last play/wait call but it implies knowing when the last invocation is (#214), which is currently impossible.
Otherwise, we could add the last frame at the end of the video but it would give one additional frame to the video, which is not good.

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.

@leotrs
Copy link
Contributor

leotrs commented Aug 3, 2020

I'd actually leave this one open since it's causing so much problems. We can revisit in the future.

@leotrs
Copy link
Contributor

leotrs commented Sep 7, 2020

Coming back to this issue because I need it fixed before I can make some desired changes to the config system.

@eulertour said

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.

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 Scene.tear_down method?

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).

@huguesdevimeux
Copy link
Member Author

@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.

@eulertour
Copy link
Member

@huguesdevimeux Why don't you think that's good? 16 frames at 15 fps is 1 second of video.

@huguesdevimeux
Copy link
Member Author

huguesdevimeux commented Sep 7, 2020

@eulertour

I mean, for two reasons :

  • because one would expect 15 frames during one second if the fps setting is 15
  • and this is not linear ((fps*duration) + 1) which would mislead people even more and especially when one wants a precise number of frames.

But maybe I'm nitpicking.

@eulertour
Copy link
Member

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.

@huguesdevimeux
Copy link
Member Author

I'm not sure to follow you.

Are you saying that 16 frames for 1 sec at 15 FPS is correct ? Why ?

@eulertour
Copy link
Member

Only the time between frames is counted toward the animation, so there is no additional 1/15 seconds added by the last frame.
For example if you wanted to show an animation at 1 fps for 1 second you'd have to generate a start and end frame, otherwise you wouldn't have any video at all.

@huguesdevimeux
Copy link
Member Author

This is where I disagree. I don't think the additional time at the end of the video is the problem.
I disagree with that :

For example if you wanted to show an animation at 1 fps for 1 second you'd have to generate a start and end frame, otherwise you wouldn't have any video at all

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.
Otherwise the frame-rate would be 2fps.
I think the only way to do what you're proposing is to "replace" the original end frame to the one with t = 1.0. Like this, we would keep the right number of frames. BUT it would mean that the video would go from a frame at t = 0.8.67777 to t = 1.0 without passing via one at t = 0.93333 (this one has been replaced) which ... is not good as well.

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)

@leotrs
Copy link
Contributor

leotrs commented Sep 7, 2020

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?

@bcl1713
Copy link

bcl1713 commented Sep 7, 2020 via email

@huguesdevimeux
Copy link
Member Author

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.

Very interesting. Then okay, let's do that.

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?

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

@leotrs
Copy link
Contributor

leotrs commented Sep 7, 2020

OK so the fix is to

  1. partition the t in an interval such that each frame will be shown on screen for a period of time exactly 1/fps. So if the fps is 15, each frame should be on screen for 1/15 of a second,
  2. EXCEPT for the final frame, which will just be shown at the end for just an instant before the video ends.

I think the first one is easy. Can we do the second one by using Scene.tear_down() or something similar? Also, we have to make sure that consecutive animations will chain correctly.

@huguesdevimeux
Copy link
Member Author

partition the t in an interval such that each frame will be shown on screen for a period of time exactly 1/fps. So if the fps is 15, each frame should be on screen for 1/15 of a second,

Wait, I think this is a bad idea, as @eulertour pointed out

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.

@eulertour
Copy link
Member

@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.

@huguesdevimeux
Copy link
Member Author

huguesdevimeux commented Sep 8, 2020

Yes, it was the wrong quote lol.
My concern was that :

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.

Sorry if I miss something, but if each frame is shown excaltly 1/fps, we will have this problem, no ?

@eulertour
Copy link
Member

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.

@huguesdevimeux
Copy link
Member Author

huguesdevimeux commented Sep 8, 2020

But, this is not what @leotrs said. I mean, currently, each frame as for t value 1/(fps - 1), so the frame with t=1.0 is never reached. So, at the animation n+1, frame at t=0.0 is unique.
If the t partition would be such that t = 1/fps, then there would be a t=1.0 frame for animation n, and a t=0.0 frame for the n+1 animation, no?

EDIT : just to be clear, I'm only talking about t values. duration of frames are processed by ffmpeg directly, so we don't have control on that.

@eulertour
Copy link
Member

@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.

@huguesdevimeux
Copy link
Member Author

Alright then, I might have misunderstood.

Anyway; we will discuss this deeper when a PR will be open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue:bug Something isn't working... For use in issues
Projects
Status: 🆕 New
6 participants