Refactored :meth:`~.Scene.play` #1019

huguesdevimeux · 2021-02-10T18:54:02Z

Motivation

Main motivation was to fix #874, and I ended but by refactoring everything.
The main goal was to refactor scene.compile_animation_data. This method did way too much, including starting the animations, determining the run time and the time progression, saving static frame, etc.

Overview / Explanation for Changes

Removed the three decorators of :meth:~.Scene.play, in particular: caching logic and file writer logic are now included within :meth:~.Scene.play (it wasn't possible before, because scene.wait and scene.play were two different things).
Added is_static_wait attributes to Wait. (<=> if wait is a frozen frame).
Renamed and moved scene.add_static_frame to renderer.freeze_current_frame.
Now when calling play without animation, it raises ValueError instead of just a warning.
Fixed :pr:874 by modfying renderer.update_skipping_status
renderer starts the animation with scene.begin_animations (scene.compile_animation_data used to do this)
The run time and the time progression generation is now done in scene.play_internal although it'd make more sense that renderer processes it later.
Added a bunch of cool tests thanks to mocks, and thanks to the new syntax scene.render

Oneline Summary of Changes

- Refactored play logic (:pr:`1019`)

Testing Status

Tests run. Added new ones.

Further Comments

This has probably broken a little of the webgl renderer.
I let scene.compile_animation_data returns None or self in function of the play has some informations (if it's a static wait, it returns non, else it returns .. self). I would like to remove this logic, as it makes not really a sense that a such method returns any information.
cc @eulertour

Acknowledgements

I have read the Contributing Guidelines

Reviewer Checklist

Newly added functions/classes are either private or have a docstring
Newly added functions/classes have tests added and (optional) examples in the docs
Newly added documentation builds, looks correctly formatted, and adds no additional build warnings
The oneline summary has been included in the wiki

behackl · 2021-02-14T11:34:25Z

Tests do not pass for Python 3.7.

huguesdevimeux · 2021-02-24T13:05:15Z

Tests do not pass for Python 3.7.

Fixed now. Thanks!

eulertour

Only small nitpicks, this mostly looks good.

manim/renderer/cairo_renderer.py

…im into refactored-play-logic

huguesdevimeux · 2021-02-27T23:10:56Z

@eulertour So, I was worried that this could break the WebGL renderer. Is this ok?

eulertour · 2021-03-01T00:38:12Z

I'd greatly prefer if it didn't since I'll probably stop working on it for a while after this release. Can you just ensure it doesn't break anything obvious?

huguesdevimeux · 2021-03-05T07:53:22Z

I'd greatly prefer if it didn't since I'll probably stop working on it for a while after this release. Can you just ensure it doesn't break anything obvious?

So, I did that, the only thing I see that could break it is that now the animations are started in begin_animations, not in compile_animations_data anymore.

I didn't test it though because I'm completly unfamiliar with it, but it should be good.

eulertour · 2021-03-07T04:23:26Z

It throws this error in case you can tell why that would happen:

  File "/home/devneal/github/manim-3/manim/grpc/impl/frame_server_impl.py", line 285, in load_scene_module
    self.generate_keyframe_data()

  File "/home/devneal/github/manim-3/manim/grpc/impl/frame_server_impl.py", line 295, in generate_keyframe_data
    self.scene.render()

  File "/home/devneal/github/manim-3/manim/scene/scene.py", line 169, in render
    self.construct()

  File "/home/devneal/github/manim-3/example_scenes/basic.py", line 80, in construct
    self.play(ShowCreation(square))

  File "/home/devneal/github/manim-3/manim/scene/scene.py", line 763, in play
    self.renderer.play(self, *args, **kwargs)

  File "/home/devneal/github/manim-3/manim/renderer/webgl_renderer.py", line 37, in play
    scene.play_internal(skip_rendering=True)

  File "/home/devneal/github/manim-3/manim/scene/scene.py", line 867, in play_internal
    self.update_to_time(t)

  File "/home/devneal/github/manim-3/manim/scene/scene.py", line 885, in update_to_time
    animation.update_mobjects(dt)

  File "/home/devneal/github/manim-3/manim/animation/animation.py", line 124, in update_mobjects
    mob.update(dt)

AttributeError: 'NoneType' object has no attribute 'update'

manim/scene/scene.py

huguesdevimeux · 2021-03-07T17:31:51Z

@eulertour At first sight I've no idea. This didn't in theory touche the mobject processing.
Maybe it's because of this ?

What was the scene ?

EDIT : after testing locally, it seems that the mobject logic is broken.

the mobjects are set to None for some reason.

jsonvillanueva · 2021-03-09T20:27:11Z

And on Windows there's a DirDeletedEvent saying that basic.py is a directory which seems strange...

Edit: It also reports None issues, and I've tried reverting the raised exception from here to no avail.

jsonvillanueva · 2021-03-09T22:02:49Z

After further debugging, it seems the issue is with the attribute .duration. If you change the scene to have Wait() animations (since Wait() sets this attribute, it works as intended). For other types of Animations inheriting from Animation, duration isn't ever set as an attribute -- hence the TypeError.

jsonvillanueva

Some short feedback. The refactor seems fine otherwise.

tests/test_scene_rendering/simple_scenes.py

tests/test_scene_rendering/test_cairo_renderer.py

jsonvillanueva · 2021-03-18T06:40:39Z

tests/test_scene_rendering/test_play_logic.py

+@pytest.mark.skipif(
+    sys.version_info < (3, 8),
+    reason="Mock object has a different implementation in python 3.7, which makes it broken with this logic.",
+)


I'm unfamiliar with Mock, but if it's possible to configure this so that it runs on 3.7 (maybe using sys.version?) then I would prefer to not skip tests

I will see what I can do.

To be honest, I didn't want to spend too much on investigating on this uncompataibilty, givent that this test is logically independent from the python version.

jsonvillanueva · 2021-03-18T06:48:54Z

manim/animation/animation.py

@@ -248,6 +248,7 @@ def __init__(
        self.duration = duration
        self.mobject = None
        self.stop_condition = stop_condition
+        self.is_static_wait = False


This is fine, but I think the name/phrase "static_wait" is misleading since a static variable in programming (and Python more specifically) refers to a variable shared between instances of the class.

Here the name is based on whether the Animation has updaters, so a better name to use here/elsewhere in Scene.py may be updaters, has_updaters, ...

Though I agree with you, I chose this name because it is how this type of wait is called within the codenbase.

I don't really like has_udapters, because it's not really what this variable is supposed to express here.
I thinl is_frozen_wait is fine, what do you think ?

manim/renderer/cairo_renderer.py

jsonvillanueva · 2021-03-18T07:01:22Z

manim/renderer/cairo_renderer.py

+        # Save a static image, to avoid rendering non moving objects.
+        self.static_image = self.save_static_frame_data(scene, scene.static_mobjects)


Altogether, the adjective "static" isn't helpful here since an image is already understood to be non-moving media. A better adjective might be "stationary", or "frozen" so it's not confused with static (as in @staticmethod) but it's not the image that is stationary/frozen, it's the Animation/the Mobjects in the Animation.

Suggested change

# Save a static image, to avoid rendering non moving objects.

self.static_image = self.save_static_frame_data(scene, scene.static_mobjects)

# Save a image, to avoid rendering non moving objects.

self.image = self.save_frame_data(scene, scene.static_mobjects)

Just for the record, this code is not mine (I moved this piece from elsewhere to here), the term static_image is braodly used to say that an image is static.

I believe this is not the right PR toc hange this, because it was not the main goal, but it's defintely an issue.

It's just an issue of better variable names, but if it helps move things along quicker then I'm fine with doing this in a future PR.

manim/renderer/cairo_renderer.py

manim/scene/scene.py

huguesdevimeux

@jsonvillanueva Thanks for the review, I answered to your suggestions

tests/test_scene_rendering/test_cairo_renderer.py

tests/test_scene_rendering/simple_scenes.py

huguesdevimeux · 2021-03-18T08:13:55Z

manim/animation/animation.py

@@ -248,6 +248,7 @@ def __init__(
        self.duration = duration
        self.mobject = None
        self.stop_condition = stop_condition
+        self.is_static_wait = False


Though I agree with you, I chose this name because it is how this type of wait is called within the codenbase.

I don't really like has_udapters, because it's not really what this variable is supposed to express here.
I thinl is_frozen_wait is fine, what do you think ?

huguesdevimeux · 2021-03-18T08:15:41Z

manim/renderer/cairo_renderer.py

+        # Save a static image, to avoid rendering non moving objects.
+        self.static_image = self.save_static_frame_data(scene, scene.static_mobjects)


Just for the record, this code is not mine (I moved this piece from elsewhere to here), the term static_image is braodly used to say that an image is static.

I believe this is not the right PR toc hange this, because it was not the main goal, but it's defintely an issue.

manim/renderer/cairo_renderer.py

manim/scene/scene.py

huguesdevimeux · 2021-03-18T08:19:45Z

tests/test_scene_rendering/test_play_logic.py

+@pytest.mark.skipif(
+    sys.version_info < (3, 8),
+    reason="Mock object has a different implementation in python 3.7, which makes it broken with this logic.",
+)


I will see what I can do.

To be honest, I didn't want to spend too much on investigating on this uncompataibilty, givent that this test is logically independent from the python version.

huguesdevimeux · 2021-03-22T09:57:37Z

I fear that this PR is made irrelevant with the new opengl_renderer, since the play logic is the same in open_gl renderer. and should be refactroed as well.

As long as open_gl renderer does not have proper tests, I won't work on it.

The tests are nevertheless addable, so I will make a separate PR.

leotrs · 2021-03-22T10:19:03Z

Not irrelevant at all, still very desirable to refactor the play logic in the cairo renderer I think. So, unless we are planning to stop all development on the cairo renderer, this should be merged.

huguesdevimeux · 2021-03-22T10:37:22Z

Not irrelevant at all, still very desirable to refactor the play logic in the cairo renderer I think. So, unless we are planning to stop all development on the cairo renderer, this should be merged.

Thanks for the reply. I changed my mind, so I tried to put again the old stuff this refactor has deleted (the cairo renderer doesn't use it anymore), to keep the opengl renderer working.

I tried a scene and it was working. Can someone confirm that it works ?

PS : Why on earth the OpenGL renderer does not have tests ??

PPS: Happy to see that you're back, @leotrs

leotrs · 2021-03-22T11:23:12Z

PS : Why on earth the OpenGL renderer does not have tests ??

We need ASAP before continuing development, I think.

PPS: Happy to see that you're back, @leotrs

jsonvillanueva · 2021-03-22T13:49:05Z

manim/renderer/cairo_renderer.py

@@ -60,7 +63,7 @@ def __init__(self, camera_class=None, skip_animations=False, **kwargs):
        self.file_writer = None
        camera_cls = camera_class if camera_class is not None else Camera
        self.camera = camera_cls()
-        self.original_skipping_status = skip_animations
+        self._original_skipping_status = skip_animations


Either this needs to change or opengl_renderer.py needs to change

Good catch. I'll modify it

huguesdevimeux · 2021-03-22T14:55:55Z

Thanks to @jsonvillanueva it is now seemingly working for the openGL renderer!

eulertour

Trying an OpenGL scene crashes for me:

 manim-2 git:(refactored-play-logic)  poetry run manim example_scenes/opengl.py SurfaceExample --use_opengl_renderer -p -ql
Manim Community v0.4.0
Traceback (most recent call last):
╭──────────────────────────────────────────────────────────────────────────────────────╮
│ File "/home/devneal/github/manim-2/manim/__main__.py", line 84, in main              │
│     81                 try:                                                          │
│     82                     renderer = OpenGLRenderer()                               │
│     83                     scene = SceneClass(renderer)                              │
│  ❱  84                     scene.render()                                            │
│     85                 except Exception:                                             │
│     86                     console.print_exception()                                 │
│     87         elif config["use_webgl_renderer"]:                                    │
│ File "/home/devneal/github/manim-2/manim/scene/scene.py", line 177, in render        │
│     174         """                                                                  │
│     175         self.setup()                                                         │
│     176         try:                                                                 │
│  ❱  177             self.construct()                                                 │
│     178         except EndSceneEarlyException:                                       │
│     179             pass                                                             │
│     180         self.tear_down()                                                     │
│ File "/home/devneal/github/manim-2/example_scenes/opengl.py", line 99, in construct  │
│     96                                                                               │
│     97         surface = surfaces[0]                                                 │
│     98                                                                               │
│  ❱  99         self.play(                                                            │
│    100             FadeIn(surface),                                                  │
│    101             ShowCreation(surface.mesh, lag_ratio=0.01, run_time=3),           │
│    102         )                                                                     │
│ File "/home/devneal/github/manim-2/manim/scene/scene.py", line 789, in play          │
│     786             return np.max([animation.run_time for animation in animations])  │
│     787                                                                              │
│     788     def play(self, *args, **kwargs):                                         │
│  ❱  789         self.renderer.play(self, *args, **kwargs)                            │
│     790                                                                              │
│     791     def wait(self, duration=DEFAULT_WAIT_TIME, stop_condition=None):         │
│     792         self.play(Wait(duration=duration, stop_condition=stop_condition))    │
│ File "/home/devneal/github/manim-2/manim/utils/caching.py", line 27, in wrapper      │
│    24     # method has to be deleted.                                                │
│    25                                                                                │
│    26     def wrapper(self, scene, *args, **kwargs):                                 │
│  ❱ 27         self.skip_animations = self.original_skipping_status                   │
│    28         self.update_skipping_status()                                          │
│    29         animations = scene.compile_animations(*args, **kwargs)                 │
│    30         scene.add_mobjects_from_animations(animations)                         │
╰──────────────────────────────────────────────────────────────────────────────────────╯
AttributeError: 'OpenGLRenderer' object has no attribute 'original_skipping_status'
 manim-2 git:(refactored-play-logic) 

manim/utils/caching.py

Co-authored-by: Devin Neal <devin@eulertour.com>

huguesdevimeux · 2021-03-23T06:31:59Z

@eulertour the last commit I made was seemingly wrong.
I commîtes your suggestion

Hugues Devimeux added 5 commits February 9, 2021 23:55

refactored play logic

a93cdf6

fixed tests (and minor improvement)

aec2409

moved time progression generation

fad7d26

fixed typo :/

9fa04ec

added tests

e92f9d0

huguesdevimeux requested review from leotrs and eulertour February 10, 2021 18:54

huguesdevimeux added 2 commits February 11, 2021 21:06

fixed tests

aef4904

fixed imports

98371e2

huguesdevimeux added 4 commits February 22, 2021 20:56

fixed tests

94cb9d2

black

d553e60

fixed tests in python 3.7

cba74e5

fixed tests for python 3.7

bef0a82

eulertour reviewed Feb 27, 2021

View reviewed changes

manim/renderer/cairo_renderer.py Outdated Show resolved Hide resolved

manim/renderer/cairo_renderer.py Outdated Show resolved Hide resolved

huguesdevimeux and others added 3 commits February 27, 2021 12:14

Update manim/renderer/cairo_renderer.py

f6f12c2

removed is_cached variable

faa7913

Merge branch 'refactored-play-logic' of github.com:huguesdevimeux/man…

d836493

…im into refactored-play-logic

huguesdevimeux mentioned this pull request Feb 28, 2021

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

Closed

updated webgl renderer and removed redondent code

c660a24

huguesdevimeux commented Mar 7, 2021

View reviewed changes

manim/scene/scene.py Outdated Show resolved Hide resolved

Update manim/scene/scene.py

5a9f4ac

jsonvillanueva requested changes Mar 18, 2021

View reviewed changes

huguesdevimeux commented Mar 18, 2021

View reviewed changes

jsonvillanueva approved these changes Mar 21, 2021

View reviewed changes

Merge branch 'master' into refactored-play-logic

a28b575

Fixed openGL ?

54e52d7

huguesdevimeux added 2 commits March 22, 2021 11:39

black

0bbdabf

fixed merge conflict by adding fixture marker

0962152

jsonvillanueva reviewed Mar 22, 2021

View reviewed changes

Called begin_animation to fix OpenGL

16296e3

Fixed original_skippingstatu sname for opengl renderer

d29ad4e

eulertour reviewed Mar 23, 2021

View reviewed changes

manim/utils/caching.py Outdated Show resolved Hide resolved

Update manim/utils/caching.py

dac4df7

Co-authored-by: Devin Neal <devin@eulertour.com>

eulertour approved these changes Mar 23, 2021

View reviewed changes

huguesdevimeux merged commit fc6159b into ManimCommunity:master Mar 23, 2021

huguesdevimeux deleted the refactored-play-logic branch March 23, 2021 08:58

jsonvillanueva added refactor Refactor or redesign of existing code maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements and removed refactor Refactor or redesign of existing code labels Mar 23, 2021

jsonvillanueva changed the title ~~Refactored play logic~~ Refactored :meth:~.Scene.play Mar 24, 2021

jsonvillanueva added the testing Anything related to testing the library label Mar 24, 2021

jsonvillanueva mentioned this pull request Mar 24, 2021

Fixed single PEP8 style in cairo_renderer.py #1164

Merged

6 tasks

behackl added this to the Release v0.5.0 milestone Mar 24, 2021

huguesdevimeux removed the testing Anything related to testing the library label Mar 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored :meth:`~.Scene.play` #1019

Refactored :meth:`~.Scene.play` #1019

huguesdevimeux commented Feb 10, 2021 •

edited by jsonvillanueva

Loading

behackl commented Feb 14, 2021

huguesdevimeux commented Feb 24, 2021

eulertour left a comment

huguesdevimeux commented Feb 27, 2021

eulertour commented Mar 1, 2021

huguesdevimeux commented Mar 5, 2021

eulertour commented Mar 7, 2021

huguesdevimeux commented Mar 7, 2021 •

edited

Loading

jsonvillanueva commented Mar 9, 2021 •

edited

Loading

jsonvillanueva commented Mar 9, 2021 •

edited

Loading

jsonvillanueva left a comment

jsonvillanueva Mar 18, 2021

huguesdevimeux Mar 18, 2021

jsonvillanueva Mar 18, 2021

huguesdevimeux Mar 18, 2021

jsonvillanueva Mar 18, 2021

huguesdevimeux Mar 18, 2021

jsonvillanueva Mar 21, 2021

huguesdevimeux left a comment

huguesdevimeux Mar 18, 2021

huguesdevimeux Mar 18, 2021

huguesdevimeux Mar 18, 2021

huguesdevimeux commented Mar 22, 2021

leotrs commented Mar 22, 2021 •

edited

Loading

huguesdevimeux commented Mar 22, 2021

leotrs commented Mar 22, 2021

jsonvillanueva Mar 22, 2021

huguesdevimeux Mar 22, 2021

huguesdevimeux commented Mar 22, 2021

eulertour left a comment

huguesdevimeux commented Mar 23, 2021

		# Save a static image, to avoid rendering non moving objects.
		self.static_image = self.save_static_frame_data(scene, scene.static_mobjects)

Refactored :meth:~.Scene.play #1019

Refactored :meth:~.Scene.play #1019

Conversation

huguesdevimeux commented Feb 10, 2021 • edited by jsonvillanueva Loading

Motivation

Overview / Explanation for Changes

Oneline Summary of Changes

Testing Status

Further Comments

Acknowledgements

Reviewer Checklist

behackl commented Feb 14, 2021

huguesdevimeux commented Feb 24, 2021

eulertour left a comment

Choose a reason for hiding this comment

huguesdevimeux commented Feb 27, 2021

eulertour commented Mar 1, 2021

huguesdevimeux commented Mar 5, 2021

eulertour commented Mar 7, 2021

huguesdevimeux commented Mar 7, 2021 • edited Loading

jsonvillanueva commented Mar 9, 2021 • edited Loading

jsonvillanueva commented Mar 9, 2021 • edited Loading

jsonvillanueva left a comment

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

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

huguesdevimeux left a comment

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

huguesdevimeux commented Mar 22, 2021

leotrs commented Mar 22, 2021 • edited Loading

huguesdevimeux commented Mar 22, 2021

leotrs commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huguesdevimeux commented Mar 22, 2021

eulertour left a comment

Choose a reason for hiding this comment

huguesdevimeux commented Mar 23, 2021

Refactored :meth:`~.Scene.play` #1019

Refactored :meth:`~.Scene.play` #1019

huguesdevimeux commented Feb 10, 2021 •

edited by jsonvillanueva

Loading

huguesdevimeux commented Mar 7, 2021 •

edited

Loading

jsonvillanueva commented Mar 9, 2021 •

edited

Loading

jsonvillanueva commented Mar 9, 2021 •

edited

Loading

leotrs commented Mar 22, 2021 •

edited

Loading