Enabled filling color by value for :class:.OpenGLSurface, replaced colors keyword argument of :meth:.Surface.set_fill_by_value with colorscale

[alembcke]

This pull requests closes #2173 .

Overview: What does this pull request change?

Enables coloring OpenGLSurface gradient colors by z-value, by passing in a list of colors and an axes when a OpenGLSurface is initialized. The method OpenGLSurface.set_fill_by_value() has been deprecated, as it never worked properly for OpenGLSurface.

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

Adds similar functionality as Surface.set_fill_by_value(), but for OpenGLSurface. Here is an example:

And the code used to generate this example:

from manim import *
from manim.opengl import *

import numpy as np


class Test(ThreeDScene):
    def construct(self):
        resolution_fa = 100
        self.set_camera_orientation(phi=75 * DEGREES, theta=-60 * DEGREES)

        axes = ThreeDAxes(
                x_range=(-3, 3, 1),
                y_range=(-3, 3, 1),
                z_range=(-5, 5, 1)
                )

        def param_trig(u, v):
            x = u
            y = v
            z = 2 * np.sin(x) + 2 * np.cos(y)
            return z

        trig_plane = OpenGLSurface(
            lambda x, y: axes.c2p(x, y, param_trig(x, y)),
            resolution=(resolution_fa, resolution_fa),
            u_range = (-3, 3),
            v_range = (-3, 3),
            axes = axes,
            color = [BLUE, GREEN, YELLOW, ORANGE, RED],
            opacity = 0.8
        )

        self.add(axes, trig_plane)

        self.interactive_embed()

Links to added or changed documentation pages

Further Information and Comments

The method OpenGLSurface.set_fill_by_value() has been deprecated as it never worked properly. The same method worked for Surface because Surface is made up of many small squares lined up next to each other, so it could color each square a different color and it would give a gradient effect. But OpenGLSurface is made up of triangles, which are fused together to create a single mobject. So to color a OpenGLSurface it is necessary to pass in the colors to the shaders, along with the vertex data at the time the surface is initialized.

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]

Looks good to me in general, thank you for your contribution! I've approved the PR, but also added some minor comments that I would like to discuss before having this merged.

[behackl]

LGTM!

[hydrobeam]

Left some comments about typing/implementation. But this is definitely a step in the right direction!

[hydrobeam]

IMO it's odd to have the surface mobject be related to an axes mob. I think it would make more sense to have a method in coordinate_system.py that ties in the axes (maybe a plot_surface similar to plot after #2187) , rather have it be a parameter in this mob?

[alembcke]

I have implemented axes.plot_surface(), but am unable to find a way to not make axes an attribute of OpenGLSurface.

[hydrobeam]

What exactly do these pivots do? I'm asking because I think it's a bit odd for axes to be involved in colouring the surface

[alembcke]

I will address your other questions too, but wanted to start with this one as it may explain some of the above.

The pivots are where the colors transition from one color to another color. So if we define the colorscale to be:

SURFACE_COLORS = [
        (BLUE, 0),
        (GREEN, 2),
        (YELLOW, 5)
    ]

some_surface = OpenGLSurface(color=SURFACE_COLORS)

Then where the surface has z=0 the surface at that point will be exactly BLUE. Likewise, at z=2 the surface will be exactly GREEN. And in between those two z-values the surface will be a linear gradient between blue and green. This is handy when you want to do something like having z-values below zero be one color and z-values above zero be another color. Also, notice the pivots do not have to be equally spaced. And if the user defines pivots that do not encompass the entire range of z-values, for example if the above surface ranged from z=-2 to z=8, then the values above or below the ending pivots will be colored a solid with the color of the ending pivot (meaning z-values below 0 will be solid BLUE and z-values above 5 will be solid YELLOW). And if you don't define pivots and only pass a list of colors, then it will define the pivots to be equally spaced over the range of the z-axis passed in from axes.

The reason axes is needed is how else would you know what the z-value is? The values available for the surface are the points on the screen, not the coordinates in an axes. So you pass in the axes in order to know what the z-values are. I wouldn't be against making this a method in coordinate_systems.py, as you suggest in a comment above, but would want to make sure others agree.

[alembcke]

@behackl and @hydrobeam, I have been thinking a lot about the implementation of this enhancement and the concerns raised. I think it would be best if I spent some time completely re-doing the implementation. So with that in mind, I have some questions for you, or anyone else who is interested:

1. I agree that this should be moved to coordinate_systems.py within ThreeDAxes as the method ~.plot_surface(). I will switch between Surface and OpenGLSurface based on which renderer is being used. Is everyone happy with that idea?

2. How should colors and pivots be set by the user? I have gone with a list of tuples approach, but would it be best to separate these out into two variables? There are other alternatives, for example Plotly has the user pass in a colorscale using the argument colorscale and then the pivots are set using cmin, cmax and cmid. Personally, I prefer to allow the user to set each pivot for maximum control, but am open to other ideas.

3. The last part of the above question raises another one, should there be a new variable for colorscales of not just surfaces, but any MObject or VMObject? If so, should it be called colorscale or something else?

4. Should I also look to change the implementation of Surface.set_fill_by_value() to align it with what we are doing here? Making it a part of ~.plot_surface() and deprecate ~.set_fill_by_value()?

For me, the key functionality is the ability to apply a colorscale by z-value to surfaces and to be able to have below zero be one color and above zero be another color (I am making finance videos, so want to show below zero as red for loss and above zero as green for profit).

Feel free to add anything I may have missed.

[alembcke]

I have made the below changes:

1. Added axes.plot_surface() which switches between Cairo and OpenGL versions of Surface and includes colorscale as an argument.

2. Modified OpenGLSurface.set_color_by_value() and Surface.set_color_by_value() to use colorscale instead of color as the input. I kept a list of lists/tuples as the method for manually setting pivot points, as I think that makes the most sense.

3. Added tests for the Cairo version of axes.plot_surface(). I checked for the OpenGL file to add the tests, but it doesn't exist (nor do any of the graphical tests) and the OpenGL tests look different from the Cairo test, so wasn't sure how to implement this.

I hope this resolves most of the issues raised. One issue that I could not solve was not making axes an attribute of OpenGLSurface. The reason being that the way the shader data is populated is that OpenGLSurface.get_shader_data() is called from the parent Mobject class. But let me know if I missed something.

Edit: Almost forgot to mention, when I tried to use self.get_axes() to retrieve the ThreeDAxes Mobject calling axes.plot_surface() it would return a VGroup rather than a ThreeDAxes Mobject. That is why I have axes passed into axes.plot_surface() for now. Do you know of a way to fix this, or was I using ~.get_axes() wrong? If so, I will change that.

Ignore the above, used self.copy(), so no longer need to pass axes into .plot_surface().

[alembcke]

Rebased to account for PR #2476 and cleaned up the docstring.

Edit: Example now renders in the docs:
https://manimce--2186.org.readthedocs.build/en/2186/reference/manim.mobject.graphing.coordinate_systems.CoordinateSystem.html#manim.mobject.graphing.coordinate_systems.CoordinateSystem.plot_surface

[behackl]

I've added one commit properly deprecating the color parameter Surface.set_fill_by_value to avoid introducing a hard breaking change, please cross-check! Otherwise this still looks fine to me, if someone using the OpenGL renderer a bit more than myself could try to run this locally too it would be much appreciated.

Thanks for your contribution!

raised issues have been addressed