Skip to content

Conversation

irvanalhaq9
Copy link
Contributor

@irvanalhaq9 irvanalhaq9 commented Aug 30, 2024

Overview: What does this pull request change?

This pull request refines the Sector class by removing the redundant parameters inner_radius and outer_radius, simplifying the constructor to accept only a single radius parameter. The inner_radius is now automatically set to 0, reflecting the true geometric nature of a sector.

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

Currently, the Sector class in Manim inherits parameters, including inner_radius and outer_radius, from the AnnularSector class.
This makes the Sector class exactly the same as the AnnularSector class. This is why the examples given in the Manim Documentation for the Sector class are examples of AnnularSector.

Actually, the Sector is conceptually a special case of AnnularSector where inner_radius should always be 0. This means that the Sector class should only require a single parameter, radius, which is then directly passed to outer_radius of AnnularSector.

Links to added or changed documentation pages

Further Information and Comments

The examples provided in the Manim documentation for the Sector class actually depict scenarios that are more representative of the AnnularSector class. This highlights the confusion caused by the redundant parameters.

  1. Current Documentation Example of Sector:
    Screenshot 2024-08-30 214305
    That example is similar to Example of AnnularSector:
    Screenshot 2024-08-30 214251

  2. Correct Sector Representation
    The following image depicts what a Sector should look like when only the radius is provided, with inner_radius set to 0. It accurately represents a sector, demonstrating how the simplified constructor correctly models this geometric shape.
    Screenshot 2024-08-30 215031

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

@JasonGrace2282 JasonGrace2282 added the breaking changes This PR introduces breaking changes label Aug 30, 2024
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.

This seems fine to me, thanks

@JasonGrace2282 JasonGrace2282 merged commit 76e6a57 into ManimCommunity:main Aug 30, 2024
18 checks passed
@irvanalhaq9
Copy link
Contributor Author

This seems fine to me, thanks

Thanks for reviewing and accepting the changes! I appreciate it.

@irvanalhaq9 irvanalhaq9 deleted the manim-sector-fix branch January 20, 2025 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking changes This PR introduces breaking changes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants