-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add force_zorder parameter #14508
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
Add force_zorder parameter #14508
Conversation
I am still torn about this change. On the one hand, I would rather see On the other hand, it does seem weird to me for a method that performs a projection calculation to return a value that didn't come from the projection computation. |
@WeatherGod If the concern is that the underlying code could use some refactoring, I agree, but I don't think that needs to be addressed as part of this change so long as it's not making things worse, e.g. a bad API change or changes that make it harder to refactor going forward. So my main concern is that some ultimate future API would be different than I'm proposing here, but I haven't noticed anyone mention that yet. Axes3D seems like a decent fit to me, but I'm not sure if there are any decent alternatives to consider. |
This PR adds an attribute
or
Here is something to show the effect of this PR. The examples are adapted from https://stackoverflow.com/questions/14824893/how-to-draw-intersecting-planes and #12620 (comment) Complete code for reproduction
The solution here is pretty non-invasive. As long as you do not set the attribute to Given that (a) It seems no-short term (or even long-term) solution to 3d plotting order is in sight, I'm very much in favor of getting this in (potentially even into 3.2). |
It was asked what needs to be done here:
|
Anything new on this? I would love to use this! |
I second this. I run into this issue basically once a year, and this seems to be an easy fix for the common case (I would argue) to produce a static image rather than an interactive plot. |
Everyone feel free to take this up and finish it. |
This is really useful to make static 3D plots with the same "look and feel" as standard mpl graphics. |
@ImportanceOfBeingErnest kindly provided a list of todos (#14508 (comment)), but I ran into issues trying to get the test suite to work. It looks like the documentation surrounding it has been updated so maybe that's now addressed, but given that there isn't an accord that this is the "correct thing to do", it's an extremely low priority for me. That may be something you want to take a shot at though. |
Thanks. I agree it is discouraging not to know if people who eventually can merge this are satisfied this is conceptually something they'll want to merge. To me, it is a pragmatic solution that will make it possible to do plots that are currently unfeasible with mpl, and the changes are so limited that the risk it has unintended consequences seems low. |
@WeatherGod, as the resident expert on the 3D support, have you developed any more firm feelings on this? |
I guess this would be the pragmatic approach. In addition to the above points by @ImportanceOfBeingErnest, I think it needs to be absolutely clear what this option is for, and what the downsides are (in particular that rotated results may not look correct). Perhaps an example linked to from the keyword argument in the docstring? |
Rotated results don't look good either way as a result of unwanted z-fights. And that is the reason for this PR: to have a chance to make static 3D plot look correct/good. |
988be50
to
4de1eb4
Compare
I am in favor of this, but I think the kwarg name is confusing. Once we get to 3D zorder is ambiguous and the name is only clear in the context that we are forcing what mpl calls zorder to ignore the depth sorting which is also reasonably called "zorder" as it is the order as we look at the 3D scene!). Maybe |
|
|
Sounds good. The change is implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me other than the two small issues...
lib/mpl_toolkits/mplot3d/axes3d.py
Outdated
@@ -84,6 +88,8 @@ def __init__( | |||
----- | |||
.. versionadded:: 1.2.1 | |||
The *sharez* parameter. | |||
.. versionadded:: TBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that we do these annotations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I remove both then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, though for what it's worth there are still a couple dozen similar 'versionadded' notes elsewhere in the file.
@@ -3505,6 +3518,7 @@ def stem(self, x, y, z, *, linefmt='C0-', markerfmt='C0o', basefmt='C3-', | |||
|
|||
stem3D = stem | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray new line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a flake8 rule violation to only have one empty line after a class definition (E305), so I figured I'd risk fixing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should get a what's new entry. See `doc/users/next_whats_new/README.rst
@@ -3505,6 +3518,7 @@ def stem(self, x, y, z, *, linefmt='C0-', markerfmt='C0o', basefmt='C3-', | |||
|
|||
stem3D = stem | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Thanks @kentcr, and congratulations on your first contribution to Matplotlib! We hope to see you back. |
Draft PR to add force_zorder parameter to Axes3D