Skip to content

[DRAFT] New ManimBezier class, and new utils/linear_interpolation.py file #3336

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

Closed
wants to merge 6 commits into from

Conversation

chopan050
Copy link
Contributor

@chopan050 chopan050 commented Aug 18, 2023

Overview: What does this pull request change?

[DRAFT; SUBJECT TO CHANGES]
As discussed in the Discord channel, I propose taking all of the Bézier-related methods and functions, which are scattered all around the code (in utils/bezier.py, VMobject, Mobject, OpenGLVMobject, OpenGLMobject, etc) and gathering them all into a single class: ManimBezier (name subject to change, still wondering how to avoid ambiguity with the CubicBezier mobject).

This class has two subclasses, ManimQuadraticBezier (intended for OpenGLVMobject and OpenGLMobject, because OpenGL works better with quadratic Béziers) and ManimCubicBezier (intended for VMobject and Mobject, which currently use Cairo which requires cubic Béziers). Both of these subclasses attempt to implement the same methods, but in a different way depending on the degree of the Bézier.

I also moved the interpolate, integer_interpolate, mid, inverse_interpolate and match_interpolate functions (defined in utils/bezier.py) into a new file, utils/linear_interpolation.py, to separate them from the other Bézier methods. The reason is that these linear interpolations are ubiquitous all around the source code, and their use cases are much broader than with just "linear Béziers", so I thought they didnt' really belong that much among the other Bézier methods.

CURRENTLY THIS IS ONLY A DRAFT. This is far from being ready for review: there are still many, many changes I need to apply, methods in Mobjects to redirect to ManimBezier, functions to optimize, and all of that. I only publish my current changes so that everyone else can see what I'm planning to do. Any suggestions will be gratefully accepted!

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

  • Tidier code: as I said before, currently the Bézier methods and functions are scattered all around the code. I attempt to fix this by moving everything inside ManimBezier, ManimQuadraticBezier and ManimCubicBezier for convenience.
  • Bezier API: the idea of creating subclasses of ManimBezier is that Cairo Mobjects use instances of ManimCubicBezier, and OpenGL Mobjects use instances of ManimQuadraticBezier. Both of these subclasses implement the same methods, but in a different way depending on the degree of the curve. In this way, ManimBezier sort of exposes an API to Mobjects: they can just call the Bézier methods from their respective ManimBezier and they do not have to worry about the underlying implementation.
    • This avoids having functions such as split_quadratic_bezier vs. an hypothetical split_cubic_bezier, or subdivide_quadratic_bezier vs. an hypothetical subdivide_cubic_bezier: they would be just calledsplit_bezier and subdivide_bezier.
    • We could also avoid directly hardcoding cubic Bézier logic in VMobject and quadratic Bézier logic in OpenGLMobject, as happens in methods such as VMobject.set_anchors_and_handles, where there are 4 hardcoded parameters rather than a variable amount of parameters.

Currently, I'm also adding the changes I proposed in #3281 and #3292.

Links to added or changed documentation pages

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

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@chopan050 chopan050 added refactor Refactor or redesign of existing code Proposal a fully discussed feature or topic regarding manims future which can be worked on immediatly labels Dec 21, 2023
@JasonGrace2282
Copy link
Member

Closing due to inactivity and as per internal discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal a fully discussed feature or topic regarding manims future which can be worked on immediatly refactor Refactor or redesign of existing code
Projects
Status: Rejected
Development

Successfully merging this pull request may close these issues.

2 participants