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

Created and optimized Bézier splitting functions such as partial_bezier_points() in manim.utils.bezier #3766

Merged

Conversation

chopan050
Copy link
Contributor

Overview: What does this pull request change?

Related PR: #3281

The original algorithm in partial_bezier_points() made a lot of calls to bezier(), making multiple redundant intermediate calculations that could've been reused for the other Béziers.

Instead, in the n-th degree case I calculate all of the necessary points in a single pass, improving from O(n³) to O(n²) time:

def partial_bezier_points(points, a, b):
    # Border cases...
    
    degree = points.shape[0] - 1
    if degree == 3:
        # calculate portion_matrix..
        return portion_matrix @ points
    # if degree in (2, 1, 0)...

    # Fallback case for nth degree Béziers
    # It is convenient that np.array copies points
    arr = np.array(points)
    N = arr.shape[0]
    
    if a != 0:
        for i in range(1, N):
            arr[: N - i] += a * (arr[1 : N - i + 1] - arr[: N - i])

    if b != 1:
        if a != 0:
            mu = (1 - b) / (1 - a)
        else:
            mu = 1 - b
        for i in range(1, N):
            arr[i:] += mu * (arr[i - 1 : -1] - arr[i:])

    return arr

Even more, the process can be encoded as a matrix multiplication, which is what I did for the simple cases (up to cubic curves), giving around 10x speedup:

    if degree == 2:
        ma, mb = 1 - a, 1 - b

        portion_matrix = np.array(
            [
                [ma * ma, 2 * a * ma, a * a],
                [ma * mb, a * mb + ma * b, a * b],
                [mb * mb, 2 * b * mb, b * b],
            ]
        )
        return portion_matrix @ points

This rewritten function can now replace partial_quadratic_bezier_points().

I applied the exact same process for creating split_bezier() and subdivide_bezier(), which are very similar and use essentially the same algorithm, and can also be encoded as matrix multiplications. Moreover, subdivide_bezier() creates memos for each generated matrix, to reuse them in future calls to the function. Currently split_bezier() isn't being used anywhere (originally subdivide_quadratic_bezier() used split_quadratic_bezier()), but I still leave it there because its docstring is useful and it might be used in the future for something else.

I also created bezier_remap(), based on quadratic_bezier_remap(), which uses subdivide_bezier(). I noticed that VMobject.insert_n_curves_to_point_list() uses an algorithm that could've been a call to bezier_remap(), so I used that instead.

All of this is thoroughly documented in docstrings, being the most important one the split_bezier() docstring, where the algorithm is explained in depth. The other docstrings are based on this explanation.

I also added many tests for many different cases.

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
Contributor

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@chopan050 chopan050 enabled auto-merge (squash) May 21, 2024 05:13
@chopan050 chopan050 merged commit 7b841c2 into ManimCommunity:main May 21, 2024
18 checks passed
@chopan050 chopan050 deleted the optimize-partial-bezier-points branch May 21, 2024 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants