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

Set corners of :class:~.Rectangle in counterclockwise direction #1249

Merged
merged 8 commits into from Apr 6, 2021

Conversation

abhijithmuthyala
Copy link
Contributor

@abhijithmuthyala abhijithmuthyala commented Apr 4, 2021

Changelog / Overview

This improves the look of transformations between rectangles and other simple mobjects.

Motivation

Here's how the Transformation from Square to Circle looks currently

Test.mp4

Notice how the Square gets flipped during the Transform.

Explanation for Changes

This is because the corners of Rectangles are set in clockwise direction

Polygon.__init__(self, UL, UR, DR, DL, color=color, **kwargs)
whereas for Arcs the points are set in counterclockwise direction
anchors = np.array(
[
np.cos(a) * RIGHT + np.sin(a) * UP
for a in np.linspace(
self.start_angle, self.start_angle + self.angle, self.num_components
)
]
)

This is what causes the Square to be flipped during the Transform. This PR changes the cw-orientation of Rectangle's corners (UL, UR, DR, DL) to ccw-orientation (UR, UL, DL, DR)

Testing Status

  • Local pass (Windows 10)
  • This is how the Transformation looks now
Test.mp4

Further Comments

This PR is inspired from Kolibril13's idea to enhance transformation. I've looked into the entire geometry.py module to check if any other Mobject needs this switch in orientation. Turned out it's just the Rectangle. Let me know if I missed something.

Checklist

  • I have read the Contributing Guidelines
  • I have written a descriptive PR title (see top of PR template for examples)
  • I have written a changelog entry for the PR or deem it unnecessary
  • My new functions/classes either have a docstring or are private
  • My new functions/classes have tests added and (optional) examples in the docs
  • My new documentation builds, looks correctly formatted, and adds no additional build warnings

Reviewer Checklist

  • The PR title is descriptive enough
  • The PR is labeled correctly
  • The changelog entry is completed if necessary
  • Newly added functions/classes either have a docstring or are private
  • Newly added functions/classes have tests added and (optional) examples in the docs
  • Newly added documentation builds, looks correctly formatted, and adds no additional build warnings

@abhijithmuthyala
Copy link
Contributor Author

abhijithmuthyala commented Apr 4, 2021

My bad, I ran pytest in master by mistake. I'll fix the tests

@huguesdevimeux
Copy link
Member

Really nice stuff

you can see why tests are failing with --show_diff flag, but I think it's fine to re-generate the graphical unit data for this test

@abhijithmuthyala abhijithmuthyala marked this pull request as draft April 4, 2021 16:40
@jsonvillanueva jsonvillanueva changed the title Set corners of Rectangle in counterclockwise direction Set corners of :class:~.Rectangle in counterclockwise direction Apr 5, 2021
@jsonvillanueva jsonvillanueva added the enhancement Additions and improvements in general label Apr 5, 2021
@abhijithmuthyala abhijithmuthyala marked this pull request as ready for review April 5, 2021 08:09
@abhijithmuthyala
Copy link
Contributor Author

I think the failing macos test can be fixed once #1255 is merged?

Copy link
Member

@jsonvillanueva jsonvillanueva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, yes tests should pass after #1255

@huguesdevimeux
Copy link
Member

tests still fail on macox -_-

@jsonvillanueva
Copy link
Member

tests still fail on macox -_-

That's because the branch hasn't updated to include the changes from #1255; @abhi3019, please update when you can.

@abhijithmuthyala
Copy link
Contributor Author

Done @jsonvillanueva

@behackl
Copy link
Member

behackl commented Apr 6, 2021

Something weird is happening here. I don't have the button for merging the latest master in the GH UI, and I also can't push to the branch. I'll merge it nonetheless now (thanks for your contribution!), and we can revert it in case it causes problems.

@behackl behackl merged commit 75fd9e2 into ManimCommunity:master Apr 6, 2021
@behackl behackl added this to the Release v0.6.0 milestone Apr 6, 2021
@jsonvillanueva
Copy link
Member

@behackl
I wonder if this happens when PRs are not created with the checkbox Allow edits from maintainers. Looking at the GitHub REST API for this PR, I don't see the "maintainer_can_modify" key in the json anywhere. Seems like it should exist regardless if it was True/False: https://docs.github.com/en/rest/reference/pulls#update-a-pull-request

@behackl
Copy link
Member

behackl commented Apr 6, 2021

@behackl
I wonder if this happens when PRs are not created with the checkbox Allow edits from maintainers. Looking at the GitHub REST API for this PR, I don't see the "maintainer_can_modify" key in the json anywhere. Seems like it should exist regardless if it was True/False: https://docs.github.com/en/rest/reference/pulls#update-a-pull-request

That would be a good explanation.

@abhijithmuthyala abhijithmuthyala deleted the ccw-orientation branch April 7, 2021 08:14
@jsonvillanueva jsonvillanueva mentioned this pull request May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additions and improvements in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants