Skip to content
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

Merged
merged 7 commits into from
Feb 27, 2021
Merged

Conversation

kentcr
Copy link
Contributor

@kentcr kentcr commented Jun 9, 2019

Draft PR to add force_zorder parameter to Axes3D

@WeatherGod
Copy link
Member

I am still torn about this change. On the one hand, I would rather see do_3d_projection() be the sole arbiter of an object's z-value (it can easily check the parent axes for the force_zorder flag). If do_3d_projection() is the only place where z-value is determined, then the end user has no need to do anything else for managing rendering during interactivity.

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.

@kentcr
Copy link
Contributor Author

kentcr commented Jun 10, 2019

@WeatherGod
The Axes3D.draw() function already modifies the zorder relative to the grids, so do_3d_projection() is already not the sole arbiter. There are also multiple do_3d_projection() functions, so if you moved it inside those functions you'd be duplicating code and each would only be the sole arbiter for its class.

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.

@anntzer anntzer added this to the v3.2.0 milestone Jun 12, 2019
@anntzer
Copy link
Contributor

anntzer commented Jun 12, 2019

Milestoning as 3.2 to at least try to avoid this falling through the cracks into oblivion like #12744 (or perhaps #12744 is better, I didn't review them carefully).

@ImportanceOfBeingErnest
Copy link
Member

ImportanceOfBeingErnest commented Aug 30, 2019

This PR adds an attribute force_zorder to the Axes3D. It can be used as

ax = fig.add_subplot(111, projection="3d")
ax.force_zorder = True

or

ax = fig.add_subplot(111, projection="3d", force_zorder=True)

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)

image

Complete code for reproduction
import matplotlib.pyplot as plt
import numpy as np
from mpl_toolkits.mplot3d import Axes3D
from mpl_toolkits.mplot3d.art3d import Poly3DCollection


fig = plt.figure()
ax1 = fig.add_subplot(221, projection="3d")
ax2 = fig.add_subplot(222, projection="3d")
ax2.force_zorder = True

ax1.set_title("default", pad=15)
ax2.set_title("force_zorder = True", pad=15)

# create an orizontal plane
corners = [[0,0,0],[0,5,0],[5,5,0],[5,0,0]]
for ax  in [ax1,ax2]:
    
    tri = Poly3DCollection([corners], alpha=1, zorder=1)
    tri.set_color('w')
    tri.set_edgecolor('k')
    ax.add_collection3d(tri)
    
    # plot a vector
    ax.plot([2,2],[2,2],[0,4], c = 'r', zorder=2)
    
    # plot some points
    ax.scatter([3,3],[1,3],[1,3], c = 'r', zorder=10)
    
    ax.set_xlim([0, 5.0])
    ax.set_ylim([0, 5.0])
    ax.set_zlim([0, 2.5])


ax3 = fig.add_subplot(223, projection="3d")
ax4 = fig.add_subplot(224, projection="3d")
ax4.force_zorder = True

dim = 10

X, Y = np.meshgrid([-dim, dim], [-dim, dim])
Z = np.zeros((2, 2))

angle = .5
X2, Y2 = np.meshgrid([-dim, dim], [0, dim])
Z2 = Y2 * angle
X3, Y3 = np.meshgrid([-dim, dim], [-dim, 0])
Z3 = Y3 * angle

r = 7
M = 1000
th = np.linspace(0, 2 * np.pi, M)

x, y, z = r * np.cos(th),  r * np.sin(th), angle * r * np.sin(th)

for ax in [ax3,ax4]:
    ax.plot_surface(X2, Y3, Z3, color='blue', alpha=.5, linewidth=0, zorder=-1)
    
    ax.plot(x[y < 0], y[y < 0], z[y < 0], lw=5, linestyle='--', color='green',
            zorder=0)
    
    ax.plot_surface(X, Y, Z, color='red', alpha=.5, linewidth=0, zorder=1)
    
    ax.plot(r * np.sin(th), r * np.cos(th), np.zeros(M), lw=5, linestyle='--',
            color='k', zorder=2)
    
    ax.plot_surface(X2, Y2, Z2, color='blue', alpha=.5, linewidth=0, zorder=3)
    
    ax.plot(x[y > 0], y[y > 0], z[y > 0], lw=5, linestyle='--', color='green',
            zorder=4)
    ax.view_init(azim=-20, elev=20)
    ax.axis('off')

plt.show()

The solution here is pretty non-invasive. As long as you do not set the attribute to True, everything stays as it is.

Given that

(a) It seems no-short term (or even long-term) solution to 3d plotting order is in sight,
(b) this solves a whole class of long-standing problems,
(c) it does not prevent future improvements on the algorithms for automatic drawing order

I'm very much in favor of getting this in (potentially even into 3.2).

@ImportanceOfBeingErnest
Copy link
Member

ImportanceOfBeingErnest commented Sep 9, 2019

It was asked what needs to be done here:

  • It needs to leave the draft status
  • We need a test of this new functionality. (And all tests need to run successfully.)
  • At the end this will need a "what's new entry" and possibly an example of how to use it.
  • At least two people need to be convinced of this being the correct thing to do. I already said that this has my vote (once the above points are handled), but ideally @WeatherGod would be one of them because he raised a concern above.

@MuellerSeb
Copy link

Anything new on this? I would love to use this!

@aomader
Copy link

aomader commented Apr 23, 2020

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.

@ImportanceOfBeingErnest
Copy link
Member

Everyone feel free to take this up and finish it.

@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 2, 2020
@ceriottm
Copy link

ceriottm commented Sep 2, 2020

This is really useful to make static 3D plots with the same "look and feel" as standard mpl graphics.
We often end up having to merge it manually on top of local installation to be able to use it.
Is there anything I can do to help getting it approved and merged?

@kentcr
Copy link
Contributor Author

kentcr commented Sep 3, 2020

@ceriottm

Is there anything I can do to help getting it approved and merged?

@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.

@ceriottm
Copy link

ceriottm commented Sep 3, 2020

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.
Also, I am a bit intimidated by the overhead of gettign to grips with the workflow for contributing to mpl, so it'd be good to know that someone who is familiar with the "way it should be done" will be looking over my shoulder.

@dopplershift
Copy link
Contributor

@WeatherGod, as the resident expert on the 3D support, have you developed any more firm feelings on this?

@WeatherGod
Copy link
Member

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?

@MuellerSeb
Copy link

in particular that rotated results may not look correct

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.

@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021
@kentcr kentcr marked this pull request as ready for review February 24, 2021 05:03
@tacaswell
Copy link
Member

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 fixed_zorder=False or depth_sorted=True as alternatives?

@tacaswell
Copy link
Member

auto_zorder, computed_zorder, respect_user_zorder also candidates.

@tacaswell
Copy link
Member

computed_zorder=True was the winner on the call.

@kentcr
Copy link
Contributor Author

kentcr commented Feb 26, 2021

@tacaswell

computed_zorder=True was the winner on the call.

Sounds good. The change is implemented.

Copy link
Member

@jklymak jklymak left a 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...

@@ -84,6 +88,8 @@ def __init__(
-----
.. versionadded:: 1.2.1
The *sharez* parameter.
.. versionadded:: TBD
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please.

Copy link
Contributor Author

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


Copy link
Member

Choose a reason for hiding this comment

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

stray new line?

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@timhoffm timhoffm left a 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

lib/mpl_toolkits/mplot3d/axes3d.py Outdated Show resolved Hide resolved
@@ -3505,6 +3518,7 @@ def stem(self, x, y, z, *, linefmt='C0-', markerfmt='C0o', basefmt='C3-',

stem3D = stem


Copy link
Member

Choose a reason for hiding this comment

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

👍

kentcr and others added 3 commits February 26, 2021 01:17
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@timhoffm timhoffm merged commit 2db6a04 into matplotlib:master Feb 27, 2021
@timhoffm
Copy link
Member

Thanks @kentcr, and congratulations on your first contribution to Matplotlib! We hope to see you back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet