Skip to content

Conversation

@nitzanbueno
Copy link
Contributor

@nitzanbueno nitzanbueno commented Aug 4, 2024

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

Subdividing a line cylinder by its height adds no extra resolution -
since it's not checkerboarded, 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.
@nitzanbueno nitzanbueno changed the title Improve cylinder rendering performance by decreasing redundant subdivision count Improve line rendering performance by decreasing redundant subdivision count Aug 4, 2024
@nitzanbueno
Copy link
Contributor Author

nitzanbueno commented Aug 4, 2024

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.

This prevents a breaking change where a tuple of resolution values
passed to Line3D.resolution would no longer work.
Also applies to Arrow3D.
@nitzanbueno
Copy link
Contributor Author

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.
image

@JasonGrace2282
Copy link
Member

JasonGrace2282 commented Aug 4, 2024

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
Copy link
Contributor Author

nitzanbueno commented Aug 5, 2024

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:
image

And here's at 20X:
image

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
Copy link
Member

JasonGrace2282 commented Aug 12, 2024

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 ✨

@nitzanbueno
Copy link
Contributor Author

Sure! Here are the benchmarks.

This is the rendered image:
image

This one is from using a hundred 24X24 lines:
image

This one is from using a hundred 2X24 lines:
image

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

@JasonGrace2282
Copy link
Member

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

@nitzanbueno
Copy link
Contributor Author

Updated now, sorry for the delay.

Copy link
Member

@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.

Thanks! Let's get this merged as soon as ci is fixed.

@chopan050 chopan050 enabled auto-merge (squash) October 20, 2024 00:36
@chopan050 chopan050 merged commit ce1fff6 into ManimCommunity:main Oct 20, 2024
18 checks passed
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.

3 participants