manim icon indicating copy to clipboard operation
manim copied to clipboard

enable z_index in Circumscribe

Open vahndi opened this issue 3 years ago • 5 comments

Overview: What does this pull request change?

adds z_index parameter to Circumscribe constructor so it can be used for the z_index of the frame (SurroundingRectangle or Circle)

Motivation and Explanation: Why and how do your changes improve the library?

Links to added or changed documentation pages

Further Information and Comments

Reviewer Checklist

  • [ ] The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • [ ] If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • [ ] If applicable: newly added functions and classes are tested

vahndi avatar Nov 17 '22 05:11 vahndi

That SGTM, but I'd rather wait for @behackl approval.

Do you mind trying to add some test about that ? It's actually a corner case of manim interesting to test (the behaviour of surrounding shapes when z_index is special)

huguesdevimeux avatar Nov 17 '22 22:11 huguesdevimeux

Sure - I'll take a look at the test suite at the weekend and see what I can do.

vahndi avatar Nov 17 '22 23:11 vahndi

Keep in mind the effects for the opengl renderer! It doesn't support z_index

MrDiver avatar Mar 31 '23 22:03 MrDiver

Keep in mind the effects for the opengl renderer! It doesn't support z_index

Just to piggyback on what MrDiver said :sweat_smile: I don't really like the concept of z_index in general and IMO it is more intuitive to replace that with a change of the Z-coordinate in general. I think that would functionally be equally useful as an indicator for Cairo to decide which Mobjects to draw in front of the others in a 2D scenario (maybe I'm missing something for the 3D scenes in Cairo, tho), while at the same time making sense for OpenGL and just being more intuitive and consistent with our human 3D reality in general.

But apart from that, I think that everything that the three of us said is not really in the scope of this PR, and I also like the changes.

I would like to again spark some discussion about this :eyes:

chopan050 avatar Dec 05 '23 04:12 chopan050

I would be on team @behackl and think we should move it and maybe also have something like a Constructor parameter for a more general Circumscribe animation? But that's just an idea.

But mainly moving the parameters would be great!

MrDiver avatar Dec 05 '23 09:12 MrDiver

Closing due to inactivity.

behackl avatar Jul 13 '24 18:07 behackl