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

[ENH]: fill_between extended to 3D #28225

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

scottshambaugh
Copy link
Contributor

@scottshambaugh scottshambaugh commented May 14, 2024

PR summary

Closes #28142

This extends the 2D fill_between method to 3D plots, and IMO works quite nicely. I’m finding it really powerful to generate surfaces easily. Inspired by @artmenlope's solution here: https://github.com/artmenlope/matplotlib-fill_between-in-3D/blob/master/FillBetween3d.py

Here's the what's new image and fillbetween3d gallery example:
whatsnew

Note that I moved the poly3d example https://matplotlib.org/3.9.0/gallery/mplot3d/polys3d.html to a new example fillunder3d, to use this new method. Then I made a new example for poly3d since adding 3D collections wasn't shown anywhere else afaik.

New fillunder3d example:
fillbetween3d

New poly3d example:
whatsnewpoly

PR checklist

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented May 14, 2024

The white seams between the patches seems to be an instance of #1188, which I don't see a bug report for in 3D rendering. Is rather annoying but doesn't seem like there are easy fixes.

@github-actions github-actions bot added the Documentation: API files in lib/ and doc/api label May 14, 2024
@scottshambaugh
Copy link
Contributor Author

Note that I decided to not implement the "mode 2" idea in https://github.com/artmenlope/matplotlib-fill_between-in-3D/blob/master/FillBetween3d.py, which concatenates all the points and plots them as one polygon. I think the only benefit is getting rid of the white seams between the patches in the filled flat curves example, but it is degenerate in the general case, and this use case can be handled by users defining their own polygons such as in the new poly3d example.

For example, Mode 1 / Mode 2:
image

@github-actions github-actions bot added the Documentation: plot types files in galleries/plot_types label May 17, 2024
@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented May 17, 2024

Here's the new plot types thumbnail showing a double helix:

image

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@scottshambaugh scottshambaugh force-pushed the 3d_fill_between branch 2 times, most recently from 779b177 to 76eff4a Compare May 17, 2024 14:29
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.

Only doc suggestions.

doc/users/next_whats_new/fill_between_3d.rst Outdated Show resolved Hide resolved
doc/users/next_whats_new/fill_between_3d.rst Outdated Show resolved Hide resolved
galleries/examples/mplot3d/polys3d.py Outdated Show resolved Hide resolved
lib/mpl_toolkits/mplot3d/axes3d.py Outdated Show resolved Hide resolved
Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

Requesting changes just so someone doesn't merge without my comment being addressed.

I think "mode 1" vs "mode 2" should be investigated here, and I'd like to see the new test case with the "mode 2" version as well to see if that applies the edge color to only the rim/boundary of the single fill_between region. Naively thinking about it that would be my preference for how to draw these, but I also haven't given it a ton of thought.

if not len(x1slice):
continue

for i in range(len(x1slice) - 1):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is in relation to your "mode 2" comment, but I would have naively expected each contiguous region to be a single polygon. Adding additional black edges in the new test case doesn't seem ideal and I would have expected all black "edge colors" to only be around the rim of the fill_between.

I would have expected something like: iterate over all "1" points adding those to the (N*2, 3) list of points for this specific region/poly, move down to "2" and iterate over those in reverse.

I think it will also be much more performant to only have a few polygons instead of one for each quad number of polygons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation: API files in lib/ and doc/api Documentation: examples files in galleries/examples Documentation: plot types files in galleries/plot_types New feature topic: mplot3d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH]: Add fill between support for 3D plots
3 participants