feat(cli/lib): use scene background color as default #160

Fairlight8 · 2023-03-17T00:21:39Z

Fixes Issue

Closes #130

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.

Note to reviewers

Please review the code, just in case ^^

for more information, see https://pre-commit.ci

jeertmans

Thank you for your work @Fairlight8, I requested a few changes before I can merge :-)

jeertmans · 2023-03-17T12:50:53Z

manim_slides/config.py

@@ -150,6 +150,7 @@ class PresentationConfig(BaseModel):  # type: ignore
    slides: List[SlideConfig]
    files: List[FilePath]
    resolution: Tuple[PositiveInt, PositiveInt] = (1920, 1080)
+    background_color: str = "black"


Suggested change

background_color: str = "black"

background_color: str = Color

Please use the Color type from Pydantic: https://docs.pydantic.dev/usage/types/#color-type.

jeertmans · 2023-03-17T12:53:51Z

manim_slides/present.py

+        self.label.setStyleSheet(
+            f"background-color: {self.thread.current_presentation.background_color}"
+        )


Suggested change

self.label.setStyleSheet(

f"background-color: {self.thread.current_presentation.background_color}"

)

self.label.setStyleSheet(

f"background-color: {self.thread.current_background_color}"

)

For readability, create a current_background_color property :-)

jeertmans · 2023-03-17T12:56:05Z

manim_slides/slide.py

@@ -53,6 +53,7 @@ def __init__(
        self.__current_animation = 0
        self.__loop_start_animation: Optional[int] = None
        self.__pause_start_animation = 0
+        self.__background_color = config["background_color"].hex


This does not work because the background_color attribute is not placed in config with ManimGL. So you need to a separate implementation, as with __resolution:

manim-slides/manim_slides/slide.py

Lines 57 to 63 in 2a327c4

@property

def __resolution(self) -> Tuple[int, int]:

"""Returns the scene's resolution used during rendering."""

if MANIMGL:

return self.camera_config["pixel_width"], self.camera_config["pixel_height"]

else:

return config["pixel_width"], config["pixel_height"]

Oh, I Didn't know, understood!

jeertmans · 2023-03-17T12:57:06Z

manim_slides/present.py

+    if background_color is not None:
+        for presentation_config in presentation_configs:
+            presentation_config.background_color = background_color
+


I think you need to remove the background_color option from App :-)

Fairlight8 · 2023-03-17T18:18:01Z

Of course, of course. Sorry for the potential mistakes. It is a simple issue but I am still new contributing to other projects.

I agree with all the changes. Do I have to accept them?

jeertmans · 2023-03-17T18:41:52Z

No problem, first contributions are always hard!

In general, you can approve them, yes. But here, some codes changes are still needed beyond those I proposed. For example, you need to import the Color from pydantic first :)

Fairlight8 · 2023-03-17T21:05:12Z

Ohhhh, true true true. So, what's the process now? Should I add a different PR? Add new commits? Can I change this PR for another commit?

jeertmans · 2023-03-17T21:22:48Z

You can just push new commits :)

for more information, see https://pre-commit.ci

Fairlight8 · 2023-03-22T00:37:27Z

Added some changes! I think that's fine now!

manim_slides/slide.py

jeertmans · 2023-03-22T08:34:43Z

Nice work! A few last changes before merging this were requested. You can see that pre-commit.ci does some type checking, and a few errors were pointed-out: https://results.pre-commit.ci/run/github/513032928/1679445219.3lZ-NZzDS-2nlcoJtXckeQ.

You can just approve the changes if you agree with them.

Fairlight8 · 2023-03-22T11:24:05Z

Another commit for those details. By the way. The github pages thing is my issue, right?

jeertmans · 2023-03-22T11:57:12Z

Another commit for those details. By the way. The github pages thing is my issue, right?

You don't have to mind about that :-)

manim_slides/slide.py

Fairlight8 · 2023-03-22T12:09:55Z

Goodness, too many new things xDDD. That is already changed, right? I pulled from git and it is already changed.

jeertmans · 2023-03-22T12:58:59Z

Yep sorry, I didn't want to bother you, so I committed the changes directly ^^

Looks good to me, I will merge this and soon release this :-) Probably as part of 4.12

Fairlight8 · 2023-03-22T13:35:58Z

Understood, thank you! Again, thanks a lot for your patience. Probably, it would be much more shorter and easier if you did that on your own, so I appreciate the patience while I learn all these things!

jeertmans · 2023-03-22T14:30:07Z

No problem, it was a pleasure :-) I've been there too!

Fairlight8 and others added 2 commits March 17, 2023 00:55

Use scene background color as default

bec16f2

[pre-commit.ci] auto fixes from pre-commit.com hooks

561afc9

for more information, see https://pre-commit.ci

pre-commit-ci bot temporarily deployed to github-pages March 17, 2023 00:22 Inactive

jeertmans requested changes Mar 17, 2023

View reviewed changes

jeertmans added enhancement New feature or request present Related to the main "present" feature labels Mar 17, 2023

jeertmans changed the title ~~Use scene background color as default~~ feat(cli/lib): scene background color as default Mar 20, 2023

jeertmans changed the title ~~feat(cli/lib): scene background color as default~~ feat(cli/lib): use scene background color as default Mar 20, 2023

Merge branch 'jeertmans:main' into main

b6f527b

Fairlight8 temporarily deployed to github-pages March 21, 2023 23:16 — with GitHub Actions Inactive

Fairlight8 and others added 2 commits March 22, 2023 01:33

Minor changes to feature: Read scene background

4b4b886

[pre-commit.ci] auto fixes from pre-commit.com hooks

1a19b41

for more information, see https://pre-commit.ci

pre-commit-ci bot temporarily deployed to github-pages March 22, 2023 00:33 Inactive

jeertmans requested changes Mar 22, 2023

View reviewed changes

manim_slides/slide.py Outdated Show resolved Hide resolved

manim_slides/slide.py Outdated Show resolved Hide resolved

manim_slides/slide.py Outdated Show resolved Hide resolved

Small fix for feature "Read bg color"

c4ca277

Fairlight8 temporarily deployed to github-pages March 22, 2023 11:23 — with GitHub Actions Inactive

jeertmans approved these changes Mar 22, 2023

View reviewed changes

manim_slides/slide.py Outdated Show resolved Hide resolved

chore(ci): add typing ignore

3c6cf94

jeertmans temporarily deployed to github-pages March 22, 2023 11:59 — with GitHub Actions Inactive

jeertmans requested changes Mar 22, 2023

View reviewed changes

manim_slides/slide.py Outdated Show resolved Hide resolved

fix(ci): typo

e027a21

jeertmans temporarily deployed to github-pages March 22, 2023 12:07 — with GitHub Actions Inactive

jeertmans merged commit 88d5987 into jeertmans:main Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli/lib): use scene background color as default #160

feat(cli/lib): use scene background color as default #160

Fairlight8 commented Mar 17, 2023 •

edited

Loading

jeertmans left a comment

jeertmans Mar 17, 2023

jeertmans Mar 17, 2023

jeertmans Mar 17, 2023

Fairlight8 Mar 17, 2023

jeertmans Mar 17, 2023

Fairlight8 commented Mar 17, 2023

jeertmans commented Mar 17, 2023

Fairlight8 commented Mar 17, 2023

jeertmans commented Mar 17, 2023

Fairlight8 commented Mar 22, 2023

jeertmans commented Mar 22, 2023

Fairlight8 commented Mar 22, 2023

jeertmans commented Mar 22, 2023

Fairlight8 commented Mar 22, 2023

jeertmans commented Mar 22, 2023

Fairlight8 commented Mar 22, 2023

jeertmans commented Mar 22, 2023

	background_color: str = "black"
	background_color: str = Color

	@property
	def __resolution(self) -> Tuple[int, int]:
	"""Returns the scene's resolution used during rendering."""
	if MANIMGL:
	return self.camera_config["pixel_width"], self.camera_config["pixel_height"]
	else:
	return config["pixel_width"], config["pixel_height"]

feat(cli/lib): use scene background color as default #160

feat(cli/lib): use scene background color as default #160

Conversation

Fairlight8 commented Mar 17, 2023 • edited Loading

Fixes Issue

Check List (Check all the applicable boxes)

Note to reviewers

jeertmans left a comment

Choose a reason for hiding this comment

jeertmans Mar 17, 2023

Choose a reason for hiding this comment

jeertmans Mar 17, 2023

Choose a reason for hiding this comment

jeertmans Mar 17, 2023

Choose a reason for hiding this comment

Fairlight8 Mar 17, 2023

Choose a reason for hiding this comment

jeertmans Mar 17, 2023

Choose a reason for hiding this comment

Fairlight8 commented Mar 17, 2023

jeertmans commented Mar 17, 2023

Fairlight8 commented Mar 17, 2023

jeertmans commented Mar 17, 2023

Fairlight8 commented Mar 22, 2023

jeertmans commented Mar 22, 2023

Fairlight8 commented Mar 22, 2023

jeertmans commented Mar 22, 2023

Fairlight8 commented Mar 22, 2023

jeertmans commented Mar 22, 2023

Fairlight8 commented Mar 22, 2023

jeertmans commented Mar 22, 2023

Fairlight8 commented Mar 17, 2023 •

edited

Loading