Bug in handling of z_index

[leotrs]

Here's a Scene

from manim import *

class SquareToCircle(Scene):
    def construct(self):
        circle = Circle().set_fill(RED, opacity=1)
        square = Square(side_length=1.7).set_fill(BLUE, opacity=1)
        square.z_index = circle.z_index - 1

        self.play(FadeIn(VGroup(circle, square)))  # all good
        self.play(ApplyMethod(circle.shift, UP))   # woops!
        self.wait(1)

And here's the output:

As you can see, the square comes on top of the circle at the end of the second animation. I was expecting the square to remain behind the circle throughout the video because of the line square.z_index = circle.z_index - 1. Is this intended behavior? Is this a known issue?

FWIW, I think currently there are two different systems living in manim. First, the Scene and Camera classes try to keep the mobjects in order, and then the order of rendering on screen is (or, used to be) taken from that order. But after the introduction of z_index in #122, all of that bookkeeping to try to maintain the order is unnecessary since at the last minute the mobjects are being sorted by their z_index. I believe it is the interaction of these two things that is causing the bug. We can perhaps take this opportunity to deprecate all of those bookkeeping operations and stick to the z_index.

cc @tony031218 as author of #122

[TonyCrane]

Sorry to see this issue too late.

I found the cause of this problem. It's not because Scene and Camera want to keep the mobjects order conflict.
This is because you added a VGroup containing square and circle to the scene, so scene.mobjects only contains one VGroup, so it will not be sorted in Camera.extract_mobject_family_members. And the submobjects in a VGroup will not be sorted by z_index, so the result of the layer is invalid.

I am fixing this bug, I will open a pull request after I finish it. Thank you for your feedback.

The following are some relevant actions during testing (you can find log files and output videos on them):
https://github.com/Tony031218/manim_projects/runs/1018365118
https://github.com/Tony031218/manim_projects/runs/1018414557

[TonyCrane]

I have a little trouble fixing this bug.
I tried to use a recursive method to sort all the mobject and their submobjects in scene.mobjects by z_index, but this would completely change the order structure inside a VGroup and cause even greater wrong display results.

You can find a video of the results of my just experiment here:
https://github.com/Tony031218/manim_projects/actions/runs/220811226
Then these are the changes I made:
manim-kindergarten/manim@ver.MK...Tony031218:ver.MK

btw, I am about to start a new week of busy learning, so I may have to wait a week before trying to fix this bug again.

And as for the problem you mentioned in this issue, you can just FadeIn the circle and the square instead of a VGroup of them. That is, use:

self.play(FadeIn(circle), FadeIn(square))

instead of:

self.play(FadeIn(VGroup(circle, square)))

[leotrs]

@tony031218 , thanks for looking into this and for providing alternatives. Something tells me that we'll have to think long and hard about this one!

[pdcxs]

Changing the library codes will cause inelegant repeat flag variables and introduce strange bugs. I think the easiest way is sorting the objects when add them into a VGroup.

class SquareToCircle(Scene):
    def construct(self):
        circle = Circle().set_fill(RED, opacity=1)
        square = Square(side_length=1.7).set_fill(BLUE, opacity=1)
        square.z_index = circle.z_index - 1

        self.play(FadeIn(VGroup(*sorted([circle, square],
            key=lambda m: m.z_index))))
        self.play(ApplyMethod(circle.shift, UP))
        self.wait()

If you want to add other elements to the group, you can use the following method:

class GroupLayerTest(Scene):
    def construct(self):
        circle = Circle().set_fill(ORANGE, opacity=0.5)
        square = Square().set_fill(BLUE, opacity=1)
        circle2 = Circle().scale(2).set_fill(RED, opacity=1)

        circle.z_index = 1
        square.z_index = 0
        circle2.z_index = -1

        group = VGroup(*sorted([circle, square],
            key=lambda m: m.z_index))
        
        self.play(FadeIn(group))

        group.add(circle2)
        group.submobjects = sorted(group.submobjects,
            key=lambda m : m.z_index)

        self.play(FadeIn(circle2))
        self.play(group.shift, UP)
        self.wait()

[eulertour]

This kind of makes me wonder whether adding the z-index attribute was ever the right choice to begin with. After all each mobject actually has a z-coordinate, so we could just use s.shift(OUT ).

But even that leaves ambiguity in how to render since you could do something like

g = VGroup(
    Circle().shift(OUT),
    Square().shift(IN),
).shift(IN)

and it doesn't answer the question as to how to render mobjects with the same z-coordinate at all.

[pdcxs]

In my opinion, the best way to render the scene is to render objects according to the z coordinate. Objects with the same z coordinate should be rendered according to the order that they are added to the scene.

[eulertour]

A few things to consider:

1. It's possible for a mobject to have different z-coordinates for different points, but I don't even know if cairo can even handle such mobjects, and it probably doesn't make sense to do anyway.

2. A mobject's position in the scene can change depending on whether or not you consider a group containing the mobject to determine its position.

[leotrs]

I agree we should probably find a solution that uses the Z coordinate instead of a z index, or some other arbitrary ordering.

For 3D scenes, we will have the problem of occlusion (which is already documented in other issues). Cairo can handle objects whose points have different Z coordinates just fine. What it cannot handle is one object occluding only part of some other object. But that's a Cairo limitation, not manim's.

[behackl]

By changing extract_mobject_family_members to sort all extracted mobjects, rather than just the passed ones (which doesn't affect mobjects which are contained within groups), i.e.

diff --git a/manim/utils/family.py b/manim/utils/family.py
--- a/manim/utils/family.py
+++ b/manim/utils/family.py
@@ -28,6 +28,7 @@ def extract_mobject_family_members(
         method = Mobject.family_members_with_points
     else:
         method = Mobject.get_family
+    extracted_mobjects = remove_list_redundancies(list(it.chain(*[method(m) for m in mobjects])))
     if use_z_index:
-        mobjects = sorted(mobjects, key=lambda m: m.z_index)
-    return remove_list_redundancies(list(it.chain(*[method(m) for m in mobjects])))
+        return sorted(extracted_mobjects, key=lambda m: m.z_index)
+    return extracted_mobjects

I get

for @leotrs's example from the original post. Which looks fine to me (?)

What is the drawback of reordering the list of extracted mobjects? The sorting is stable, i.e., doesn't change the order of mobjects with the same z_index, and it is not like the list returned by extract_mobject_family_members is really used by anything non-rendering related (at least it didn't seem otherwise after a quick search for extract_mobject_family_members -- I could be wrong though). Anyways: locally, our current test suite also passes with this change.