Changed `color`, `stroke_color` and `fill_color` attributes to properties #2332

marcin-serwin · 2021-11-26T21:10:58Z

Overview: What does this pull request change?

color, stroke_color and fill_color are now properties in VMobject, making them synchronized with set_* and get_* methods
Surrounding code depending on these attributes was also updated to work properly with them

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

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

behackl

I'm slightly hesitant to merge this – on the one hand, this is a nice quality of life improvement for Mobject, but at the same time it lets VMobject diverge more from OpenGLVMobject. Maybe the same attributes could be turned into properties for OpenGLVMobject as well?

behackl

This looks good now, thank you! It would be good to have someone else look over your changes as well; maybe @hydrobeam?

manim/mobject/types/opengl_vectorized_mobject.py

hydrobeam

Left some questions about some decisions here, also some minor comments.

hydrobeam · 2021-12-26T06:22:42Z

manim/mobject/mobject.py

@@ -78,6 +78,7 @@ class Mobject:
    """

    animation_overrides = {}
+    submobjects = []


What was the reasoning for adding this class level attribute? Also, why did you decide against this in opengl_mobject.py?

This was an ugly workaround similar to the one pointed out in the previous review. I've removed it.

hydrobeam · 2021-12-26T06:23:04Z

manim/mobject/mobject.py

+
+        self.color = Color(color) if color else None
+


NIT: odd spacing

Suggested change

self.color = Color(color) if color else None

self.color = Color(color) if color else None

hydrobeam · 2021-12-26T06:24:26Z

manim/mobject/svg/svg_mobject.py

@@ -76,6 +77,10 @@ def __init__(
        should_subdivide_sharp_curves=False,
        should_remove_null_curves=False,
        color=None,
+        *,


Is this a typo or intentional?

This is intentional, so as not to change the constructor signature. fill_color, stroke_color and stroke_opacity were keyword only before and they are keyword only now.

hydrobeam · 2021-12-26T06:29:16Z

manim/mobject/svg/text_mobject.py

@@ -1066,7 +1065,7 @@ def __init__(
        else:
            self.line_spacing = self._font_size + self._font_size * self.line_spacing

-        file_name = self.text2svg()
+        file_name = self.text2svg(Color(color) if color else None)


What happens if None is passed here? I'm not familiar with manimpango so I'm not sure if None could cause problems here

Since None is not a valid color it would default to black, but I've made it more explicit.

hydrobeam · 2021-12-26T06:34:03Z

manim/mobject/types/vectorized_mobject.py

        self.update_rgbas_array(array_name, color, opacity)
        if width is not None:
            setattr(self, width_name, width)
        if opacity is not None:
            setattr(self, opacity_name, opacity)
-        if color is not None:
+        if background:


Why switch this to depending on background? Also, it might be better to have it lumped in with the rest of the if background: attribute setting.

The old condition, color is not None, should also be present, I've fixed it now. It is depending on background because only then the color_name should be updated, otherwise it does not exist. I've made it more explicit, so hopefully the code is more understandable now.

hydrobeam · 2021-12-26T06:36:37Z

manim/utils/config_ops.py

@@ -56,10 +56,10 @@ def __set_name__(self, obj, name):
        self.name = name

    def __get__(self, obj, owner):
-        return obj.__dict__["data"][self.name]
+        return obj.data[self.name] if self.name in obj.data else []


Checking if an attribute is in data might prove to be too expensive and cause a performance dropoff, since data is called a lot of times (haven't tested, just speculation). Is there a way to get around doing this?

This turned out to be no longer necessary after fixing ugly workarounds described above.

hydrobeam · 2021-12-26T06:44:30Z

manim/mobject/types/opengl_vectorized_mobject.py

        self.stroke_opacity = stroke_opacity
-        self.stroke_width = stroke_width
+        self.stroke_width = [stroke_width]


Why did this need to be changed? Iirc set_stroke should be handling converting the float into a 2d-array to be placed into self.data

I've no idea why I've changed it. I've removed it and everything works fine.

Darylgolden · 2022-01-28T09:45:51Z

Sorry for the long wait! Can you resolve the merge conflicts?

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

Darylgolden

Left two minor comments about typings, otherwise LGTM!

Darylgolden · 2022-01-28T15:59:10Z

manim/mobject/svg/svg_mobject.py

@@ -9,11 +9,12 @@
 import re
 import string
 import warnings
-from typing import Dict, List
+from typing import Dict, List, Optional


These imports are not used.

Darylgolden · 2022-01-28T16:00:19Z

manim/mobject/svg/text_mobject.py

@@ -1225,7 +1225,7 @@ def _text2hash(self):
        hasher.update(id_str.encode())
        return hasher.hexdigest()[:16]

-    def _text2svg(self):
+    def _text2svg(self, color: Optional[Color]):


Suggested change

def _text2svg(self, color: Optional[Color]):

def _text2svg(self, color: Color | None):

stale

naveen521kk · 2022-02-06T17:26:18Z

Looks like this has broken t2s and t2w in Text https://docs.manim.community/en/latest/reference/manim.mobject.svg.text_mobject.Text.html#textitalicandboldexample

marcin-serwin · 2022-02-06T19:13:44Z

Looks like this has broken t2s and t2w in Text https://docs.manim.community/en/latest/reference/manim.mobject.svg.text_mobject.Text.html#textitalicandboldexample

I'll look at it tomorrow.

citrusmunch mentioned this pull request Nov 26, 2021

Mobject.set does not set VMobject attributes #2320

Open

hydrobeam added the pr:bugfix Bug fix for use in PRs solving a specific issue:bug label Nov 27, 2021

behackl reviewed Dec 2, 2021

View reviewed changes

marcin-serwin marked this pull request as draft December 3, 2021 14:34

marcin-serwin force-pushed the change-colors-to-properties branch from 9ddaf5f to eaf7f69 Compare December 3, 2021 15:06

marcin-serwin marked this pull request as ready for review December 3, 2021 20:03

marcin-serwin requested a review from behackl December 3, 2021 20:05

behackl approved these changes Dec 3, 2021

View reviewed changes

manim/mobject/types/opengl_vectorized_mobject.py Outdated Show resolved Hide resolved

marcin-serwin force-pushed the change-colors-to-properties branch from b958734 to 631f7ad Compare December 22, 2021 21:22

marcin-serwin requested a review from naveen521kk December 22, 2021 21:59

hydrobeam previously requested changes Dec 26, 2021

View reviewed changes

Darylgolden requested a review from hydrobeam January 14, 2022 01:54

marcin-serwin added 12 commits January 28, 2022 11:06

Make color, stroke_color and fill_color properties

b2084ee

Update code to properly use the stroke- fill- and color properties

ad82906

Make colors properties in OpenGL VMobject

c83e45f

Initialize svg mobjects with correct style

290dac6

Pass the color to hash calculating function directly

09ec113

Remove workaround with submobjects

a147528

Make text2svg handling of None color explicit

a705845

Fix background_stroke_color updating

e38d30c

Remove in data check

b388aa5

Remove typo ?

9128e56

Remove odd spacing

7a88f25

Fix issues with new text color settings

aeca1fa

marcin-serwin force-pushed the change-colors-to-properties branch from 5d15936 to aeca1fa Compare January 28, 2022 10:39

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

7b956a2

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

Darylgolden approved these changes Jan 28, 2022

View reviewed changes

Implement suggestions

035ae57

Darylgolden merged commit 717abfb into ManimCommunity:main Jan 28, 2022

naveen521kk mentioned this pull request Feb 6, 2022

t2s and t2w in Text is broken in main branch #2529

Closed

behackl changed the title ~~Change color, stroke_color and fill_color fields to properties~~ Changed color, stroke_color and fill_color attributes to properties Feb 20, 2022

behackl added the enhancement Additions and improvements in general label Feb 20, 2022

behackl changed the title ~~Changed color, stroke_color and fill_color attributes to properties~~ Changed color, stroke_color and fill_color attributes to properties Feb 20, 2022

k4pran mentioned this pull request Jul 23, 2022

Unable to pass list of stroke_color for opengl #2897

Open


	self.color = Color(color) if color else None
	self.color = Color(color) if color else None

	def _text2svg(self, color: Optional[Color]):
	def _text2svg(self, color: Color \| None):

Changed color, stroke_color and fill_color attributes to properties #2332

Changed color, stroke_color and fill_color attributes to properties #2332

Uh oh!

Conversation

marcin-serwin commented Nov 26, 2021 • edited by behackl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview: What does this pull request change?

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

Reviewer Checklist

Uh oh!

behackl left a comment

Choose a reason for hiding this comment

Uh oh!

behackl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hydrobeam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Darylgolden commented Jan 28, 2022

Uh oh!

Darylgolden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naveen521kk commented Feb 6, 2022

Uh oh!

marcin-serwin commented Feb 6, 2022

Uh oh!

Uh oh!

Changed `color`, `stroke_color` and `fill_color` attributes to properties #2332

Changed `color`, `stroke_color` and `fill_color` attributes to properties #2332

marcin-serwin commented Nov 26, 2021 •

edited by behackl

Loading