Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
eea5a8f
tod90a631
Compare@@ -483,13 +481,13 @@ def test_scatter3d_modification(fig_ref, fig_test): | |||
depthshade=False, s=75, linewidths=3) | |||
@pytest.mark.parametrize('depthshade', [True, False]) |
scottshambaughDec 21, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
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 thePatch3DCollection
s 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.
scottshambaughDec 21, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
227fc6d
to6ce1a1a
Compare73d3d1b
to987ac55
Compareb5b15a0
toc210aac
CompareThis is good to go |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
31816fd
tof580423
CompareAppVeyor failure is unrelated |
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!
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.
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
f580423
to73b3b08
CompareRebase conflict was negligible and I added those versionadded tags. CI looks good so merging! |
6758f8a
intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
PR summary
Replaces#23085, which was orphaned
Closes#22861
This PR has a few changes:
dephshade_minalpha
to control the minimum opacity of the depthshading.depthshade
anddepthshade_minalpha
arguments are now controlled with rcparams.The commits are broken up by authorship so please don't squash when merging.
PR checklist