-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
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.
for more information, see https://pre-commit.ci
I originally intended for this to be with the Cylinder class, but I forgot to take into account that cylinders are checkered as well. |
This prevents a breaking change where a tuple of resolution values passed to Line3D.resolution would no longer work. Also applies to Arrow3D.
You could try maybe scaling the figure up and seeing if it makes a difference, then? |
Here's the results for a more upscaled line (made by zooming the camera on test_Line3D). It failed because the line was actually checkered before, which changed values at the subpixel level. |
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:
Other than that, it looks good to me! Thanks for helping make Manim better ✨ |
Yeah I think it looks fine to me :) |
Updated now, sorry for the delay. |
There was a problem hiding this 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.
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