Skip to content

Fix bug using z_index in VGroup #502

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

Closed
wants to merge 1 commit into from
Closed

Conversation

pdcxs
Copy link

@pdcxs pdcxs commented Oct 2, 2020

An easy but not elegant solution of problem in #327

@leotrs
Copy link
Contributor

leotrs commented Oct 2, 2020

Thanks! Did you mean to open this PR as a draft? It's unclear whether you are proposing we review this now.

  1. Please use the PR template next time.
  2. This has broken virtually every test. This doesn't mean the fix is wrong, but it means it's incomplete. (This is what makes me think this is not ready for review.)
  3. As we move toward using z_index more extensively, I would very much like to see new tests for it. Can you please add tests to the final version of this PR? If you are unsure of how to do that, we can of course discuss it here.

Marking as draft for now.

@leotrs leotrs marked this pull request as draft October 2, 2020 11:50
@behackl behackl added enhancement Additions and improvements in general pr:bugfix Bug fix for use in PRs solving a specific issue:bug and removed enhancement Additions and improvements in general labels Oct 2, 2020
@leotrs
Copy link
Contributor

leotrs commented Oct 8, 2020

@pdcxs did you have time to look into this? :)

@pdcxs
Copy link
Author

pdcxs commented Oct 9, 2020

@pdcxs did you have time to look into this? :)
Sorry, I'm too busy to write codes these days. I just put a sort according to the z-index in mobject add method to fix the bug #327. But I really cannot understand why all checks are failed. Maybe it is because I directly edit the code on github web page and the format is not correct?

@leotrs
Copy link
Contributor

leotrs commented Oct 9, 2020

Thanks for your answer.

Unfortunately, I do believe that changes in z_order must come with tests on them because they may change virtually every scene in existence. I think this is why the tests are failing.

I'd rather live with a known bug than merge this to fix a bug that breaks everything else. So, until you have time to look into this again, or someone else takes this branch up, I'm closing this.

@leotrs leotrs closed this Oct 9, 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.

3 participants