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

Fix DiGraph edges not fading correctly on FadeIn and FadeOut #3786

Merged
merged 7 commits into from
Jun 22, 2024

Conversation

Nikhil-42
Copy link
Contributor

@Nikhil-42 Nikhil-42 commented May 29, 2024

Overview: What does this pull request change?

This pull request resolves issue #3749 in which edges do not FadeOut properly when using DiGraph. The default updater in the DiGraph class used the edge constructor .become to attach the edges to the vertices. I replaced the updater to function more similarly to the Graph updater which moves the endpoints of the Line. Also, noticed that Line::set_points_by_ends did not project onto the bounding box of start and end so I updated the behavior to match the constructor and the type hints to align with usage in the example scenes.

  • Documented behavior of Line::set_points_by_ends
  • Updated type hints in Line
  • Altered edge updaters in graph.py to use Line::set_points_by_ends instead of .become

Links to added or changed documentation pages

https://manimce--3786.org.readthedocs.build/en/3786/reference/manim.mobject.geometry.line.Line.html#manim.mobject.geometry.line.Line.set_points_by_ends

Further Information and Comments

Fixes #3749

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

@Nikhil-42 Nikhil-42 marked this pull request as ready for review May 30, 2024 04:51
Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Looks good in general, just one small comment.

manim/mobject/geometry/line.py Outdated Show resolved Hide resolved
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
@Nikhil-42
Copy link
Contributor Author

@chopan050 Thanks for the edit!

Side note: I'm interested in helping with the rewrite, do you know how I could get into that?

Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@chopan050
Copy link
Contributor

Side note: I'm interested in helping with the rewrite, do you know how I could get into that?

Any help is greatly appreciated! Simply join #dev-chat on the Manim Discord server, or the [manim/experimental] post on #development, and say that you're interested in helping. Somewhere in that post there's a list of the current tasks we have to do in experimental.

@chopan050 chopan050 changed the title Fixes #3749 Digraph FadeOut Fix DiGraph edges not fading correctly on FadeIn and FadeOut Jun 22, 2024
@chopan050 chopan050 enabled auto-merge (squash) June 22, 2024 20:16
@chopan050 chopan050 merged commit d5cdd79 into ManimCommunity:main Jun 22, 2024
19 checks passed
@JasonGrace2282
Copy link
Member

JasonGrace2282 commented Jun 23, 2024

Feel free to ping me on Discord (I'm still @JasonGrace2282 ) if you want me to walk you through where we are right now in the rewrite :)
I'm always glad to have more people interested :)
You can also take a look at #3817

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

Successfully merging this pull request may close these issues.

FadeOut on DiGraph does not work smoothly
3 participants