Improve line rendering performance by decreasing redundant subdivision count

nitzanbueno commented 3 months ago

Subdividing a cylinder by its height adds no extra resolution - all new rectangles would look the same as one long rectangle. Decreasing the default subdivision resolution to 2 reduces submobject count by 12x while sacrificing no quality.

Overview: What does this pull request change?

Decrease default cylinder height resolution to 2

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

I was rendering a 3D scene with a bunch of lines and initializing them took a very long time. I then noticed that lines (and cylinders in general) are subdivided to 24 rectangles by the height, not only the radius. This is wasteful because cylinders are straight - 24 small rectangles on one face of the cylinder will look identical to one long rectangle. Reducing the height resolution to 2 improves performance and submobject count 12x while sacrificing no quality at all.

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

nitzanbueno commented 3 months ago

I originally intended for this to be with the Cylinder class, but I forgot to take into account that cylinders are checkered as well. Lines aren't though, so I believe this will help reduce render times for people in my situation.

nitzanbueno commented 3 months ago

The tests still fail - I think it's because the opacity is rendered slightly differently. I can't tell the difference between the two pictures.

JasonGrace2282 commented 3 months ago

You could try maybe scaling the figure up and seeing if it makes a difference, then? I'm a little bit busy right now, but hopefully within the next week or so I'll be able to take a look at your PR.

nitzanbueno commented 3 months ago

You could try maybe scaling the figure up and seeing if it makes a difference, then? I'm a little bit busy right now, but hopefully within the next week or so I'll be able to take a look at your PR.

Here's the results for a more upscaled line (made by zooming the camera on test_Line3D).

Here's at 10X:

And here's at 20X:

It failed because the line was actually checkered before, which changed values at the subpixel level. I believe you'll agree that this checkering isn't worth the performance drop (specifically on lines and arrows), and I personally think it makes more sense for the line not to be checkered.

JasonGrace2282 commented 3 months ago

Yeah, I just did some of my own testing, and came to the same conclusion as you. I think this PR should be fine - however, I do have some requests:

Add a note in the documentation about how to get the checkering pattern (I think there are some people who would prefer it despite the performance hit)
Send a benchmark of your scene (a screenshot of snakeviz output is good enough). It would be nice to see a performance comparison :)

Other than that, it looks good to me! Thanks for helping make Manim better :sparkles:

nitzanbueno commented 3 months ago

Sure! Here are the benchmarks.

This is the rendered image:

This one is from using a hundred 24X24 lines:

This one is from using a hundred 2X24 lines:

(three_dimensions.py:907(__init__) is Line3D.__init__.)

JasonGrace2282 commented 2 months ago

Yeah I think it looks fine to me :) Do you mind regenerating the test data?

nitzanbueno commented 1 month ago

Updated now, sorry for the delay.

ManimCommunity / manim