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

Remove default graph edge z_index of -1 #3588

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gyoge0
Copy link

@gyoge0 gyoge0 commented Jan 19, 2024

Overview: What does this pull request change?

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

Links to added or changed documentation pages

Further Information and Comments

Trying to create graph edges individually in an AnimationGroup results in the animation not playing and the finished state just popping on screen after the animation duration ends (#3584). Looking around, #2594 introduced a default z_index of -1 for all edges in graphs. This was brought up in #3132 but was never resolved. I've reintroduced the changes, removing the default z_index from the Graph class and its inheritors.

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
Copy link
Member

behackl commented Jan 20, 2024

Simply removing the non-default z_index is not a great solution. We need to ensure that edges are drawn before vertices (which is the reason why we are currently setting the z_index).

When a new Graph is created, we ensure that this happens regardless of the z_index by adding the corresponding submobjects in a certain order (first edges, then vertices) to Graph.submobjects. If we manipulate the initially created graph by calling add_edges or add_vertices, however, this initial separation into vertices and edges is no longer enforced; new edges are simply appended at the end of the submobjects list, after the initial edges.

Setting the z_index is a convenient solution for forcing the correct order during rendering -- with, apparently, unfortunate downsides. It will need a somewhat more sophisticated solution to resolve this; we're open for suggestions.

@gyoge0
Copy link
Author

gyoge0 commented Jan 20, 2024

Could we keep track of the vertex count and insert new vertices in the middle of Graph.submobjects? I suppose this would work if vertices are only added via Graph.add_vertices().

@behackl
Copy link
Member

behackl commented Jan 22, 2024

Sounds like a good approach. It would make sense to check at which points submobjects are Graph.added to a graph -- but I think if we make sure that the methods in graph.py work correctly, we could drop the z_index.

insert and remove graph vertices between existing vertices and edges to ensure the correct rendering order
@gyoge0
Copy link
Author

gyoge0 commented Feb 9, 2024

Any progress on this?

@MrDiver
Copy link
Collaborator

MrDiver commented Apr 1, 2024

If you made progress on this, you might want to comment it!

@gyoge0
Copy link
Author

gyoge0 commented Apr 5, 2024

I went through and ensured methods that add vertices to the Graph insert them before all the edges, so removing z index shouldn't break anything. The test scene from the issue renders properly now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

3 participants