Bugfix: Apply Updaters to Graph Edges During Edge Creation

[tlcyr4]

Overview: What does this pull request change?

This fixes a problem where edges added to a graph after the graph's initial creation don't track their vertices' position until the end of the creation animation.

https://github.com/ManimCommunity/manim/assets/12660397/7c4336b1-27fc-4bd7-86c1-531f97fab3b5

The issue here is a consequence of how parallel updates work during some animations like Create, where we create a defensive copy of the mobject and draw a progression of interpolations starting from a blank canvas and working back toward that defensive copy.

If there's another set of updates that we need to apply to this mobject in tandem with the animation, we apply them to the defensive copy, which was created just before updates were suspended on the original mobject.

This can cause problems when the mobject has a parent mobject that's also involved in the animation.

First, in this Graph case, the Edge's updater was actually attached the the parent Graph (it was one big updater that kept all the Graph's Edges on their vertices). The animation that applied to the Graph was an AnimationGroup, which doesn't directly call updaters on its target mobject, and instead relies on its child Animations to apply updaters on their targets. However, the child Animation, a Create(Line), applied to the Edge rather than the Graph, so nothing was calling updaters for the Graph itself until the very end of the animation.

Second, when updates were suspended for the Graph, that operation was applied recursively to all submobjects of the Graph. Since this happened before the defensive copy of the Edge was created, that defensive copy inherited the "updating_suspended=True" flag, which blocked all updates from applying to the Edge until the end of the overall animation.

To resolve the above, this PR makes two changes:

1. Split out the Graph's global edge updater into separate updaters for each Edge, and attach them directly to those edges.
2. Explicitly set "updating_suspended=False" when spawning these "starting_mobject" defensive copies. I think the intent when creating the starting_mobject is just to inherit color, position, etc, and not metadata like updating_suspended.

This doesn't help with other cases where we attach a child's updater to the parent, but the general idea of an updater affecting a mobject other than the one it's directly attached to, regardless of parental relationship, seems like a whole can of worms to me.

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

bugfixes

Links to added or changed documentation pages

Should not affect any documentation.

Further Information and Comments

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

[tlcyr4]

Addresses #3584

[behackl]

Thanks for your efforts! I have not reviewed your changes made to GenericGraph yet in detail, but it looks like you substantially cleaned up the part where config dicts are assembled to take the correct shape.

IIRC, there was a reason why I went with one global graph-updater instead of one updater per edge -- perhaps it was performance. Things have changed quite substantially since the initial implementation though, so perhaps this isn't really relevant anymore; I'll run some tests.

I actually also think that AnimationGroup should in that case also explicitly call its starting_mobject's and/or group's updaters; for this to be useful in this particular situation it would, however, also require to specify the graph as the AnimationGroup's group (which actually already happens in the construction of the animation in GenericGraph._add_edges_animation). I am not sure whether this would be enough to make your fix also work with the single Graph-level updater though (which is just what I'd prefer semantically; I like the idea of the Graph being responsible for keeping the edges attached to its vertices, instead of the edges having to track their sibling mobjects).

I'll look into this some more. The failing tests are likely due to a different version of cairo on your end. If this is the case, I can regenerate it for you to make the pipeline pass.

Again, thanks for your efforts!

[tlcyr4]

Not sure why that last test I wrote keeps acting up. It's failing locally for me now, even right after a --set_test. I'll have to look into that.

I see what you're saying with tracking sibling mobjects being just as weird if not weirder than applying to a parent. If you want to go down the road of applying the update from the AnimationGroup/Graph, the change that looks interesting to me is removing the update suspension from AnimationGroup's begin(), since all the child animations suspend their own mobjects' updates anyway, and suspending animation on the group/parent overall is what blocks updates to parent mobjects like the Graph.

I made a quick and dirty draft PR of this https://github.com/ManimCommunity/manim/pull/3838, and it seems to pass my original test (creating an edge while moving a vertex), without breaking any existing tests.

[tlcyr4]

In the meantime, I just pushed a lighter version of this change that still cleans up the config dict shaping, which fixes two problems where

1. tip_configs for edges added after initial creation were not applied
2. edges in digraphs added after initial creation don't have tips at all

[behackl]

I've applied the suggestions from my review. It does seem like these changes here are no longer sufficient to resolve #3584 -- this is now addressed in #3838, right? @tlcyr4

I'm happy to merge this here as-is just because the initialization of Graphs is much cleaner.

[JasonGrace2282]

Before merging, I would prefer the docs build passes - does the example need to be changed?