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

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

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 19 additions & 7 deletions manim/renderer/cairo_renderer.py
Expand Up @@ -37,10 +37,11 @@ def handle_play_like_call(func):
"""

def wrapper(self, scene, *args, **kwargs):
self.file_writer.begin_animation(not self.skip_animations)
func(self, scene, *args, **kwargs)
self.file_writer.end_animation(not self.skip_animations)
allow_write = not self.skip_animations
self.file_writer.end_animation(allow_write)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close any open pipes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion : Could you add ao comment saying this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you are asking here 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.num_plays += 1
self.file_writer.begin_animation(allow_write)
func(self, scene, *args, **kwargs)

return wrapper

Expand Down Expand Up @@ -74,7 +75,7 @@ def __init__(self, camera_class=None, skip_animations=False, **kwargs):
self.original_skipping_status = skip_animations
self.skip_animations = skip_animations
self.animations_hashes = []
self.num_plays = 0
self.num_plays = -1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you do this and then change the comparison in update_skipping_status()?

Copy link
Contributor Author

@leotrs leotrs Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the first time that handle_play_like_call is called (i.e. the first time an animation is played), step 1 of my explanation in the OP will try to close any open pipes and increment the num_plays counter.

So, I could implement a bunch of logic in multiple places to avoid incrementing the counter that first time only. OR I can just start the count at -1 😅 Accordingly, update_skipping_status also needs to change because it's called before the increment happens so it is now off-by-one.

For the record, I think this is hacky af and I hate it. But also I'm getting tired of this PR 👀 and I think this can maybe be merged now since it doesn't break any other tests and we can deal with the nasty details later?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why increment the counter after the pipe is closed? Couldn't you just do it at the end of play()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On paper, that sounds like a good idea. I just tried it, but it's just bugging out 🤷

If anyone can figure out a way of implementing @eulertour 's suggestion, please go ahead.

Otherwise, I'm going to add some tests to this and call it.

self.time = 0

def init(self, scene):
Expand All @@ -93,6 +94,7 @@ def play(self, scene, *args, **kwargs):
def update_frame( # TODO Description in Docstring
self,
scene,
dt,
mobjects=None,
background=None,
include_submobjects=True,
Expand Down Expand Up @@ -123,6 +125,8 @@ def update_frame( # TODO Description in Docstring
scene.mobjects,
scene.foreground_mobjects,
)
scene.update_mobjects(dt, mobjects)

if background is not None:
self.camera.set_frame_to_background(background)
else:
Expand Down Expand Up @@ -178,16 +182,24 @@ def update_skipping_status(self):
raises an EndSceneEarlyException if they don't correspond.
"""
if config["from_animation_number"]:
if self.num_plays < config["from_animation_number"]:
if self.num_plays < config["from_animation_number"] - 1:
self.skip_animations = True
if config["upto_animation_number"]:
if self.num_plays > config["upto_animation_number"]:
if self.num_plays > config["upto_animation_number"] - 1:
self.skip_animations = True
raise EndSceneEarlyException()

def finish(self, scene):
self.skip_animations = self.original_skipping_status

# if there was an animation being played, render the last frame
if hasattr(self.file_writer, "writing_process"):
self.update_frame(scene, self.camera.frame_rate, ignore_skipping=False)
self.add_frame(self.camera.pixel_array)

self.file_writer.end_animation(not self.skip_animations)
self.num_plays += 1

self.file_writer.finish()
if config["save_last_frame"]:
self.update_frame(scene, ignore_skipping=False)
self.file_writer.save_final_image(self.camera.get_image())
19 changes: 11 additions & 8 deletions manim/scene/scene.py
Expand Up @@ -141,16 +141,17 @@ def get_attrs(self, *keys):
"""
return [getattr(self, key) for key in keys]

def update_mobjects(self, dt):
"""
Begins updating all mobjects in the Scene.
def update_mobjects(self, dt, mobjects=None):
"""Update all mobjects in the Scene.

Parameters
----------
dt: int or float
Change in time between updates. Defaults (mostly) to 1/frames_per_second
"""
for mobject in self.mobjects:
if mobjects is None:
mobjects = self.mobjects
for mobject in mobjects:
mobject.update(dt)

def should_update_mobjects(self):
Expand Down Expand Up @@ -809,7 +810,9 @@ def play_internal(self, *args, **kwargs):
moving_mobjects,
stationary_mobjects,
) = self.get_moving_and_stationary_mobjects(animations)
self.renderer.update_frame(self, mobjects=stationary_mobjects)
self.renderer.update_frame(
self, self.renderer.camera.frame_rate, mobjects=stationary_mobjects
)
self.static_image = self.renderer.get_frame()
time_progression = self.get_animation_time_progression(animations)

Expand All @@ -821,8 +824,8 @@ def play_internal(self, *args, **kwargs):
animation.update_mobjects(dt)
alpha = t / animation.run_time
animation.interpolate(alpha)
self.update_mobjects(dt)
self.renderer.update_frame(self, moving_mobjects, self.static_image)

self.renderer.update_frame(self, dt, moving_mobjects, self.static_image)
self.renderer.add_frame(self.renderer.get_frame())
if stop_condition is not None and stop_condition():
time_progression.close()
Expand All @@ -833,7 +836,7 @@ def play_internal(self, *args, **kwargs):
animation.clean_up_from_scene(self)

def add_static_frames(self, duration):
self.renderer.update_frame(self)
self.renderer.update_frame(self, self.renderer.camera.frame_rate)
dt = 1 / self.renderer.camera.frame_rate
self.renderer.add_frame(
self.renderer.get_frame(),
Expand Down
8 changes: 6 additions & 2 deletions manim/scene/scene_file_writer.py
Expand Up @@ -121,8 +121,10 @@ def add_partial_movie_file(self, hash_animation):
if not hasattr(self, "partial_movie_directory"):
return

# None has to be added to partial_movie_files to keep the right index with scene.num_plays.
# i.e if an animation is skipped, scene.num_plays is still incremented and we add an element to partial_movie_file be even with num_plays.
# None has to be added to partial_movie_files to keep the right index
# with scene.num_plays. i.e if an animation is skipped,
# scene.num_plays is still incremented and we add an element to
# partial_movie_file be even with num_plays.
if hash_animation is None:
self.partial_movie_files.append(None)
return
Expand Down Expand Up @@ -383,6 +385,8 @@ def close_movie_pipe(self):
input buffer, and move the temporary files into their permananant
locations
"""
if not hasattr(self, "writing_process"):
return
self.writing_process.stdin.close()
self.writing_process.wait()
shutil.move(
Expand Down
6 changes: 4 additions & 2 deletions manim/utils/caching.py
Expand Up @@ -23,11 +23,13 @@ def wrapper(self, scene, *args, **kwargs):
self.update_skipping_status()
animations = scene.compile_play_args_to_animation_list(*args, **kwargs)
scene.add_mobjects_from_animations(animations)

if self.skip_animations:
logger.debug(f"Skipping animation {self.num_plays}")
func(self, scene, *args, **kwargs)
# If the animation is skipped, we mark its hash as None.
# When sceneFileWriter will start combining partial movie files, it won't take into account None hashes.
# If the animation is skipped, we mark its hash as None. When
# Scenefilewriter starts combining partial movie files, it won't take into
# account None hashes.
self.animations_hashes.append(None)
self.file_writer.add_partial_movie_file(None)
return
Expand Down
Binary file not shown.
Expand Up @@ -5,7 +5,7 @@
"width": 1920,
"height": 1080,
"avg_frame_rate": "60/1",
"duration": "4.000000",
"nb_frames": "240"
"duration": "4.016667",
"nb_frames": "241"
}
}
}
Expand Up @@ -5,7 +5,7 @@
"width": 854,
"height": 480,
"avg_frame_rate": "15/1",
"duration": "7.000000",
"nb_frames": "105"
"duration": "7.066667",
"nb_frames": "106"
}
}
}
Expand Up @@ -5,7 +5,7 @@
"width": 854,
"height": 480,
"avg_frame_rate": "15/1",
"duration": "5.000000",
"nb_frames": "75"
"duration": "5.066667",
"nb_frames": "76"
}
}
}
Expand Up @@ -5,7 +5,7 @@
"width": 1920,
"height": 1080,
"avg_frame_rate": "60/1",
"duration": "1.000000",
"nb_frames": "60"
"duration": "1.016667",
"nb_frames": "61"
}
}
}
6 changes: 3 additions & 3 deletions tests/control_data/videos_data/SquareToCircleWithlFlag.json
Expand Up @@ -5,7 +5,7 @@
"width": 854,
"height": 480,
"avg_frame_rate": "15/1",
"duration": "1.000000",
"nb_frames": "15"
"duration": "1.066667",
"nb_frames": "16"
}
}
}
6 changes: 3 additions & 3 deletions tests/test_color.py
Expand Up @@ -13,14 +13,14 @@ def test_import_color():
def test_background_color():
S = Scene()
S.camera.background_color = "#ff0000"
S.renderer.update_frame(S)
S.renderer.update_frame(S, 0)
assert np.all(S.renderer.get_frame()[0, 0] == np.array([255, 0, 0, 255]))

S.camera.background_color = "#436f80"
S.renderer.update_frame(S)
S.renderer.update_frame(S, 0)
assert np.all(S.renderer.get_frame()[0, 0] == np.array([67, 111, 128, 255]))

S.camera.background_color = "#bbffbb"
S.camera.background_opacity = 0.5
S.renderer.update_frame(S)
S.renderer.update_frame(S, 0)
assert np.all(S.renderer.get_frame()[0, 0] == np.array([187, 255, 187, 127]))
36 changes: 36 additions & 0 deletions tests/test_graphical_units/test_last_frame.py
@@ -0,0 +1,36 @@
import pytest

import manim as mn
from manim import color as C
from ..utils.testing_utils import get_scenes_to_test
from ..utils.GraphicalUnitTester import GraphicalUnitTester


class LastFrame(mn.Scene):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def construct(self):
tick_start = 1.0
tick_end = 3.0
val_tracker = mn.ValueTracker(tick_start)
square = mn.Square(fill_opacity=1).set_stroke(width=0)
self.add(square)
num_colors = 1000
cols = mn.color_gradient([C.RED, C.WHITE, C.BLUE], num_colors)

def col_uptater(mob):
integ = int(
(val_tracker.get_value() - tick_start)
/ (tick_end - tick_start)
* (num_colors - 1)
)
mob.set_color(cols[integ])

square.add_updater(col_uptater)
self.play(val_tracker.set_value, tick_end, rate_func=mn.linear, run_time=3)


MODULE_NAME = "last_frame"


@pytest.mark.parametrize("scene_to_test", get_scenes_to_test(__name__), indirect=False)
def test_scene(scene_to_test, tmpdir, show_diff):
GraphicalUnitTester(scene_to_test[1], MODULE_NAME, tmpdir).test(show_diff=show_diff)
4 changes: 2 additions & 2 deletions tests/test_scene_rendering/test_caching_related.py
Expand Up @@ -19,7 +19,7 @@ def test_wait_skip(tmp_path, manim_cfg_file, simple_scenes_path):
"manim",
simple_scenes_path,
scene_name,
"-l",
"-ql",
"--media_dir",
str(tmp_path),
"-n",
Expand All @@ -42,7 +42,7 @@ def test_play_skip(tmp_path, manim_cfg_file, simple_scenes_path):
"manim",
simple_scenes_path,
scene_name,
"-l",
"-ql",
"--media_dir",
str(tmp_path),
"-n",
Expand Down
12 changes: 11 additions & 1 deletion tests/utils/video_tester.py
Expand Up @@ -3,6 +3,8 @@
from functools import wraps
import os

import numpy as np

from ..utils.commands import capture


Expand All @@ -28,14 +30,22 @@ def _load_video_data(path_to_data):
return json.load(open(path_to_data, "r"))


def _keys_are_different(v1, v2):
try:
v1, v2 = float(v1), float(v2)
return not np.isclose(v1, v2, atol=1e-3, rtol=1e-3)
except ValueError:
return v1 != v2


def _check_video_data(path_control_data, path_to_video_generated):
control_data = _load_video_data(path_control_data)
config_generated = _get_config_from_video(path_to_video_generated)
config_expected = control_data["config"]
diff_keys = [
d1[0]
for d1, d2 in zip(config_expected.items(), config_generated.items())
if d1[1] != d2[1]
if _keys_are_different(d1[1], d2[1])
]
# \n does not work in f-strings.
newline = "\n"
Expand Down