Fix 183: "t values issue that causes the animations to not be finished entirely" #698

leotrs · 2020-11-11T15:37:58Z

Motivation

Manim doesn't render animations completely. For a video rendering library, this should be priority 1 right now. See #183 for @huguesdevimeux's excellent report. In short, the animation

class MyScene(Scene):
    def construct(self):
        self.play(ShowCreation(Square()))

generates a video whose last frame is this:

You can see that the square has not been rendered fully, there is still a bit of the animation left to play (see open top left corner).

After discussion in #183, the way to solve this seems to be to simply add the last frame manually at the end of the video. With the mess that is scene file writer, scene, renderer, caching, etc, it was not obvious to me how to do this correctly. Luckily, thanks to all the good work put it by the dev team recently, I was finally able to figure it out, I think.

Explanation for Changes

The way things currently work:

Each call to play opens a new FFMPEG pipe.
Each frame is fed into the pipe.
When animation ends, the pipe is closed.

The way I'm proposing we do things:

Each call to play closes a previous FFMPEG pipe, if one exists.
Open a new pipe.
Feed each frame into the new pipe.
Leave the pipe open.

After all frames are fed, one of two things can happen: (a) play is called with a new animation (in which case it would close the currently open pipe in step 1.), or (b) the video ends. If the video ends, Renderer.finish() is called, which now takes care of writing the last frame (this is the bug fix! 🎉) and closing the open pipe, so no pipes are ever left unattended. This is basically what you see in the diff in renderer/cairo_renderer.py.

There is only one sticking point: the first call to play will try to close an (unexisting) pipe, so I added a quick check for that in scene/scene_file_writer.py

Testing Status

The tests currently do not pass, because this fix will change some tests. However before further work I wanted to hear what everyone thinks. Mainly, this bug will make it so that a manim script with a single 1s animation will render a video that is longer than 1s. I believe this should be fixable by tweaking the last call to ffmpeg somehow, but I'm not sure how.

Further comments

I believe #618 should be addressed only after merging this (or else fixing #183)

This is a pretty big change and it will break user space so thoughts appreciated @ManimCommunity/core

leotrs · 2020-11-11T15:41:10Z

manim/renderer/cairo_renderer.py

@@ -38,9 +38,9 @@ def handle_play_like_call(func):

    def wrapper(self, scene, *args, **kwargs):
        allow_write = not config["skip_animations"]
+        self.file_writer.end_animation(allow_write)


close any open pipes

leotrs · 2020-11-11T15:41:24Z

manim/renderer/cairo_renderer.py

+        self.update_frame(scene, ignore_skipping=False)
+        self.add_frame(self.camera.pixel_array)
+        self.file_writer.end_animation(not config["skip_animations"])


add the last frame - This fixes the bug!

huguesdevimeux

As discussed in #183, we took the decision to add an extra frame at the end of the video, so it seems good to me.

However, Could you :

Maybe add a comment saying that it adds an additional frame to the video (because, it's not very intuitive that 1seconde at 15fps will be .. 16 frames)
And a video test (videos test compare meta data of the videos, including the number of frames).

EDIT : I didn't see, but tests are already failing because of this
EDIT2 : I didn't read neither you PR text, so I basically repeated eveything you said there sorry

huguesdevimeux · 2020-11-11T19:29:21Z

manim/renderer/cairo_renderer.py

@@ -38,9 +38,9 @@ def handle_play_like_call(func):

    def wrapper(self, scene, *args, **kwargs):
        allow_write = not config["skip_animations"]
+        self.file_writer.end_animation(allow_write)


Suggestion : Could you add ao comment saying this?

I don't understand what you are asking here 🤔

I think @huguesdevimeux means to add a comment saying "Close any open pipes"

leotrs · 2020-11-12T15:13:59Z

Maybe add a comment saying that it adds an additional frame to the video (because, it's not very intuitive that 1seconde at 15fps will be .. 16 frames)

I still intend to make the output video be 1s long, but it will still be 16 frames. I personally don't have a problem with that, I think the off-by-one error here is fine (and may in fact be expected).

And a video test (videos test compare meta data of the videos, including the number of frames).

I will add tests once more people have taken a look and agreed that this PR is in the right path.

behackl · 2020-11-13T01:31:33Z

Maybe add a comment saying that it adds an additional frame to the video (because, it's not very intuitive that 1seconde at 15fps will be .. 16 frames)

I still intend to make the output video be 1s long, but it will still be 16 frames. I personally don't have a problem with that, I think the off-by-one error here is fine (and may in fact be expected).

I agree with this approach. It makes sense to partition the time interval of one second into a series of half-open intervals [0, 1/30) + [1/30, 2/30) + ... + [29/30, 30/30) each showing one frame in order to produce a 30fps video, as well as an additional final frame in the end (which "closes" the interval, so to say).

The docbuild failed with AttributeError: 'SceneFileWriter' object has no attribute 'writing_process', though -- it seems there need to be some further changes?

eulertour · 2020-11-14T01:22:39Z

I agree with the approach too, it'll just require some tweaking to make the tests pass.

kolibril13 · 2020-11-14T08:04:29Z

I also agree with this additional frame strategy. Nice that you found a way to solve this!

kolibril13 · 2020-11-14T08:12:50Z

Just tested this script:

class VideoTest(Scene):
    def construct(self):
        def func1(num):
            text =Text(f"{num}",font='Arial').scale(3)
            text.num= num
            return text
        text= func1(0)
        def update_curve(d,dt):
            d.num = d.num+1
            d.become(func1(d.num))
        self.add(text)
        text.add_updater(update_curve)
        self.play(ShowCreation(Square()))

output from master:

(not closing)
And output from this branch:

(properly closing)
Buut note that the updater Text in branch #698 is not changing anymore from the pre-last to the last frame:

kolibril13 · 2020-11-14T09:56:26Z

And regarding my previous post:
This might be a completly different bug, but the frame counting starts at 2 and not at zero as expected.

leotrs · 2020-11-14T16:44:33Z

Great find @kolibril13 this is exactly why I didn't want to implement the tests yet.

Let's take this script:

import manim as mn

mn.config.frame_rate = 5
mn.config.pixel_height = 150
mn.config.pixel_width = 150


def make_lbl(num):
    text = mn.Text(f"{num}").scale(7)
    text.num = num
    return text


def update_lbl(lbl, dt):
    lbl.num += 1
    lbl.become(make_lbl(lbl.num))


class MyScene(mn.Scene):
    def construct(self):
        text = make_lbl(0).add_updater(update_lbl)
        self.add(text)
        sq = mn.Square(stroke_width=30).scale(3)
        self.play(mn.ShowCreation(sq), run_time=1)

(I think the syntax for the updaters is actually kind of terrible - it seems it should be a lot easier to do this. But anyhow...)

on master this produces the following five frames:

and on this PR branch:

Notes

I think both are wrong when it comes to the value of the updater. It obviously should not start at 2. Whether it should start at 0 or 1 is up for discussion I think. However, this seems to be a problem with updaters, not with the issue at hand.
This branch correctly finishes the ShowCreation animation but does not update the value of the updater. I think this is a problem with how I have implemented it.

leotrs · 2020-11-14T17:31:21Z

Update: with the latest commit, now the output of the previous script is the following. Now the last frame has the correct updaters, but now everything starts at 3 😂 I need help figuring out why!

leotrs · 2020-11-14T22:43:45Z

@kolibril13 the problem of starting at the number 2 will be fixed by #710

leotrs · 2020-11-14T22:43:50Z

Depends on #710

…hen an animation is being played

leotrs · 2020-11-14T23:40:34Z

Ok, after #710, there was one further bug to squash. Basically, scene.update_mobjects was being called from scene.play_internal. This makes sense because you obviously want to update the mobjects whenever you are playing an animation. However, if one manually calls renderer.update_frame, the mobjects were not being updated at all.

So I moved the call to scene.update_mobjects from scene.play_internal to renderer.update_frame, and now this works. See the current output of the script above:

eulertour · 2020-11-15T02:14:04Z

if one manually calls renderer.update_frame

what

leotrs · 2020-11-15T02:17:34Z

if one manually calls renderer.update_frame

what

I mean if any part of the codebase (other than Scene.play_internal) calls renderer.update_frame(), then the Mobjects won't be updated. But sometimes we do want to call update_frame from outside play_internal, for example for the fix of the present bug we want to call update_frame from renderer.finish.

leotrs · 2020-11-15T18:41:03Z

Update: #710 is merged so I just need to fix the tests (this will involve changing some pre-existing tests) and adding new ones.

kolibril13 · 2020-11-17T11:28:12Z

Is there yet a way to re-generate all test data from the current reference scripts?
If not, I think it would be really nice to have a script in manim/tests that does that.
As there is already template_generate_graphical_units_data.py, maybe we can add as well template_regenerate_all_test_data_from_current_branch.py

PhilippImhof · 2020-11-22T09:10:11Z

Here you are for Mac OS Catalina and Python 3.8.6

From current master:

From this branch (pr/698):

leotrs · 2020-11-22T16:16:09Z

What? Now I'm confused.

naveen521kk · 2020-11-22T16:23:16Z

Maybe it's a linux problem, because both Windows and Mac seem to work the same way?

leotrs · 2020-11-22T16:27:57Z

Ah no no no, I fell in the whole problem of this issue 😅 @naveen521kk and @PhilippImhof could you pretty please send me the output of the same script but not the last frame generated with -s! Instead, please run it with --save_pngs and send me all of the frames 🙂 (you may want to remove your media/ dir first and disable the cache so it doesn't get confused with the previous run of the same script...)

PhilippImhof · 2020-11-22T21:04:01Z

Here you go. First the pictures from current master:

And now the ones from this PR:

Again, this is on Mac OS Catalina, Python 3.8.6 and invoked as manim leotrs-test.py --save_pngs --disable_caching

leotrs · 2020-11-22T23:17:27Z

Thank you @PhilippImhof

I believe this is related to the fact that macos and windows added the last frame with a different duration than linux did. I do not like where this is going.

… works

naveen521kk · 2020-11-23T03:39:18Z

From this one

From master

On python 3.7.9, windows.

leotrs · 2020-11-23T13:49:10Z

Thanks all. I now believe the problem is how the numbers themselves are being rendered. See e.g. this comment.

I think I'm just going to change the test to do the same thing but say with colors instead of Text. I'll get to it when I have time.

leotrs · 2020-11-24T01:16:01Z

Solved! Ready for final review.

huguesdevimeux · 2020-11-24T13:09:51Z

tests/test_graphical_units/test_last_frame.py

-def update_lbl(lbl, dt):
-    lbl.num += 1
-    lbl.become(make_lbl(lbl.num))
-

 class LastFrame(mn.Scene):


Comment/issue : I'm worry that it won't test anything, as graphical unit tests are run with the equivalent of -s flag ( every animation is skipped)
therfore, the last t value is always the run_time.

I'm not sure how this tests the change either...

I was going through older issues/PRs again and recall @kolibril13 mentioning a way to test video which is VERY applicable here as a way to test this PR. #1035

I recommend we test in this manner.

for the record, in #1019 I Implemented such tests on t values, so no need to implement this here.

jsonvillanueva

As @leotrs likely doesn't have time to update this PR, I could take over (and also look into @eulertour's suggestion). Would anyone mind if I commit directly onto this branch and bring it up-to-date with current master?

jsonvillanueva · 2021-03-08T17:32:25Z

manim/renderer/cairo_renderer.py

@@ -38,9 +38,9 @@ def handle_play_like_call(func):

    def wrapper(self, scene, *args, **kwargs):
        allow_write = not config["skip_animations"]
+        self.file_writer.end_animation(allow_write)


I think @huguesdevimeux means to add a comment saying "Close any open pipes"

huguesdevimeux · 2021-03-09T12:31:34Z

I think it will cause huge conflicts with #1019 . (I'm the author of #1019, so I'm biaised).

I think it makes more sense to merge #1019 before this, since #1019 is mostly clean up PR that add tests, and then to come back later to this. (Not because I don't want to fix merge conflicts on my PR, really not, this is a rational thought).

jsonvillanueva · 2021-03-09T13:43:22Z

I think it makes more sense to merge #1019 before this, since #1019 is mostly clean up PR that add tests, and then to come back later to this. (Not because I don't want to fix merge conflicts on my PR, really not, this is a rational thought).

Makes sense, I'll hold off on any such work on this PR until then and checkout/review that #1019. Once merged though, are there any other objections to moving this along? I suppose I could start a separate PR entirely.

leotrs · 2021-03-09T15:09:57Z

Please do.

…

On Tue, Mar 9, 2021, 6:42 AM Jason Villanueva ***@***.***> wrote: ***@***.**** commented on this pull request. As @leotrs <https://github.com/leotrs> likely doesn't have time to update this PR, I could take over (and also look into @eulertour <https://github.com/eulertour>'s suggestion <https://github.com/ManimCommunity/manim/pull/698/files#r528143200>). Would anyone mind if I commit directly onto this branch and bring it up-to-date with current master? ------------------------------ In manim/renderer/cairo_renderer.py <#698 (comment)>: > @@ -38,9 +38,9 @@ def handle_play_like_call(func): def wrapper(self, scene, *args, **kwargs): allow_write = not config["skip_animations"] + self.file_writer.end_animation(allow_write) I think @huguesdevimeux <https://github.com/huguesdevimeux> means to add a comment saying "Close any open pipes" — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#698 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAILYAALCXYGG2JMUX64LEDTCX3SXANCNFSM4TSEKU6A> .

GameDungeon · 2021-03-28T03:51:54Z

@leotrs Could you fix the merge issues now that #1019 has been merged?

jsonvillanueva · 2021-03-31T11:43:36Z

@leotrs Could you fix the merge issues now that #1019 has been merged?

Leo is on paternity leave, but I'll attempt to handle conflicts now

Darylgolden · 2022-02-16T01:42:08Z

Converting to draft since it has merge conflicts and due to inactivity.

MrDiver · 2022-06-18T17:51:36Z

I would close this for now and we can take a fresh look at this on the new version and see what we are able to fix with the knowledge we aquired from this PR. I linked it to the original issue such that we do not loose this information.

t values issue that causes the animations to not be finished entirely. #183
Seems to be still an issue on the current version of the main branch.

first attempt

d831aa6

leotrs commented Nov 11, 2020

View reviewed changes

huguesdevimeux requested changes Nov 11, 2020

View reviewed changes

huguesdevimeux marked this pull request as draft November 11, 2020 20:58

huguesdevimeux added pr:bugfix Bug fix for use in PRs solving a specific issue:bug test requested Implementation of tests are requested labels Nov 11, 2020

update all mobjects each time we update the frame

02fe66c

leotrs added 3 commits November 14, 2020 18:36

add changes from ManimCommunity#710

a1a7fa5

mobjects should be updated each time the frame is updated, not only w…

116429b

…hen an animation is being played

use the camera frame rate, not the config frame rate

15f033a

black

1d0858b

behackl added the pr:dependent This PR or issue requires that another PR is merged first label Nov 15, 2020

leotrs removed the pr:dependent This PR or issue requires that another PR is merged first label Nov 16, 2020

Merge branch 'master' of github.com:ManimCommunity/manim

bfe7f4f

add useless lines - these are overwritten by how GraphicalUnitsTester…

0876e31

… works

leotrs added 2 commits November 23, 2020 19:02

change test to use colors, not Text

b9d8f46

black

0e06786

leotrs requested review from eulertour and huguesdevimeux November 24, 2020 01:16

leotrs marked this pull request as ready for review November 24, 2020 01:16

leotrs requested a review from behackl November 24, 2020 01:16

huguesdevimeux reviewed Nov 24, 2020

View reviewed changes

jsonvillanueva reviewed Mar 9, 2021

View reviewed changes

huguesdevimeux mentioned this pull request Sep 1, 2021

Fix last frame of the video not rendered #1997

Closed

3 tasks

leleogere mentioned this pull request Sep 3, 2021

Allow to suspend and resume updating smoothly #1873

Closed

3 tasks

Darylgolden marked this pull request as draft February 16, 2022 01:42

MrDiver linked an issue Jun 18, 2022 that may be closed by this pull request

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

Open

MrDiver closed this Jun 18, 2022

Fix 183: "t values issue that causes the animations to not be finished entirely" #698

Fix 183: "t values issue that causes the animations to not be finished entirely" #698

Conversation

leotrs commented Nov 11, 2020 • edited

Motivation

Explanation for Changes

Testing Status

Further comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huguesdevimeux left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leotrs commented Nov 12, 2020

behackl commented Nov 13, 2020

eulertour commented Nov 14, 2020

kolibril13 commented Nov 14, 2020

kolibril13 commented Nov 14, 2020 • edited

kolibril13 commented Nov 14, 2020

leotrs commented Nov 14, 2020 • edited

on master this produces the following five frames:

and on this PR branch:

Notes

leotrs commented Nov 14, 2020

leotrs commented Nov 14, 2020

leotrs commented Nov 14, 2020

leotrs commented Nov 14, 2020

eulertour commented Nov 15, 2020

leotrs commented Nov 15, 2020 • edited

leotrs commented Nov 15, 2020

kolibril13 commented Nov 17, 2020

PhilippImhof commented Nov 22, 2020

leotrs commented Nov 22, 2020

naveen521kk commented Nov 22, 2020

leotrs commented Nov 22, 2020

PhilippImhof commented Nov 22, 2020 • edited

leotrs commented Nov 22, 2020

naveen521kk commented Nov 23, 2020

leotrs commented Nov 23, 2020

leotrs commented Nov 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsonvillanueva Feb 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsonvillanueva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huguesdevimeux commented Mar 9, 2021 • edited by kilacoda-old

jsonvillanueva commented Mar 9, 2021

leotrs commented Mar 9, 2021 via email

GameDungeon commented Mar 28, 2021

jsonvillanueva commented Mar 31, 2021

Darylgolden commented Feb 16, 2022

MrDiver commented Jun 18, 2022

leotrs commented Nov 11, 2020 •

edited

huguesdevimeux left a comment •

edited

kolibril13 commented Nov 14, 2020 •

edited

leotrs commented Nov 14, 2020 •

edited

leotrs commented Nov 15, 2020 •

edited

PhilippImhof commented Nov 22, 2020 •

edited

jsonvillanueva Feb 28, 2021 •

edited

huguesdevimeux commented Mar 9, 2021 •

edited by kilacoda-old