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

feat(lib): add skip_animations compatibility #516

Merged
merged 8 commits into from
Jan 21, 2025

Conversation

Rapsssito
Copy link
Contributor

Fixes Issue

Fixes #514

Description

As described in title.

Check List

Check all the applicable boxes:

  • I understand that my contributions needs to pass the checks;
  • If I created new functions / methods, I documented them and add type hints;
  • If I modified already existing code, I updated the documentation accordingly;
  • The title of my pull request is a short description of the requested changes.

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 79.51%. Comparing base (df31345) to head (5be5209).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
manim_slides/slide/base.py 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #516      +/-   ##
==========================================
- Coverage   79.56%   79.51%   -0.05%     
==========================================
  Files          23       23              
  Lines        1962     1972      +10     
==========================================
+ Hits         1561     1568       +7     
- Misses        401      404       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jeertmans jeertmans added enhancement New feature or request lib Related to the library (a.k.a. module) labels Jan 14, 2025
@jeertmans
Copy link
Owner

Hi @Rapsssito, thanks for your contribution!

Small comments:

  1. Did you test your code? It doesn't look like skip_animations is passed to Manim (which will thus render the animations, but skip them from Manim Slides's config). I think you need to edit the following
    @BaseSlideConfig.wrapper("base_slide_config")
    def next_slide(
    self,
    *args: Any,
    base_slide_config: BaseSlideConfig,
    **kwargs: Any,
    ) -> None:
    Scene.next_section(self, *args, **kwargs)
    BaseSlide.next_slide.__wrapped__(
    self,
    base_slide_config=base_slide_config,
    )

    and set something like Scene.next_section(self, *args, skip_animations=base_slide_config.skip_animations, **kwargs) to make sure the argument is passed to next_section.
  2. Could you add a test in tests/test_slides.py::TestSlide?
  3. Could you document your changes in the CHANGELOG?

Many thanks!

@jeertmans jeertmans changed the title feat: Add skip_animations compatibility feat(lib): add skip_animations compatibility Jan 15, 2025
@Rapsssito
Copy link
Contributor Author

@jeertmans, I did test the code, but I just checked if the slide was indeed not there... My bad. I am sorry about my ignorance, but how can I test it in tests/test_slides.py::TestSlide? Is there a way to check for the existence intermediate animation files?

@jeertmans
Copy link
Owner

We don't need to go that far in the testing, especially as it would highly depend on where Manim stores partial animation files.

But you could add a test that reads the PresentationConfig object (from the generated .json file) and checks that slides are indeed missing when skip_animations=True is set.

@jeertmans
Copy link
Owner

Note, it might be interesting to document this new parameter here:

:param args:
Positional arguments passed to
:meth:`Scene.next_section<manim.scene.scene.Scene.next_section>`,
or ignored if `manimlib` API is used.
:param loop:
If set, next slide will be looping.
:param auto_next:
If set, next slide will play immediately play the next slide
upon terminating.
.. warning::
Only supported by ``manim-slides present``
and ``manim-slides convert --to=html``.
:param playback_rate:
Playback rate at which the video is played.
.. warning::
Only supported by ``manim-slides present``.
:param reversed_playback_rate:
Playback rate at which the reversed video is played.
.. warning::
Only supported by ``manim-slides present``.
:param notes:
Presenter notes, in Markdown format.
.. note::
PowerPoint does not support Markdown formatting,
so the text will be displayed as is.
.. warning::
Only supported by ``manim-slides present``,
``manim-slides convert --to=html`` and
``manim-slides convert --to=pptx``.
:param dedent_notes:
If set, apply :func:`textwrap.dedent` to notes.
:param kwargs:
Keyword arguments passed to
:meth:`Scene.next_section<manim.scene.scene.Scene.next_section>`,
or ignored if `manimlib` API is used.

and mention that it is passed to `:meth:`Scene.next_section<manim.scene.scene.Scene.next_section>`, or ignored if `manimlib` API is used.

@Rapsssito
Copy link
Contributor Author

Rapsssito commented Jan 20, 2025

I have added the fix and the changelog. However, I am afraid I need your assistance with the tests. I was not able to execute them in my environment with uv run pytest .\tests\test_slide.py because I get the error ModuleNotFoundError: No module named 'manimlib'. Also, I am not sure how should I access the PresentationConfig config from the TestSlide class scope.

Note, it might be interesting to document this new parameter here:

:param args:
Positional arguments passed to
:meth:`Scene.next_section<manim.scene.scene.Scene.next_section>`,
or ignored if `manimlib` API is used.
:param loop:
If set, next slide will be looping.
:param auto_next:
If set, next slide will play immediately play the next slide
upon terminating.
.. warning::
Only supported by ``manim-slides present``
and ``manim-slides convert --to=html``.
:param playback_rate:
Playback rate at which the video is played.
.. warning::
Only supported by ``manim-slides present``.
:param reversed_playback_rate:
Playback rate at which the reversed video is played.
.. warning::
Only supported by ``manim-slides present``.
:param notes:
Presenter notes, in Markdown format.
.. note::
PowerPoint does not support Markdown formatting,
so the text will be displayed as is.
.. warning::
Only supported by ``manim-slides present``,
``manim-slides convert --to=html`` and
``manim-slides convert --to=pptx``.
:param dedent_notes:
If set, apply :func:`textwrap.dedent` to notes.
:param kwargs:
Keyword arguments passed to
:meth:`Scene.next_section<manim.scene.scene.Scene.next_section>`,
or ignored if `manimlib` API is used.

and mention that it is passed to `:meth:`Scene.next_section<manim.scene.scene.Scene.next_section>`, or ignored if `manimlib` API is used.

I have also added it, but reading the documentation, I would assume it is the same as (I actually did until I discovered it raised an Exception):

            :param args:
            Positional arguments passed to
            :meth:`Scene.next_section<manim.scene.scene.Scene.next_section>`,
            or ignored if `manimlib` API is used.

Rapsssito and others added 2 commits January 20, 2025 14:55
Co-authored-by: Jérome Eertmans <jeertmans@icloud.com>
@Rapsssito
Copy link
Contributor Author

Rapsssito commented Jan 20, 2025

In case you missed it, the tests are still missing! I need your help with the implementation.

@jeertmans
Copy link
Owner

In case you missed it, the tests are still missing! I need your help with the implementation.

I didn't miss, I was just waiting for your feedback on that :-)

I will implement the tests myself then ;-)

@jeertmans
Copy link
Owner

Thanks for your contribution @Rapsssito!

@jeertmans jeertmans merged commit 32ab690 into jeertmans:main Jan 21, 2025
37 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lib Related to the library (a.k.a. module)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] skip_animations causes ValueError: Cannot merge an empty list of files!
2 participants