FIX : 472 / Small refactor of partial-movie-files handling (#489)

* fix #460 by enabling skipping wait statements.

* added tests

* refactored partialmovie files handling

* improved berobosity of hashing

* fixed tests

* fixed black

* fixed file name

* change format to f string

* fixed comment

* fixed comment

* Update manim/scene/scene.py

Co-authored-by: Benjamin Hackl <devel@benjamin-hackl.at>

* fix test output (caused by 3fff83e)

Co-authored-by: Hugues Devimeux <hugues.devimeux@gmail.com>
Co-authored-by: Benjamin Hackl <devel@benjamin-hackl.at>

Author: 3 people

manim/scene/scene.py

@@ -103,7 +103,7 @@ def __init__(self, **kwargs):
                 self,
                 **file_writer_config,
             )
-        self.play_hashes_list = []
+        self.animations_hashes = []
 
         self.mobjects = []
         self.original_skipping_status = file_writer_config["skip_animations"]
@@ -852,13 +852,16 @@ def wrapper(self, *args, **kwargs):
             if file_writer_config["skip_animations"]:
                 logger.debug(f"Skipping animation {self.num_plays}")
                 func(self, *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.
+                self.animations_hashes.append(None)
+                self.file_writer.add_partial_movie_file(None)
                 return
             if not file_writer_config["disable_caching"]:
                 mobjects_on_scene = self.get_mobjects()
                 hash_play = get_hash_from_play_call(
                     self, self.camera, animations, mobjects_on_scene
                 )
-                self.play_hashes_list.append(hash_play)
                 if self.file_writer.is_already_cached(hash_play):
                     logger.info(
                         f"Animation {self.num_plays} : Using cached data (hash : %(hash_play)s)",
@@ -867,7 +870,12 @@ def wrapper(self, *args, **kwargs):
                     file_writer_config["skip_animations"] = True
             else:
                 hash_play = "uncached_{:05}".format(self.num_plays)
-                self.play_hashes_list.append(hash_play)
+            self.animations_hashes.append(hash_play)
+            self.file_writer.add_partial_movie_file(hash_play)
+            logger.debug(
+                "List of the first few animation hashes of the scene: %(h)s",
+                {"h": str(self.animations_hashes[:5])},
+            )
             func(self, *args, **kwargs)
 
         return wrapper
@@ -890,20 +898,28 @@ def wrapper(self, duration=DEFAULT_WAIT_TIME, stop_condition=None):
             if file_writer_config["skip_animations"]:
                 logger.debug(f"Skipping wait {self.num_plays}")
                 func(self, duration, stop_condition)
+                # 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.
+                self.animations_hashes.append(None)
+                self.file_writer.add_partial_movie_file(None)
                 return
             if not file_writer_config["disable_caching"]:
                 hash_wait = get_hash_from_wait_call(
                     self, self.camera, duration, stop_condition, self.get_mobjects()
                 )
-                self.play_hashes_list.append(hash_wait)
                 if self.file_writer.is_already_cached(hash_wait):
                     logger.info(
                         f"Wait {self.num_plays} : Using cached data (hash : {hash_wait})"
                     )
                     file_writer_config["skip_animations"] = True
             else:
                 hash_wait = "uncached_{:05}".format(self.num_plays)
-                self.play_hashes_list.append(hash_wait)
+            self.animations_hashes.append(hash_wait)
+            self.file_writer.add_partial_movie_file(hash_wait)
+            logger.debug(
+                "Animations hashes list of the scene : (concatened to 5) %(h)s",
+                {"h": str(self.animations_hashes[:5])},
+            )
             func(self, duration, stop_condition)
 
         return wrapper

manim/scene/scene_file_writer.py

@@ -37,6 +37,9 @@ class SceneFileWriter(object):
             The PIL image mode to use when outputting PNGs
         "movie_file_extension" (str=".mp4")
             The file-type extension of the outputted video.
+        "partial_movie_files"
+            List of all the partial-movie files.
+
     """
 
     def __init__(self, scene, **kwargs):
@@ -46,7 +49,7 @@ def __init__(self, scene, **kwargs):
         self.init_output_directories()
         self.init_audio()
         self.frame_count = 0
-        self.index_partial_movie_file = 0
+        self.partial_movie_files = []
 
     # Output directories and files
     def init_output_directories(self):
@@ -113,6 +116,29 @@ def init_output_directories(self):
                     )
                 )
 
+    def add_partial_movie_file(self, hash_animation):
+        """Adds a new partial movie file path to scene.partial_movie_files from an hash. This method will compute the path from the hash.
+
+        Parameters
+        ----------
+        hash_animation : str
+            Hash of the animation.
+        """
+
+        # 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
+        new_partial_movie_file = os.path.join(
+            self.partial_movie_directory,
+            "{}{}".format(
+                hash_animation,
+                file_writer_config["movie_file_extension"],
+            ),
+        )
+        self.partial_movie_files.append(new_partial_movie_file)
+
     def get_default_module_directory(self):
         """
         This method gets the name of the directory containing
@@ -149,7 +175,7 @@ def get_resolution_directory(self):
         This method gets the name of the directory that immediately contains the
         video file. This name is ``<height_in_pixels_of_video>p<frame_rate>``.
         For example, if you are rendering an 854x480 px animation at 15fps,
-        the name of the directory that immediately contains the video file
+        the name of the directory that immediately contains the video,  file
         will be ``480p15``.
 
         The file structure should look something like::
@@ -186,29 +212,6 @@ def get_image_file_path(self):
         """
         return self.image_file_path
 
-    def get_next_partial_movie_path(self):
-        """
-        Manim renders each play-like call in a short partial
-        video file. All such files are then concatenated with
-        the help of FFMPEG.
-
-        This method returns the path of the next partial movie.
-
-        Returns
-        -------
-        str
-            The path of the next partial movie.
-        """
-        result = os.path.join(
-            self.partial_movie_directory,
-            "{}{}".format(
-                self.scene.play_hashes_list[self.index_partial_movie_file],
-                file_writer_config["movie_file_extension"],
-            ),
-        )
-        self.index_partial_movie_file += 1
-        return result
-
     def get_movie_file_path(self):
         """
         Returns the final path of the written video file.
@@ -399,7 +402,9 @@ def open_movie_pipe(self):
         FFMPEG and begin writing to FFMPEG's input
         buffer.
         """
-        file_path = self.get_next_partial_movie_path()
+        file_path = self.partial_movie_files[self.scene.num_plays]
+
+        # TODO #486 Why does ffmpeg need temp files ?
         temp_file_path = (
             os.path.splitext(file_path)[0]
             + "_temp"
@@ -497,21 +502,20 @@ def combine_movie_files(self):
         # cuts at all the places you might want.  But for viewing
         # the scene as a whole, one of course wants to see it as a
         # single piece.
-        partial_movie_files = [
-            os.path.join(
-                self.partial_movie_directory,
-                "{}{}".format(hash_play, file_writer_config["movie_file_extension"]),
-            )
-            for hash_play in self.scene.play_hashes_list
-        ]
-        if len(partial_movie_files) == 0:
-            logger.error("No animations in this scene")
-            return
+        partial_movie_files = [el for el in self.partial_movie_files if el is not None]
+        # NOTE : Here we should do a check and raise an exeption if partial movie file is empty.
+        # We can't, as a lot of stuff (in particular, in tests) use scene initialization, and this error would be raised as it's just
+        # an empty scene initialized.
+
         # Write a file partial_file_list.txt containing all
         # partial movie files. This is used by FFMPEG.
         file_list = os.path.join(
             self.partial_movie_directory, "partial_movie_file_list.txt"
         )
+        logger.debug(
+            f"Partial movie files to combine ({len(partial_movie_files)} files): %(p)s",
+            {"p": partial_movie_files[:5]},
+        )
         with open(file_list, "w") as fp:
             fp.write("# This file is used internally by FFMPEG.\n")
             for pf_path in partial_movie_files:

manim/utils/hashing.py

@@ -252,9 +252,11 @@ def get_hash_from_play_call(
     ]
     t_end = perf_counter()
     logger.debug("Hashing done in %(time)s s.", {"time": str(t_end - t_start)[:8]})
+    hash_complete = f"{hash_camera}_{hash_animations}_{hash_current_mobjects}"
     # This will reset ALREADY_PROCESSED_ID as all the hashing processus is finished.
     ALREADY_PROCESSED_ID = {}
-    return "{}_{}_{}".format(hash_camera, hash_animations, hash_current_mobjects)
+    logger.debug("Hash generated :  %(h)s", {"h": hash_complete})
+    return hash_complete
 
 
 def get_hash_from_wait_call(
@@ -299,15 +301,15 @@ def get_hash_from_wait_call(
         ALREADY_PROCESSED_ID = {}
         t_end = perf_counter()
         logger.debug("Hashing done in %(time)s s.", {"time": str(t_end - t_start)[:8]})
-        return "{}_{}{}_{}".format(
-            hash_camera,
-            str(wait_time).replace(".", "-"),
-            hash_function,
-            hash_current_mobjects,
-        )
+        hash_complete = f"{hash_camera}_{str(wait_time).replace('.', '-')}{hash_function}_{hash_current_mobjects}"
+        logger.debug("Hash generated :  %(h)s", {"h": hash_complete})
+        return hash_complete
     ALREADY_PROCESSED_ID = {}
     t_end = perf_counter()
     logger.debug("Hashing done in %(time)s s.", {"time": str(t_end - t_start)[:8]})
-    return "{}_{}_{}".format(
-        hash_camera, str(wait_time).replace(".", "-"), hash_current_mobjects
+    hash_complete = (
+        f"{hash_camera}_{str(wait_time).replace('.', '-')}_{hash_current_mobjects}"
     )
+
+    logger.debug("Hash generated :  %(h)s", {"h": hash_complete})
+    return hash_complete

tests/control_data/logs_data/BasicSceneLoggingTest.txt

@@ -1,6 +1,9 @@
 {"levelname": "INFO", "module": "config", "message": "Log file will be saved in <>"}
 {"levelname": "DEBUG", "module": "hashing", "message": "Hashing ..."}
 {"levelname": "DEBUG", "module": "hashing", "message": "Hashing done in <> s."}
+{"levelname": "DEBUG", "module": "hashing", "message": "Hash generated :  <>"}
+{"levelname": "DEBUG", "module": "scene", "message": "List of the first few animation hashes of the scene: <>"}
 {"levelname": "INFO", "module": "scene_file_writer", "message": "Animation 0 : Partial movie file written in <>"}
+{"levelname": "DEBUG", "module": "scene_file_writer", "message": "Partial movie files to combine (1 files): <>"}
 {"levelname": "INFO", "module": "scene_file_writer", "message": "\nFile ready at <>\n"}
 {"levelname": "INFO", "module": "scene", "message": "Rendered SquareToCircle\nPlayed 1 animations"}

tests/test_scene_rendering/test_caching_related.py

@@ -0,0 +1,53 @@
+import os
+import pytest
+import subprocess
+from manim import file_writer_config
+
+from ..utils.commands import capture
+from ..utils.video_tester import *
+
+
+@video_comparison(
+    "SceneWithMultipleWaitCallsWithNFlag.json",
+    "videos/simple_scenes/480p15/SceneWithMultipleWaitCalls.mp4",
+)
+def test_wait_skip(tmp_path, manim_cfg_file, simple_scenes_path):
+    # Test for PR #468. Intended to test if wait calls are correctly skipped.
+    scene_name = "SceneWithMultipleWaitCalls"
+    command = [
+        "python",
+        "-m",
+        "manim",
+        simple_scenes_path,
+        scene_name,
+        "-l",
+        "--media_dir",
+        str(tmp_path),
+        "-n",
+        "3",
+    ]
+    out, err, exit_code = capture(command)
+    assert exit_code == 0, err
+
+
+@video_comparison(
+    "SceneWithMultiplePlayCallsWithNFlag.json",
+    "videos/simple_scenes/480p15/SceneWithMultipleCalls.mp4",
+)
+def test_play_skip(tmp_path, manim_cfg_file, simple_scenes_path):
+    # Intended to test if play calls are correctly skipped.
+    scene_name = "SceneWithMultipleCalls"
+    command = [
+        "python",
+        "-m",
+        "manim",
+        simple_scenes_path,
+        scene_name,
+        "-l",
+        "--media_dir",
+        str(tmp_path),
+        "-n",
+        "3",
+    ]
+    out, err, exit_code = capture(command)
+    assert exit_code == 0, err