Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix depth shading on 3D scatterplots#29287

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

scottshambaugh
Copy link
Contributor

@scottshambaughscottshambaugh commentedDec 11, 2024
edited
Loading

PR summary

Replaces#23085, which was orphaned

Closes#22861

This PR has a few changes:

  1. It fixes the linked issue where depthshading is applied inconsistently on 3D scatter plots, leading to some edge cases with discontinuities in appearance.
  2. It adds a keyword argumentdephshade_minalpha to control the minimum opacity of the depthshading.
  3. The defaults fordepthshade anddepthshade_minalpha arguments are now controlled with rcparams.

The commits are broken up by authorship so please don't squash when merging.

PR checklist

@@ -483,13 +481,13 @@ def test_scatter3d_modification(fig_ref, fig_test):
depthshade=False, s=75, linewidths=3)


@pytest.mark.parametrize('depthshade', [True, False])
Copy link
ContributorAuthor

@scottshambaughscottshambaughDec 21, 2024
edited
Loading

Choose a reason for hiding this comment

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

The new depthshade behavior applies depth shading for eachPatch3DCollection. Since we are splitting this reference image into 4x scatter commands, it will have different depth shading than the single 1x scatter command for the test image.

image

I think this per-collection depth shading makes the most sense. The alternative would be to apply depth shading based on a fixed distance from the camera, which would give undesirable transparency to equal-distance points near the far clipping plane of the camera. Perhaps that is more intuitive for some people? But IMO the depth shading is a visual cue to help distinguish between points, so the absolute shading should be as opaque as possible, and the transparency only applied to show relative shading.

The middle ground would be to track all the patches in the scene and apply depth shading based on all thePatch3DCollections collectively, but I think that's probably hard to handle internally.

The original PR which added this test (#18293) was only concerned with thedepthshading=False case, so I don't think we lose much by removing theTrue case.

Copy link
ContributorAuthor

@scottshambaughscottshambaughDec 21, 2024
edited
Loading

Choose a reason for hiding this comment

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

Of note, the behavior before this current PR did not seem to be showing depthshading at all during thedepthshading=True test case. I'm not sure why.

main branch Screenshot 2024-12-21 145212

@scottshambaughscottshambaugh marked this pull request as ready for reviewDecember 21, 2024 23:11
@scottshambaughscottshambaughforce-pushed thescatter3d_shading branch 2 times, most recently from73d3d1b to987ac55CompareDecember 21, 2024 23:39
@scottshambaughscottshambaughforce-pushed thescatter3d_shading branch 3 times, most recently fromb5b15a0 toc210aacCompareJanuary 5, 2025 23:50
@scottshambaugh
Copy link
ContributorAuthor

This is good to go

@scottshambaugh
Copy link
ContributorAuthor

AppVeyor failure is unrelated

Copy link
Member

@oscargusoscargus 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!

I think there should be.. versionadded:: 3 11 where there are new arguments added (on public methods), but feel free to self-merge once those are added.

scottshambaugh reacted with thumbs up emoji
Oblimanand others added6 commitsJanuary 30, 2025 08:29
This update changes the depthscale behavior from the original Normalize method to an improved version that behaves well even when points are closely spaced.Update art3d.pyFixed line lengths, added handling of zero-length datasets when computing the data scale.Updated art3d.py per feedbackRenamed get_data_scale and dscl to indicate that they are private, added description of function's purpose, and replaced np.power with np.sqrt.Update art3d.pyForgot this portion of the update earlier. Fixes the alpha draw order flipping as the point depth inverts even though the alpha should not change per point.Update art3d.py per reviewer feedbackOnly one dataset of (X, Y, Z) needs to be checked for zero length assuming all have the same length.Instead of using a small constant in the denominator it's cleaner to just check for '_dscl==0' and return ones instead.This applies for datasets with no datapoints, all the same datapoint such that no scale can be estimated.Update art3d.py per reviewer feedbackFixed some word wrapping, made get_data_scale more compact, replaced _dscl with the more descriptive _data_scale, removed redundant comments, added clearer comments for implementation of shading modes
Added kwargs for depth-shadingCreate depthshading_improvement.rstUpdate art3d.py formattingUpdate axes3d.py formattingUpdate depthshading_improvement.rst formatting
Remove legacy and inverted depth shade optionsRemove depthshade gettersgettermake depthshade_minalpha kwarg onlykwarg ordergetterUpdate image test failuresLintingfix masked scatter depth shadingremoved depthshade checkfix testsFix build warning
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>Docstrings and typosCode review updatesvarnamevarname
@scottshambaugh
Copy link
ContributorAuthor

Rebase conflict was negligible and I added those versionadded tags. CI looks good so merging!

@scottshambaughscottshambaugh merged commit6758f8a intomatplotlib:mainJan 30, 2025
39 of 41 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic approved these changes

@oscargusoscargusoscargus approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.11.0
Development

Successfully merging this pull request may close these issues.

[Bug]: 3D scatter plot flips alpha order depending on depth relative to camera
4 participants
@scottshambaugh@QuLogic@oscargus@nhansendev

[8]ページ先頭

©2009-2025 Movatter.jp