ManimCommunity / manim

A community-maintained Python framework for creating mathematical animations.
https://www.manim.community
MIT License
19.96k stars 1.48k forks source link

Making ManimColor an Immutable datatype #3455

Open MartinXPN opened 7 months ago

MartinXPN commented 7 months ago

Description of bug / unexpected behavior

I'm trying to run manim with Python 3.11 (3.11.6 more specifically). Yet, I get a ValueError telling me that ManimColor class has a mutable default for a field color.

Expected behavior

It should work without errors.

How to reproduce the issue

Code for reproducing the problem Place the following code in a file `main.py` ```py from manim import * class CreateThumbnail(Scene): def construct(self): text = Text( 'Some\nTitle', font_size=77 ).center().shift(3 * LEFT).shift(2 * UP) self.add(text) ``` Run the following command to compile: ```shell python -m manim main.py CreateThumbnail -qm -v WARNING ```

Logs

Terminal output ``` ... ... @dataclass ^^^^^^^^^ File "/opt/homebrew/Cellar/python@3.11/3.11.6_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/dataclasses.py", line 1230, in dataclass return wrap(cls) ^^^^^^^^^ File "/opt/homebrew/Cellar/python@3.11/3.11.6_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/dataclasses.py", line 1220, in wrap return _process_class(cls, init, repr, eq, order, unsafe_hash, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/opt/homebrew/Cellar/python@3.11/3.11.6_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/dataclasses.py", line 958, in _process_class cls_fields.append(_get_field(cls, name, type, kw_only)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/opt/homebrew/Cellar/python@3.11/3.11.6_1/Frameworks/Python.framework/Versions/3.11/lib/python3.11/dataclasses.py", line 815, in _get_field raise ValueError(f'mutable default {type(f.default)} for field ' ValueError: mutable default for field color is not allowed: use default_factory ```

System specifications

System Details - OS macOS 13.3 - RAM: 64GB - Python version (`python/py/python3 --version`): 3.11 - Installed modules (provide output from `pip list`): manim and numpy
FFMPEG Output of `ffmpeg -version`: ``` ffmpeg version 6.0 Copyright (c) 2000-2023 the FFmpeg developers built with Apple clang version 14.0.3 (clang-1403.0.22.14.1) configuration: --prefix=/opt/homebrew/Cellar/ffmpeg/6.0_1 --enable-shared --enable-pthreads --enable-version3 --cc=clang --host-cflags= --host-ldflags= --enable-ffplay --enable-gnutls --enable-gpl --enable-libaom --enable-libaribb24 --enable-libbluray --enable-libdav1d --enable-libjxl --enable-libmp3lame --enable-libopus --enable-librav1e --enable-librist --enable-librubberband --enable-libsnappy --enable-libsrt --enable-libsvtav1 --enable-libtesseract --enable-libtheora --enable-libvidstab --enable-libvmaf --enable-libvorbis --enable-libvpx --enable-libwebp --enable-libx264 --enable-libx265 --enable-libxml2 --enable-libxvid --enable-lzma --enable-libfontconfig --enable-libfreetype --enable-frei0r --enable-libass --enable-libopencore-amrnb --enable-libopencore-amrwb --enable-libopenjpeg --enable-libspeex --enable-libsoxr --enable-libzmq --enable-libzimg --disable-libjack --disable-indev=jack --enable-videotoolbox --enable-audiotoolbox --enable-neon libavutil 58. 2.100 / 58. 2.100 libavcodec 60. 3.100 / 60. 3.100 libavformat 60. 3.100 / 60. 3.100 libavdevice 60. 1.100 / 60. 1.100 libavfilter 9. 3.100 / 9. 3.100 libswscale 7. 1.100 / 7. 1.100 libswresample 4. 10.100 / 4. 10.100 libpostproc 57. 1.100 / 57. 1.100 ```
MartinXPN commented 7 months ago

I was wondering if it would be possible to make the colors inside manim immutable? Otherwise, it's not possible to use them in dataclasses as default values.

DeathEaterSam commented 7 months ago

Weird. Just installed python 3.11.6 and installed manim and numpy in a fresh venv. FFMPEG version's also the same. Ran your code and everything rendered fine. Try a pip install -U manim and see if that fixes it.

MartinXPN commented 7 months ago

Sorry for the confusion. Seems like my provided code was not complete. Here is the complete example:

from manim import *
from dataclasses import dataclass

@dataclass
class Array:
    values: list[int | None]
    color: str = WHITE       # <- This causes an issue because Python now requires the default values to be non-mutable

class CreateThumbnail(Scene):
    def construct(self):
        text = Text(
            'Some\nTitle', font_size=77
        ).center().shift(3 * LEFT).shift(2 * UP)
        self.add(text)
    # ...
DeathEaterSam commented 7 months ago

How does this work as a solution?


from manim import *
from dataclasses import dataclass
from typing import ClassVar

@dataclass
class Array:
    values: list[int | None]
    color: ClassVar[ManimColor] = WHITE       # <- No longer raises a ValueError

class CreateThumbnail(Scene):
    def construct(self):
        text = Text(
            'Some\nTitle', font_size=77
        ).center().shift(3 * LEFT).shift(2 * UP)
        self.add(text)
    # ...```
MartinXPN commented 7 months ago

It doesn't. What if we need several different arrays? This should be an instance variable.

behackl commented 7 months ago

color: str = WHITE

If you want to store it as a str, then the workaround would be to do so explicity: color: str = WHITE.to_hex().

Making ManimColor immutable is an interesting thought though. CC @MrDiver

MartinXPN commented 7 months ago

I would be happy to store it as another type as well. The problem is that it's currently mutable. So, the only way to have it in a dataclass is to have it with field(default_factory=lambda: WHITE). Which is not ideal especially when the colors can actually be immutable.

MrDiver commented 7 months ago

I mean i kind of wrote it immutable so there shouldn't be methods that change the color in place if i didn't miss anything, so what would be the thing that signifies it's immutable?

MartinXPN commented 7 months ago

Maybe making ManimColor a frozen dataclass might help? Currently, the colors are of type ManimColor, which is a class; therefore, the values of all the colors are mutable.

MrDiver commented 7 months ago

Let's see if I can manage to do this, I think the main problem will be getting the constructor to work. If you're on Discord it would be nice if you could join the dev-chat to maybe work out a solution that also works for different use-cases. Because i never really had the time to look at semantics of Dataclasses in that context.

MartinXPN commented 7 months ago

Just joined the Discord community. You should probably be able to accomplish everything that's done inside the current constructor with __post_init__().

MartinXPN commented 6 months ago

Hi @MrDiver are there any updates on this? Here are some thoughts that might be helpful:

What you wanna do is get rid of the constructor and place all the logic inside a __post_init__() That would ensure that the instance is initialized properly. You can create attributes inside it, etc.

I think from_xy should be a static method that returns a Color instance right? In that case, there shouldn't be any problems with the initialization

This might be helpful: https://chat.openai.com/share/511eb72a-c43f-442e-aa2a-8d48c9821c2e

This is the stack overflow question: https://stackoverflow.com/questions/53756788/how-to-set-the-value-of-dataclass-field-in-post-init-when-frozen-true

MrDiver commented 6 months ago

Hey, yes i was still experimenting with that and trying to find edge cases where we might run into problems see the HSL color class. This can also be done immutable.

So it seems like this should be fine as a long term change.

JasonGrace2282 commented 2 weeks ago

Another thought would be adding a __slots__.