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

Added colorscale to axes.plot() #3148

Merged
merged 19 commits into from
Jul 25, 2024
Merged

Added colorscale to axes.plot() #3148

merged 19 commits into from
Jul 25, 2024

Conversation

alembcke
Copy link
Contributor

@alembcke alembcke commented Feb 4, 2023

Overview: What does this pull request change?

Added colorscale and colorscale_axis to axes.plot() to allow for colorscales by value for line plots.

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

Adds functionality to the library consistent with Surface and OpenGLSurface.

Links to added or changed documentation pages

https://manimce--3148.org.readthedocs.build/en/3148/reference/manim.mobject.graphing.coordinate_systems.CoordinateSystem.html#manim.mobject.graphing.coordinate_systems.CoordinateSystem.plot

Further Information and Comments

Example code:

from manim import *

class PlotExample(Scene):
    def construct(self):
        # construct the axes
        axes = Axes(
            x_range=[0, 6],
            y_range=[-2, 2],
            tips=False,
        )

        curve = axes.plot(
                lambda x: 2 * np.sin(x), x_range=(0.0, 6, 0.001), colorscale=[BLUE, GREEN, YELLOW, ORANGE, RED]
        )

        self.add(axes, curve)

PlotExample_ManimCE_v0 17 2

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

Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I have noticed that a visually similar behavior can already be achieved by setting the color of the plot to a gradient, and then specifying the sheen direction. Here is what I mean:

from manim import *

class PlotExample(Scene):
    def construct(self):
        # construct the axes
        axes = Axes(
            x_range=[0, 6],
            y_range=[-2, 2],
            tips=False,
        )

        curve = axes.plot(
                lambda x: 2 * np.sin(2*x),
                x_range=(0.0, 6, 0.001),
        )
        curve.set_color([BLUE, GREEN, YELLOW, ORANGE, RED])
        curve.set_sheen_direction(UP)

        self.add(axes, curve)

for setting the gradient along the y-axis, and the same code as above, but with a different sheen direction

        curve.set_sheen_direction(RIGHT)

for the gradient along the x-axis.

I still like the proposed interface, because it resembles the interface for 3D plots -- however, it might be worth it to implement it using these simpler, and already accessible methods; just from a point of making maintaining a little bit easier. What do you think?

manim/mobject/graphing/coordinate_systems.py Outdated Show resolved Hide resolved
@@ -588,6 +590,8 @@ def plot(
function: Callable[[float], float],
x_range: Sequence[float] | None = None,
use_vectorized: bool = False,
colorscale: Union[Iterable[Color], Color] | 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.

Passing a single color, which appears to be allowed based on this type hint, yields

ValueError: invalid literal for int() with base 16: ''

Also, the second option (passing an interable containing tuples of the form (color, pivot) is not captured by this type hint, this should be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the right way to do it?

colorscale: Union[Iterable[Color], Iterable[Color, float]] | None = None,

Copy link
Collaborator

@MrDiver MrDiver Mar 31, 2023

Choose a reason for hiding this comment

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

#2822 I think this can be typed a little bit more concise in the typing PR

But also it would just be ParsableManimColor in the future.

color: ParsableManimColor | list[ParsableManimColor] | None,

Co-authored-by: Benjamin Hackl <devel@benjamin-hackl.at>
@alembcke
Copy link
Contributor Author

alembcke commented Mar 9, 2023

What about the pivots? To me those are key as I set values below 0 to be one color and above 0 to be another color. Is it possible to set pivots with the sheen direction?

@behackl
Copy link
Member

behackl commented Mar 28, 2023

What about the pivots? To me those are key as I set values below 0 to be one color and above 0 to be another color. Is it possible to set pivots with the sheen direction?

True! I don't think our current implementation of gradients allows to manually set pivots. Maybe this is something that we could consider after we have #3020 and implement our own ManimColorGradient class... Until then, your "manual" implementation might be a more suitable way to go.

@alembcke
Copy link
Contributor Author

Added a test to round out this PR.

@behackl is there anything else I need to do to get this PR in shape to be merged? Or are we waiting on #3020 to be merged so that we implement ManimColorGradient? If so, any idea on when that would happen?

@MrDiver
Copy link
Collaborator

MrDiver commented Aug 4, 2023

It doesn't work with opengl renderer

  • CurvesAsSubmobjects doesn't seem to work in OpenGL

@MrDiver
Copy link
Collaborator

MrDiver commented Aug 4, 2023

Can i just say this is hellishly slow ? I am trying to render this and it takes about a second on a good run i think that might be problematic for a lot of cases if people really wanted to use it

grafik

That is the output i can currently achieve when i use it with opengl. I am not quite sure how your solution works but there might still be something missing.

grafik

We basically spend 90% of the time just creating mobjects

grafik

In cairo this doesn't happen for some reason. But 6 seconds for a singular curve is pretty slow in my opinion.

@MrDiver
Copy link
Collaborator

MrDiver commented Aug 4, 2023

It takes 155 seconds in cairo to draw this. I don't think this is acceptable tbh.
grafik

PlotExample.mp4

@MrDiver
Copy link
Collaborator

MrDiver commented Aug 4, 2023

I would suggest a solution which somehow exploits curve.set_stroke([BLUE, RED, GREEN, YELLOW]) because that might be the faster way to change the colors without making thousands of little mobjects when it's not needed.

@alembcke
Copy link
Contributor Author

alembcke commented Aug 5, 2023

@MrDiver the number of sub-mobjects created is up to the user. It comes from x_range=(0.0, 6, 0.001) in axes.plot(). I choose 0.001 to ensure a nice smooth transition of colors. But changing it to 0.1 still results in a decent transition of colors and takes a fraction of the time:

CurveExample_ManimCE_v0 17 3

Is that a reasonable solution? It still doesn't work in OpenGL, but that as you point out is because CurvesAsSubmobjects doesn't work in OpenGL and so the appropriate solution would be to get CurvesAsSubmobjects working in OpenGL in a separate PR.

As for using curve.set_stroke([BLUE, RED, GREEN, YELLOW]), I don't see how that can be done while allowing to set at least a mid-point for the colors. I say that as I often set certain colors for less than 0 and others for greater than 0. To me this is necessary and is already how Surface and OpenGLSurface work with colorscales (as well as other charting libraries like Plotly). I think it is worth it to discuss how colorscales should be implemented across all three, as there is some duplication of code. That is why I asked @behackl above about #3020 and implementing ManimColorGradient, as I would prefer that colorscales be implemented there and then each of these just use that implementation.

@MrDiver
Copy link
Collaborator

MrDiver commented Aug 5, 2023

I already got the OpenGL thing to work, that's why i have the flamegraph for the 95 seconds. That's what it takes to render this I still think it is a little bit much to create so many mobjects for that. As far as i understand it colors may be applied independently of the number of mobjects.

@MrDiver
Copy link
Collaborator

MrDiver commented Aug 5, 2023

@alembcke

class PlotExample(Scene):
    def construct(self):
        # construct the axes
        axes = Axes(
            x_range=[0, 6],
            y_range=[-2, 2],
            tips=False,
        )

        curve = axes.plot(
            lambda x: 2 * np.sin(x), x_range=(0.0, 6), use_vectorized=True
        )
        colors = [
            "#ff0000",
            "#ffa500",
            "#ffff00",
            "#008000",
            "#0000ff",
            "#4b0082",
            "#ee82ee",
        ]
        # curve.set_stroke(colors)
        curve.set_color(colors)
        # curve.set_fill(colors, opacity=1)
        self.play(Write(axes), Write(curve))
        self.play(curve.animate.set_sheen_direction(UP))
        self.play(curve.animate.set_sheen_direction(RIGHT))
        self.play(curve.animate.set_sheen_direction(LEFT))
        self.wait()

You might wanna take a look at this 👀 i just realized you can do that so it might make realizing the color stuff a lot easier because it is not dependend on the number of curves

PlotExample.mp4

@MrDiver
Copy link
Collaborator

MrDiver commented Aug 5, 2023

PlotExample.mp4

And you can scale the amount of colors without increasing the rendering time at all.

grafik
For 100 different colors the whole animation not just an image

Doing only an image:
grafik

So instead of generating all the curves and then iterating over them and setting all the fancy colors i would suggest just generating a color list and then setting the colors directly

Or we might just take a look at the implementation of that and see if we can borrow some color setting things such that you can get a custom pivot.

@MrDiver
Copy link
Collaborator

MrDiver commented Aug 5, 2023

I added some changes to the PR, you can check if they have the same behavior that you intended. I just reused all of your code. I also added a test to check if the color axis parameter actually works so that it doesn't get missed in the graphical tests.

@alembcke
Copy link
Contributor Author

alembcke commented Aug 5, 2023

Just to reiterate here what was discussed on Discord, I rendered a bunch of videos I have that use this functionality and they all behave the same way as before but render much faster.

Copy link
Collaborator

@MrDiver MrDiver left a comment

Choose a reason for hiding this comment

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

We need to review the Test data again because main changed something i suppose

@alembcke
Copy link
Contributor Author

This is weird because the tests pass on my machine. I even deleted the folder with Manim in it and did a git clone and reran the tests - they still pass.

I double and triple checked that I am on the line_gradient branch and I ran poetry install and poetry run pytest. Am I doing anything wrong here?

@chopan050 chopan050 added the new feature Enhancement specifically adding a new feature (feature request should be used for issues instead) label Dec 21, 2023
@chopan050
Copy link
Contributor

I double and triple checked that I am on the line_gradient branch and I ran poetry install and poetry run pytest. Am I doing anything wrong here?

Maybe you also needed to run poetry shell to enter your environment 🤔

As MrDiver said: the tests needed an update, because they stopped working after certain commit in main. Don't worry: I already updated the tests myself and your PR has passed all checks 😉

@alembcke alembcke requested a review from MrDiver February 2, 2024 12:15
@MrDiver MrDiver enabled auto-merge (squash) July 25, 2024 18:41
@MrDiver MrDiver merged commit 20d0194 into ManimCommunity:main Jul 25, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Enhancement specifically adding a new feature (feature request should be used for issues instead)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants