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

Rewrite put_start_and_end_on to work in 3D #1355

Merged
merged 44 commits into from Apr 23, 2021

Conversation

Darylgolden
Copy link
Member

@Darylgolden Darylgolden commented Apr 18, 2021

Changelog / Overview

Motivation

Previously,

class MovingVertices(ThreeDScene):
    def construct(self):
        self.set_camera_orientation(phi=75 * DEGREES, theta=30 * DEGREES)
        vertices = [1, 2, 3, 4]
        edges = [(1, 2), (2, 3), (3, 4), (1, 3), (1, 4)]
        g = Graph(vertices, edges)
        self.play(Create(g))
        self.wait()
        self.play(g[1].animate.move_to([1, 1, 1]),
                  g[2].animate.move_to([-1, 1, 2]),
                  g[3].animate.move_to([1, -1, -1]),
                  g[4].animate.move_to([-1, -1, 0]))
        self.wait()

broke and rendered like this:

scriptoutput.1.mp4

Rewriting put_start_and_end_on to work in 3D fixes this:

MovingVertices.mp4

the angle is slightly different? There seems to be non-deterministic output with that code. Stable also has that problem (at least on my system), so I don't think it's my fault.

Testing Status

Further Comments

Dependent on #1354. I hope I did the git stuff properly.

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

@Darylgolden Darylgolden marked this pull request as draft April 18, 2021 02:11
@Darylgolden Darylgolden marked this pull request as ready for review April 18, 2021 02:48
@Darylgolden
Copy link
Member Author

Darylgolden commented Apr 18, 2021

This branch has the same non-deterministic errors I pointed out on #1354. I'm also not sure what to do about the failing tests on Mac, it seems like numpy evaluates the function slightly differently on there, causing a very small numerical error.

@jsonvillanueva
Copy link
Member

This branch has the same non-deterministic errors I pointed out on #1354. I'm also not sure what to do about the failing tests on Mac, it seems like numpy evaluates the function slightly differently on there, causing a very small numerical error.

Seems related to #972

@Darylgolden Darylgolden marked this pull request as draft April 20, 2021 01:10
@Darylgolden Darylgolden marked this pull request as ready for review April 21, 2021 02:18
@Darylgolden
Copy link
Member Author

Even after rounding, the tests still fail on Mac because -0 is different from 0 for some reason.

@Darylgolden
Copy link
Member Author

I managed to get the tests working, but using sort of hacky methods.

@Darylgolden
Copy link
Member Author

I've written a test in three_d, let me know if it's misplaced.

Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

LGTM! I only have one cosmetic remark (which doesn't really matter though); I think this can also be merged as-is.

tests/test_graphical_units/test_threed.py Show resolved Hide resolved
@behackl
Copy link
Member

behackl commented Apr 23, 2021

I noticed that the test time went up by a bit, and after checking the test out locally I see that my suggestion was bad: Dot3D is rather heavy for the rendering (although it looks better in the example). I've reverted the test and the corresponding data back to using the default vertex type. If the pipeline passes after this, the PR can be merged.

@behackl behackl added the pr:bugfix Bug fix for use in PRs solving a specific issue:bug label Apr 23, 2021
@behackl behackl added this to the Release v0.6.0 milestone Apr 23, 2021
@behackl behackl merged commit a3faf1c into ManimCommunity:master Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:bugfix Bug fix for use in PRs solving a specific issue:bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants