Skip to content

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