-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
3948 update documentation for sphere and add overlap example #3973
base: main
Are you sure you want to change the base?
3948 update documentation for sphere and add overlap example #3973
Conversation
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 for this PR! I left comments where I request some changes.
This example shows that overlapping spheres can intersect with rough transitions. | ||
.. manim:: ExampleSphereOverlap |
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.
Add an extra line in between, so that Sphinx renders the example correctly.
This example shows that overlapping spheres can intersect with rough transitions. | |
.. manim:: ExampleSphereOverlap | |
This example shows that overlapping spheres can intersect with rough transitions. | |
.. manim:: ExampleSphereOverlap |
u_range=[0, TAU], | ||
v_range=[0, PI] |
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.
The u_range
and v_range
default values are already [0, TAU]
and [0, PI]
, so we might as well simply remove these explicit arguments if the intention of the original example was to actually use u
in [0, TAU]
and v
in [0, PI]
.
The original example is, indeed, weird. Why did it use u_range
and v_range
in the first place? Maybe the idea could have been to show, for example, half a sphere by setting v_range=[0, PI/2]
.
In order to showcase the proper use of u_range
and v_range
, may I ask you to please create a 3rd example where you use a different value for these parameters? Then, we could simply remove u_range
and v_range
from here.
u_range=[0, TAU], | |
v_range=[0, PI] |
Overview: What does this pull request change?
This pull request resolves 3948, by changing the misleading u_range and v_range of the example. I also added a overlap example.
Motivation and Explanation: Why and how do your changes improve the library?
This resolves 3948
Links to added or changed documentation pages
ExampleSphere
Reviewer Checklist