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

Improve line rendering performance by decreasing redundant subdivision count #3893

Merged
merged 9 commits into from
Oct 20, 2024

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