Skip to content

Conversation

@BrorSebastianSjovald
Copy link

Overview: What does this pull request change?

This PR improves the color handling by deprecating uses of the Colors class and replace usage of the Enums with color constants as specified in #2451 . At this point, a deprecation warning is raised when trying to use the Colors class using manim.utils.deprecation.

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

The overall goal is to simplify the use of color by standardizing the approach as stated in #2451 . By deprecating the use of the Enums and providing a way of using the constants in a similar way, we're creating a standard for how colors should be used.

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

ludwigjo and others added 9 commits March 9, 2022 14:16
…o… (#6)

* Removed enum from color class and temporar fix to access constant from text_mobjects, fix #2

* Removed color class and improved safety on how constant are accessed, fix #2
* Converts values in setting_args dict in _get_settings_from_t2xs to string to make sure that all values are of type string.

* Makes color constants instances of Color-class from colour-module and updates color_gradient to handle gradient with one color, fixes #7.
* Added deprecation warnings for color conversions, feat #1

* Update and remove unecessary warnings, fix #1

* Added more specific deprecation warnings

* Removed warnings from functions that are not in `colour` module

* Fixed deprecation warning in `color_to_rgb`, fix #1
* Added deprecation warnings when accessing color enums, fix #8

* Simplified import statement for warnings, fix #8
Copy link
Member

@Darylgolden Darylgolden left a comment

Choose a reason for hiding this comment

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

Thank you for your work! Other than some minor typehint problems, I think this is good to go.

self,
mobject,
color: Colors | None = None,
color: str | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
color: str | None = None,
color: Color | str | None = None,

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure that a color object would work fine here too.

# Background rectangle
def add_background_rectangle(
self, color: Optional[Colors] = None, opacity: float = 0.75, **kwargs
self, color: str = None, opacity: float = 0.75, **kwargs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self, color: str = None, opacity: float = 0.75, **kwargs
self, color: Color | str | None = None, opacity: float = 0.75, **kwargs

@behackl
Copy link
Member

behackl commented Apr 15, 2022

@BrorSebastianSjovald @ludwigjo any updates here?

@MrDiver
Copy link
Collaborator

MrDiver commented Jun 18, 2022

Closing this for now. Making Colors the main source of truth again with exporting constants shouldn't break anything. colour.py has just too many duplicated features to our own implementation.

@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

None yet

Projects

Status: Rejected

Development

Successfully merging this pull request may close these issues.

6 participants