Skip to content

Conversation

sparshg
Copy link
Contributor

@sparshg sparshg commented Apr 5, 2021

Changelog / Overview

  • If no animations in scene and asked to preview/render a video, preview/render an image instead of raising a confusing error.
  • Replace self.play(Animation()) with self.add() in tests (without changing control data)
  • Fixed a badly written test (ValueTracker test)

Explanation for changes

As we already know, there is a lot of confusion when new users try to render scenes without animations and getting weird errors. After the scene is finished, just before writing the file, we can check if there are any animations in the scene, if not it changes config["save_last_frame"] to True, and config["write_to_movie"] to False.

fixes #1052 #869 and ManimCommunity/manim-website#7

Tests

Passing

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

@sparshg sparshg marked this pull request as draft April 5, 2021 15:31
@sparshg sparshg marked this pull request as ready for review April 5, 2021 15:47
@sparshg sparshg marked this pull request as draft April 5, 2021 15:59
@sparshg sparshg marked this pull request as ready for review April 5, 2021 16:16
@sparshg
Copy link
Contributor Author

sparshg commented Apr 5, 2021

Should I remove the condition that the image is only generated if attempting to preview/open-file-location for the video? coz without -p or -f, you will still see that error.

@huguesdevimeux
Copy link
Member

Should I remove the condition that the image is only generated if attempting to preview/open-file-location for the video? coz without -p or -f, you will still see that error.

I don't think so, but the error should be suppressed in favor of a logging error message. (I don't think it would fit in this PR though, up to you).

By the way, could you add a log message when an image is displayed by default ?

@sparshg
Copy link
Contributor Author

sparshg commented Apr 6, 2021

Ok I added some logging warnings and also file_writer.finish() is not called if no animations were found to suppress the error message

Copy link
Member

@jsonvillanueva jsonvillanueva left a comment

Choose a reason for hiding this comment

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

On second thought, I don't think the logging warnings should exist. I think the expected behavior for manim -ql [FILE] and manim -pql [FILE] should be to output some file if possible. There should only be a warning if the user explicitly asks for a filetype that isn't possible (e.g. user passes --format option expecting mp4/mov/gif file but there's no animations -- a PNG will always be possible since blank background image)

@sparshg sparshg changed the title Display image instead if no animation found while writing video Render image if no animation in scene Apr 7, 2021
@sparshg sparshg requested a review from jsonvillanueva April 7, 2021 13:16
@behackl
Copy link
Member

behackl commented Apr 7, 2021

The issues with the documentation should now be resolved; it might take a while until it manages to run the build (people are working on too many PRs in parallel, which is nice on another level 😄). If it passes, I believe that this can be merged.

Thank you very much for your contribution; I'll wait with my approval until the pipeline passes.

@behackl behackl added the new feature Enhancement specifically adding a new feature (feature request should be used for issues instead) label Apr 7, 2021
@behackl behackl added this to the Release v0.6.0 milestone Apr 7, 2021
@behackl behackl changed the title Render image if no animation in scene Render image automatically if no animation is played in a scene Apr 7, 2021
@jsonvillanueva
Copy link
Member

Ahh... I had a hunch the fix was something to do with manim_directive.py

@behackl behackl merged commit 08e06e1 into ManimCommunity:master Apr 8, 2021
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
None yet
Development

Successfully merging this pull request may close these issues.

Show helpful error message instead of FileNotFound
4 participants