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

Fixed behavior of config.background_opacity < 1 and duplicated log messages #3680

Merged
merged 14 commits into from
Jul 14, 2024

Conversation

abul4fia
Copy link
Contributor

@abul4fia abul4fia commented Apr 7, 2024

Setting background_transparency=0.0 should have the same effect than setting transparent=True. The output format must be compatible with transparency (.mov, .webm). Otherwise ffpmeg fails.

This patch ensures that background_transparency=0.0 triggers the change of the output format. Also a WARNING is raised about the change in the output format.

I want to thank @SirJamesClarkMaxwell for inspiring me to tackle this issue.

Overview: What does this pull request change?

Motivation and Explanation: Why and how do your changes improve the library?

Links to added or changed documentation pages

Further Information and Comments

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

Setting background_transparency=0.0 should have the same effect than
setting transparent=True. The output format must be compatible
with transparency (.mov, .webm). Otherwise ffpmeg fails.

This patch ensures that background_transparency=0.0 triggers the
change of the output format. Also a WARNING is raised about
the change in the output format.

I want to thank @SirJamesClarkMaxwell for inspiring me to tackle
this issue.
@JasonGrace2282 JasonGrace2282 changed the title Fixes #3614 Fix config.background_opacity=0 failing Apr 8, 2024
@JasonGrace2282 JasonGrace2282 added the pr:bugfix Bug fix for use in PRs solving a specific issue:bug label Apr 22, 2024
Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

LGTM

@behackl
Copy link
Member

behackl commented Apr 25, 2024

Wouldn't actually any value for background_opacity smaller than 1 mean that the video should be rendered with transparency?

@behackl
Copy link
Member

behackl commented Jul 13, 2024

I've pushed some fixes for the behavior of config.background_opacity strictly between 0 and 1, and added an explicit test for the case of config.background_opacity == 0.5.

It would be great if someone could cross-review my changes, otherwise I'm happy to get this merged.

@behackl
Copy link
Member

behackl commented Jul 13, 2024

I've now properly fixed the logger situation -- sorry, this should probably have been a separate PR. Should be easiest to review by looking at c800ba1 and 78dfd41 separately.

Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

Took a look at the changes, mostly looks good to me. I did leave some comments, so I would appreciate if you or @behackl could take a look :)
I also have one question now: with this change, should we just have a per-file logger with logger = logging.getLogger("manim")? If so, would it make sense to remove the logger from being exported in manim._config?

Sidenote:
I was actually having similar problems on experimental so maybe this will help out :)

tests/test_config.py Outdated Show resolved Hide resolved
manim/utils/deprecation.py Outdated Show resolved Hide resolved
tests/module/utils/test_deprecation.py Outdated Show resolved Hide resolved
@behackl behackl changed the title Fix config.background_opacity=0 failing Fixed behavior of config.background_opacity < 1 and duplicated log messages Jul 14, 2024
@behackl behackl added the enhancement Additions and improvements in general label Jul 14, 2024
Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

Left one idea - otherwise LGTM!

tests/module/utils/test_deprecation.py Show resolved Hide resolved
Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

Actually just realized there is a line added that was added in another PR (#3839 )
The other changes look useful, so maybe just merge main into this branch and I'll double check then?

manim/_config/utils.py Show resolved Hide resolved
Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

Oh nice, I love git :)
LGTM, for real, this time.

@behackl behackl merged commit f1ce512 into ManimCommunity:main Jul 14, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additions and improvements in general pr:bugfix Bug fix for use in PRs solving a specific issue:bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants