Skip to content

Refactoring Scene.mobjects #358

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

Open
leotrs opened this issue Aug 26, 2020 · 6 comments
Open

Refactoring Scene.mobjects #358

leotrs opened this issue Aug 26, 2020 · 6 comments
Assignees
Labels
refactor Refactor or redesign of existing code

Comments

@leotrs
Copy link
Contributor

leotrs commented Aug 26, 2020

Currently, Scene.mobjects is implemented as a list, and the Scene class does a lot of bookkeeping in order to keep it neatly organized. This is necessary because mobjects must be kept in the order that mobjects are going to be rendered on screen.

Or at least that was the case, until z-index was implemented #117. It is not without bugs (#327) but I think that keeping the z-order of each mobject is far better than trying to keep the list organized.

The truth is that Scene.mobjects should never have been a list. There main reason is that you cannot just choose to append something to it. You have to use restructure_mobjects every time you touch it. This is done so that mobjects are kept in the right order, but also to avoid duplication, deal with VGroups, etc. So, Scene.mobjects is implemented as a list but it's never used as one. Sounds familiar? This means that Scene.mobjects should be its own class that handles all of these operations. If Scene.mobjects were a different class, then any dev working on Scene-derived classes will never have to think about whether to use append, add, or when to call restructure_mobjects.

In my mind I can think of a few things to do here:

  1. Extract all of the Scene.mobjects logic and define a new class OrderedMobjectList or something along those lines.
  2. Inside the new class, the mobjects collection need not be a list. I think it should be a priority queue instead, where the priority values are the z-order of each mobject.
  3. (Maybe) I'm not sure if mobjects needs to ever contain a Group or VGroup. I cannot find a reason why self.add(some_group) could't just add each element in the group to the scene, instead of adding the group itself to the scene. Does a Scene really need to keep track of which objects are grouped together? If the only reason to do this is to keep track of rendering order, we already have z-index for that!

Please share your thoughts.

@leotrs leotrs changed the title Can Scene.mobjects be a set? Refactoring Scene.mobjects Aug 26, 2020
@leotrs leotrs self-assigned this Aug 26, 2020
@leotrs leotrs added the refactor Refactor or redesign of existing code label Aug 26, 2020
@huguesdevimeux
Copy link
Member

Everything seems very good to me. Mobjects handling is very, very messy.

Does a Scene really need to keep track of which objects are grouped together?

And I don't think so, as every time manim extract every family members to add them to scene.mobjects.

@leotrs
Copy link
Contributor Author

leotrs commented Aug 27, 2020

Tagging the devs as this is a major project that requires consensus from the community @ManimCommunity/core

@MysaaJava
Copy link
Contributor

But how do you know which mobject is in front of the other if z_index are the same ?

@leotrs
Copy link
Contributor Author

leotrs commented Sep 15, 2020

The point is that Scene.add would assign a new z_index to an object when it is being added. So by default there are no mobjects with repeated z_index.

If you then manually change the z_index of a mobject that happens to clash with that of another one, then they can each be rendered in an arbitrary order OR the tie can be broken by the time of addition (the oldest added mobject gets rendered first).

@MysaaJava
Copy link
Contributor

And if, in the future, you want to put a method to put an object between two other. Would you have to use floating point zindex ? More commonly, if you want to put a .moveBehind() method, how would you do that ? I think that a dedicated data structure (maybe a tree) would allow to move objects to the front or to the back easily. Of course, it would be compatible with z-index methods.
What do you think ?

@leotrs
Copy link
Contributor Author

leotrs commented Sep 18, 2020

Yeah I think there's no problem in using floating point indices. As long as there is a unique way of sorting the mobjects, you should be able to implement "move behind" and "move to the back".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor or redesign of existing code
Projects
Status: 🆕 New
Development

No branches or pull requests

3 participants