Skip to content

Fix #101 #164

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

Merged
merged 8 commits into from
Jul 8, 2020
Merged

Fix #101 #164

merged 8 commits into from
Jul 8, 2020

Conversation

huguesdevimeux
Copy link
Member

@huguesdevimeux huguesdevimeux commented Jun 23, 2020

Fixes #101

The issue was that the lines of NumberPlane were automatically centred on the axis, which could cause problem when x_max was not equal to x_min. (same for y).
Example of the issue :

class Test(Scene):
    def construct(self):
        NPac=NumberPlane(x_max=20).add_coordinates()
        NPac.scale(0.3)
        NPac.center()
        self.add(NPac)

Render this :
Test
As you can see, x-paralleles lines are shifted to the left.
Now, this is fixed :
Test

Test

I tested it with tests set up by #133 (not merged yet) and locally.

I also added a little bit of docstrings.

@huguesdevimeux huguesdevimeux added the pr:bugfix Bug fix for use in PRs solving a specific issue:bug label Jun 23, 2020
@huguesdevimeux huguesdevimeux marked this pull request as draft June 23, 2020 07:17
@huguesdevimeux huguesdevimeux changed the title Fix #101 Fix #101 (wip) Jun 23, 2020
@huguesdevimeux huguesdevimeux changed the title Fix #101 (wip) Fix #101 Jun 23, 2020
@huguesdevimeux huguesdevimeux marked this pull request as ready for review June 23, 2020 07:29
PgBiel
PgBiel previously requested changes Jun 23, 2020
Copy link
Member

@PgBiel PgBiel left a comment

Choose a reason for hiding this comment

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

the docs gang strikes again

@XorUnison
Copy link
Collaborator

We should also keep in mind that the x/y_min/max values are eventually to be replaced with xRange/yRange (or x_range, however the variable should be called), though I think we should first push this fix, I'll then make an issue for switching to ranges if nothing like that is around yet.

huguesdevimeux and others added 4 commits June 24, 2020 13:19
Co-authored-by: Pg Biel <9021226+PgBiel@users.noreply.github.com>
This merge is necessary to add a new test
@huguesdevimeux
Copy link
Member Author

huguesdevimeux commented Jun 24, 2020

@PgBiel Thank you so much Captain Docs ! I did your suggestions. Don't hesitate to check it, if you don't mind. I also thought to add a doc for NumberPlane, but how to do so with all the CONFIG mess ? What to document ?

@XorUnison Why not, but for me x_max is fine. Why do you think it would be better with a range thing?

@huguesdevimeux
Copy link
Member Author

huguesdevimeux commented Jun 24, 2020

Update : I also fixed a minor bug, when passing a value for faded_line_ratio, the number of faded lines between two non-faded lines was equal to value - 1, which was a bug. I think it was implemented to handle the case when faded_line_ratio is 0, but I don't think this is necessary.

Copy link
Contributor

@TonyCrane TonyCrane left a comment

Choose a reason for hiding this comment

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

Very effective, this problem has troubled me for a long time :D

Copy link
Contributor

@safinsingh safinsingh left a comment

Choose a reason for hiding this comment

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

See comments

@huguesdevimeux
Copy link
Member Author

@safinsingh Fixed, thank you

@kilacoda-old kilacoda-old self-requested a review July 8, 2020 12:56
@Aathish04 Aathish04 dismissed stale reviews from safinsingh and PgBiel July 8, 2020 13:00

Addressed

@huguesdevimeux huguesdevimeux merged commit ea79842 into ManimCommunity:master Jul 8, 2020
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.

Bug in NumberPlane when using x_max (and likely the y/min values too)
7 participants