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

RFC: Allow users to force zorder in 3D plots #14175

Closed
kentcr opened this issue May 9, 2019 · 17 comments
Closed

RFC: Allow users to force zorder in 3D plots #14175

kentcr opened this issue May 9, 2019 · 17 comments
Milestone

Comments

@kentcr
Copy link
Contributor

kentcr commented May 9, 2019

At present, the draw order order in 3D plots is determined exclusively by sorting the values returned by calling do_3d_projection() in each collection/patch:

# Calculate projection of collections and patches and zorder them.
# Make sure they are drawn above the grids.
zorder_offset = max(axis.get_zorder()
for axis in self._get_axis_list()) + 1
for i, col in enumerate(
sorted(self.collections,
key=lambda col: col.do_3d_projection(renderer),
reverse=True)):
col.zorder = zorder_offset + i
for i, patch in enumerate(
sorted(self.patches,
key=lambda patch: patch.do_3d_projection(renderer),
reverse=True)):
patch.zorder = zorder_offset + i

However, this sorting is not always ideal and a common issue reported by users. I'd propose adding a "force_zorder" optional parameter to Axes3D (defaulting to False) that would ignore the above calculation and instead use the zorder already stored in each collection/patch, i.e.

if self.force_zorder:
    for col in self.collections:
        col.zorder = zorder_offset + col.zorder
    for patch in self.patches:
        patch.zorder = zorder_offset + patch.zorder
else:
    # current code

This keeps existing behavior as default, but allows users to override that behavior. This does allow users to specify duplicate or negative zorders, as they can in 2D plots, but that hasn't generated errors in my testing.

However, not being at all familiar with the deeper workings, I'd rather get comments/concerns from the matplotlib maintainers and contributors before setting off to produce a full pull request.

Thanks!

@ImportanceOfBeingErnest
Copy link
Member

My view on this: Just try to implement this and see if it works. You can fork matplotlib, create a branch, change the code to your liking. Then, you see if it works for your application or not.
If it does, you have a local branch with the desired feature implemented and can use it for your plots.

Creating a PR from that branch is then trivial. So just create the PR. At this point everyone can look at the actual implementation and identify potential flaws; also you get feedback from the online test system.

Even if this does not make it into matplotlib, you have gained the following:

  • you have a version that works for you and that you can use
  • It's public, everyone else who needs it, can use it as well by just checking out the commit.
  • Potential problems with this are documented inside of the PR, available for everyone, also for the next person trying to implement something similar - who would then not need to start from scratch.

And not to forget, if it does work and make it into the library, you have helped the worldwide community to get a better matplotlib.

@kentcr
Copy link
Contributor Author

kentcr commented May 9, 2019

It's already working locally for my own purposes, but to my mind a pull request implies I think I've done the full job. If it's acceptable to submit a work-in-progress in order to receive feedback, then sure, I don't mind doing that. Either way, thanks for the response!

@jklymak
Copy link
Member

jklymak commented May 9, 2019

It’s often useful to come up with an example, show the with-patch change and compare to the example run on master so we can see the difference Otherwise you are asking us to apply your patch and come up w our own example.

@jklymak
Copy link
Member

jklymak commented May 9, 2019

That’s helpful as well but I meant what code did you test with and what did the plots look like before and after? Include the plots here.

@kentcr
Copy link
Contributor Author

kentcr commented May 9, 2019

I might spend the time on that later, but I don't think it should be necessary to get feedback about the idea.

@WeatherGod
Copy link
Member

I am not really all that convinced that this option would be all that beneficial. For example, when interacting with the scene, particularly with rotating it, the user would need to recalculate the zorder themselves or it will look all wrong.

Now, perhaps if we frame this feature in the right way, such as "don't let matplotlib get in my way, I'll take full responsibility for zsorting", then maybe this feature would make sense?

@story645
Copy link
Member

story645 commented May 9, 2019

If it's acceptable to submit a work-in-progress in order to receive feedback, then sure, I don't mind doing that. Either way, thanks for the response!

Github specifically has a draft pr feature that's absolutely welcome here. Do you think something could be added to the docs/README to make it clearer that matplotlib accepts those sorts of PRs?

@kentcr
Copy link
Contributor Author

kentcr commented May 10, 2019

@WeatherGod
I agree with the sentiment that this puts the onus on the user and we should make that clear. I was thinking force_zorder would help people stop and ask why it's "force" and not just "enable" or "use". Of course, we'd want to explain it further in documentation. The benefit is that at present I can't even find a hacky workaround (aside from editing the library) for this problem. Assuming no one is keeping that workaround a secret, why make people download the library the edit it themselves like I had to?

@story645 Thanks, that's helpful!

@WeatherGod
Copy link
Member

Been thinking on this over the weekend. The more I think about it, the less I like the idea of a flag to turn on and off the depth-calculation. What if we look at this differently, and allow users to plug in a different function for calculating the z value, and let the default function be the current one? In some respects, we actually do this right now by providing a choice between 2 different 3d projections.

@kentcr
Copy link
Contributor Author

kentcr commented May 14, 2019

As it stands, the projections just return a value that gets sorted and is used to set zorder. So the zorder is already the ultimate arbiter, but it gets overwritten during the draw. That already seems a bit wonky to me, so I don't know why you'd then add a layer that would force a user to provide a function that has to handle the depth calculation for two different object types (collections and patches), which ultimately is just going to set the zorder anyway.

They already have access to the zorder of the objects. It's already a bit odd that the zorder they set gets completely ignored. They can already write their own functions to determine and set the zorder of those objects. I'd think they could also use the existing projection functions, else we could expose them.

That all said, there's apparently stuff going on inside those projection functions that do more than just determine the order, so as it stands they need to be called anyway. So the flag wouldn't stop them from running, it'd just ignore their return values.

I've also learned that the draw function gets called more than once for a single plot, so I'm hoping we can just ignore the offset too.

@zorvesfa
Copy link

zorvesfa commented Jun 7, 2019

As a user, I would like to say that a mechanism of this kind would indeed be very useful. I got into the issue several times while producing static figures for articles. For this use case playing nice with interactivity is not required. But being able to override the default behavior and to force the z-order is extremely useful.
There is no easy solution without modifying matplotlib. I tried several things and once wrote a horrible hacky overloaded 3D axes to be able to do that. Unsurprisingly it broke at the next update.

As evidenced by the current pull request and the linked bugs and SO questions in #14175 (comment) (also issue #14148), this is a common issue.
This is also apparently not the first time that a mechanism allowing the user to force the z-order if they want was proposed, see the pull request #12744

The current PR gives me hope that it will soon be possible to manually set the z-order of elements in a 3D plot in an easy way! I really hope it does not end up abandoned like #12744!

@kentcr
Copy link
Contributor Author

kentcr commented Jun 9, 2019

@zorvesfa
Thanks for the encouragement. You got me off my butt enough to at least create a draft PR, here:
#14508

@kentcr
Copy link
Contributor Author

kentcr commented Mar 4, 2021

Merged

@kentcr kentcr closed this as completed Mar 4, 2021
@QuLogic QuLogic added this to the v3.5.0 milestone Mar 4, 2021
@nathanielvirgo
Copy link

How does one use this feature? Is it documented anywhere?

I have some old code that does some zorder tricks but it's inexplicably broken now, so I'm hoping enabling this will fix it again.

@nathanielvirgo
Copy link

Oh never mind, it's not the z axis stuff that's broken, it's something else. Still sounds like it would be helpful to document this though.

@timhoffm
Copy link
Member

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

No branches or pull requests

10 participants