Properly respect submobjects when sorting by z_index

[behackl]

Motivation

The only option to reorder Mobjects in a Scene from foreground to background, apart from adding them in the desired order in the list of (sub)mobjects currently is z_index -- and it is not working particularly well, see #327.

Overview / Explanation for Changes

This PR changes the sorting of mobjects w.r.t. their z_index. Currently, only mobjects in Scene.mobjects and/or Scene.foreground_mobjects are sorted, and not all the submobjects contained in these families.

In terms of the example provided in #327 this means that while both circle and square have a z_index set, only the z_index of the VGroup containing both (which is added to the scene via the FadeIn animation) is considered when sorting. With this PR, the z_index of the objects themselves is considered.

As an example, a simple scene which first sets the z_index of a square, a triangle and a circle to 0, 1, and 2, respectively, renders as

ZIndexTest.mp4

with this change, and as

ZIndexTest.mp4

on current master.

Note: This change should rather be seen as a quick bugfix; we should still revisit the entire idea of z_index (and, e.g., replace it by sorting by the z coordinate or so).

Oneline Summary of Changes

- Fixed: `z_index` of mobjects contained in others as submobjects is now properly respected (:pr:`872`)

Testing Status

See rendered example above, I've also added it as a graphical unit test (because the current, wrong, behavior can be observed from the final frame.)

Acknowledgements

• I have read the Contributing Guidelines

Reviewer Checklist

• Newly added functions/classes are either private or have a docstring
• Newly added functions/classes have tests added and (optional) examples in the docs
• Newly added documentation builds, looks correctly formatted, and adds no additional build warnings
• The oneline summary has been included in the wiki

[huguesdevimeux]

SGTM.

Quick q : I saw that this is not recursive. Is it fine?

[huguesdevimeux]

Comment At some point all the iterable handling in manim sutff should definitely be changed, as this is .. hmm .. not pretty

[behackl]

Agreed, we should refactor this. Stuff concerning families is really weird.

[kolibril13]

Cool!
Thanks for this contribution, I am looking forward to use this feature.
A few things that come into my mind.
We have now a lot of functionality on how layers can get changed in manim:

1. The z_index
2. The order in which Mobjects are added on screen (self.add(m1,m2))
3. The functions self.bring_to_front(m1) and self.bring_to_back(m1)
4. self.mobjects[0],self.mobjects[1] = self.mobjects[1], self.mobjects[0]

And of course, there is the z variable, which is not related to the screen order right now.
So I think we should mention somewhere in the docs (maybe with a flowchart), which functionalities are related to each other.
One might now wonder why bring_to_front won't change the z_index.
But that's for the future, right now it is nice to have z_index behaving properly again.

[kolibril13]

I just tested this feature with this below script, and I think that MathTex has a problem with z_index right now:

from manim import *
config.background_color = WHITE
class MovingCameraOnGraph( Scene):
    def construct(self):
        logo_green = "#87c2a5"
        logo_blue = "#525893"
        logo_red = "#e07a5f"
        ds_m = MathTex(r"\mathbb{M}", z_index=12).scale(7) # <- Problem!
        ds_m.shift(2.25*LEFT + 1.5*UP)
        circle = Circle(color=logo_green,
                        fill_opacity=1,
                        z_index=7)
        square = Square(color=logo_blue,
                        fill_opacity=1,
                        z_index=5)
        triangle = Triangle(color=logo_red,
                            fill_opacity=1,
                            z_index=3)
        circle.shift(LEFT)
        square.shift(UP)
        triangle.shift(RIGHT)
        vgroup = VGroup(triangle, square, circle, ds_m).move_to(ORIGIN).scale(0.2)
        self.add(vgroup)
        m5 = Square(fill_opacity=1, fill_color=BLACK).set_z_index(0)
        self.add(m5)
        self.wait()

MovingCameraOnGraph.mp4

When I change MathTex to Tex, the error is still there, however when I change MathTex to Dot with z_index 12, I get this as expected:

[behackl]

SGTM.

Quick q : I saw that this is not recursive. Is it fine?

I think so: the method selected above gets all the contained submobjects recursively, so extracted_mobjects already is a flattened list of all mobjects in the scene.

[behackl]

Cool!
Thanks for this contribution, I am looking forward to use this feature.
A few things that come into my mind.
We have now a lot of functionality on how layers can get changed in manim:

1. The z_index
2. The order in which Mobjects are added on screen (self.add(m1,m2))
3. The functions self.bring_to_front(m1) and self.bring_to_back(m1)
4. self.mobjects[0],self.mobjects[1] = self.mobjects[1], self.mobjects[0]

While this might seem like a lot of options, 2, 3, and 4 all simply modify self.mobjects IIRC. The only actual alternative is 1, z_index -- and if you are in the situation where you would like the submobjects of some mobject to be created in a certain order, modifying self.submobjects is not an option, only z_index is.

[behackl]

I just tested this feature with this below script, and I think that MathTex has a problem with z_index right now:

Excellent catch! z_index was not properly propagated to TexSymbol; fixed via aa331cf.

[kolibril13]

While this might seem like a lot of options, 2, 3, and 4 all simply modify self.mobjects IIRC. The only actual alternative is 1, z_index -- and if you are in the situation where you would like the submobjects of some mobject to be created in a certain order, modifying self.submobjects is not an option, only z_index is.

Exactly, I just think we should mention this somewhere in the docs.

Excellent catch! z_index was not properly propagated to TexSymbol; fixed via aa331cf.

And a very quick fix ;)