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

Allow playing Animations asynchronously using OpenGLRenderer #1528

Closed
wants to merge 10 commits into from

Conversation

AntonBallmaier
Copy link
Contributor

@AntonBallmaier AntonBallmaier commented May 17, 2021

Changelog / Overview

Scene.play can take a new keyword argument run_async. If True this will start the animation and return to executing Scene.construct immediately. This feature is only availably using the OpenGLRenderer.

Motivation

see #1488
closes #1267

Explanation for Changes

Documentation Reference

Testing Status

Further Comments

I would like to add an example to the docs, but as far as I know we cannot render OpenGl-videos to the docs, right?

Checklist

  • I have read the Contributing Guidelines
  • I have written a descriptive PR title (see top of PR template for examples)
  • I have written a changelog entry for the PR or deem it unnecessary
  • My new functions/classes either have a docstring or are private
  • My new functions/classes have tests added and (optional) examples in the docs
  • My new documentation builds, looks correctly formatted, and adds no additional build warnings

Reviewer Checklist

  • The PR title is descriptive enough
  • The PR is labeled correctly
  • The changelog entry is completed if necessary
  • Newly added functions/classes either have a docstring or are private
  • 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

@AntonBallmaier AntonBallmaier added the new feature Enhancement specifically adding a new feature (feature request should be used for issues instead) label May 17, 2021
@AntonBallmaier AntonBallmaier requested a review from behackl May 17, 2021 10:12
@TRoboto
Copy link
Collaborator

TRoboto commented May 17, 2021

I've tried to play your example #1267, with preview enabled, after converting it to OpenGL, but it didn't work as it should. When I write to a file, I receive this error.

ValueError: write to closed file

Find the scene below. Note that you should temporarily change this line to lines = OpenGLVGroup() in order for it to work.

class UsingUpdater(Scene):
    def construct(self):
        # Setup
        dots = OpenGLVGroup(
            *[
                Dot(RIGHT * random.randrange(-6, 6) + UP * random.randrange(-2, 2))
                for i in range(10)
            ]
        )
        line = Line(UP * 3, DOWN * 3).align_on_border(LEFT)
        self.add(dots)
        self.play(Create(line))

        # Updater
        def flash_hit_dots(mob):
            for dot in dots:
                if dot.get_center()[0] < mob.get_center()[0] and not hasattr(
                    dot, "marked"
                ):
                    self.play(Flash(dot, run_time=0.5))
                    dot.marked = True

        line.add_updater(flash_hit_dots)

        # Animate Line
        self.play(line.animate.align_on_border(RIGHT), run_time=5)
        self.wait()

@AntonBallmaier
Copy link
Contributor Author

AntonBallmaier commented May 17, 2021

I've tried to play your example #1267, with preview enabled, after converting it to OpenGL, but it didn't work as it should. When I write to a file, I receive this error.

ValueError: write to closed file

That is the error that one gets without the changes I made. Have you made sure that you are actually running the code on the branch of this PR? I just tested the same code and don't get that error.

Find the scene below. Note that you should temporarily change this line to lines = OpenGLVGroup() in order for it to work.

class UsingUpdater(Scene):
    def construct(self):
        # Setup
        dots = OpenGLVGroup(
            *[
                Dot(RIGHT * random.randrange(-6, 6) + UP * random.randrange(-2, 2))
                for i in range(10)
            ]
        )
        line = Line(UP * 3, DOWN * 3).align_on_border(LEFT)
        self.add(dots)
        self.play(Create(line))

        # Updater
        def flash_hit_dots(mob):
            for dot in dots:
                if dot.get_center()[0] < mob.get_center()[0] and not hasattr(
                    dot, "marked"
                ):
                    self.play(Flash(dot, run_time=0.5))
                    dot.marked = True

        line.add_updater(flash_hit_dots)

        # Animate Line
        self.play(line.animate.align_on_border(RIGHT), run_time=5)
        self.wait()

I tested the code and had to make a tiny adjustment for it to work:

class UsingUpdater(Scene):
    def construct(self):
        # Setup
        dots = OpenGLVGroup(
            *[
                Dot(RIGHT * random.randrange(-6, 6) + UP * random.randrange(-2, 2))
                for i in range(10)
            ]
        )
        line = Line(UP * 3, DOWN * 3).align_on_border(LEFT)
        self.add(dots)

        # Updater
        def flash_hit_dots(mob):
            for dot in dots:
                if dot.get_center()[0] < line.get_center()[0] and not hasattr(
                    dot, "marked"
                ):
                    self.play(Flash(dot, run_time=0.5, rate_func=rush_from))
                    dot.marked = True

        line.add_updater(flash_hit_dots)

        # Animate Line
        self.play(line.animate.align_on_border(RIGHT), run_time=5)
        self.wait()

Resulting in:

UsingUpdater.mp4

@TRoboto
Copy link
Collaborator

TRoboto commented May 17, 2021

The old scene still doesn't work but with your edits, it does. When the mob argument is used inside the updater function, everything breaks. Is this supposed to happen?

@AntonBallmaier
Copy link
Contributor Author

AntonBallmaier commented May 17, 2021

The old scene still doesn't work but with your edits, it does. When the mob argument is used inside the updater function, everything breaks. Is this supposed to happen?

Definitely not, and I will open have opened an issue addressing that bug. It is however (as far as I can tell) (for sure) not related to this PR. See #1536 :) Thanks for the tip making me look into this 👍

manim/scene/scene.py Outdated Show resolved Hide resolved
@TRoboto
Copy link
Collaborator

TRoboto commented May 17, 2021

Definitely not, and I will open have opened an issue addressing that bug. It is however (as far as I can tell) (for sure) not related to this PR. See #1536 :) Thanks for the tip making me look into this +1

Thank you for the unstoppable contributions, your work is astonishing. This PR LGTM now.

@AntonBallmaier AntonBallmaier requested review from huguesdevimeux and removed request for behackl May 27, 2021 11:15
Copy link
Member

@huguesdevimeux huguesdevimeux left a comment

Choose a reason for hiding this comment

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

So, that's a great addition. I left some comments.
This crucially needs two things to my eyes :

  • Comments of the logic ; there are parts that are kind of obscure and need explanation.
  • This needs test, I think both graphical and units. You could use mocks to test, I think.

Aside, that's (again!) a great contribution !

@@ -946,6 +1032,7 @@ def play_internal(self, skip_rendering=False):
named parameters affecting what was passed in ``args``,
e.g. ``run_time``, ``lag_ratio`` and so on.
"""
self.sync_play_running = True
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 using a decorator here would make sense and would improve readability imo

"""
mobject = animation.mobject

animation.suspend_mobject_updating = False
Copy link
Member

Choose a reason for hiding this comment

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

Question : why ?

animation.update_mobjects(dt)
update.total_time += dt

update.total_time = 0
Copy link
Member

Choose a reason for hiding this comment

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

Coment : Although I undestand what you're doing here, I'm not really a big fan of adding attributes to functions.
Why not turing update into a proper class with a __call__ method ?

Comment on lines +889 to +905
if self.sync_play_running:
if run_async is None:
run_async = True
if not run_async:
raise ValueError(
"Cannot start a synchronous animation while another is running."
)
elif run_async is None:
run_async = False

if run_async:
animations = self.compile_animations(*animations, **kwargs)

self.add_mobjects_from_animations(animations)
for animation in animations:
self._turn_animation_to_updater(animation)
return
Copy link
Member

Choose a reason for hiding this comment

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

I believe the whole logic can be simplified.
You could do instead

<run_sync is False by default)
if self.sync_play_running 
   run_sync = False if config[blabla] = blab else run_sync
   if not run_sync: 
       # ValueError blabla
   # and then the play logic (inside l 899). 
# here (outside the if) the normal play logic. 

What do you think ?

@Darylgolden
Copy link
Member

Hi, can I check on the status of this PR?

@MrDiver
Copy link
Collaborator

MrDiver commented Jun 18, 2022

Closing this for now because there is no activity. We can come back to this. The issue is on the project board and the PR is linked!

@MrDiver MrDiver closed this Jun 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Enhancement specifically adding a new feature (feature request should be used for issues instead)
Projects
Status: Rejected
Development

Successfully merging this pull request may close these issues.

Starting Animations using Scene.play while others are running
5 participants