Skip to content

fixed transparency bug #10487

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

Closed
wants to merge 0 commits into from
Closed

Conversation

DanielMatu
Copy link
Contributor

@DanielMatu DanielMatu commented Feb 16, 2018

in Poly3DCollection's set_alpha method, facecolor was getting updated when facecolors3d should have been updated instead.

fixes #10237

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@WeatherGod
Copy link
Member

good catch! Do you happen to have a snippet of code that could be used as a small test?

@WeatherGod WeatherGod added this to the v2.2.0 milestone Feb 16, 2018
@DanielMatu
Copy link
Contributor Author

DanielMatu commented Feb 16, 2018

Snippet of code to reproduce fixed results (bugged on live)

from mpl_toolkits.mplot3d import Axes3D
from mpl_toolkits.mplot3d.art3d import Poly3DCollection
import matplotlib.pyplot as plt

fig = plt.figure()
ax = Axes3D(fig)
x = [0, 1, 1, 0]
y = [0, 0, 1, 1]
z = [0, 1, 0, 1]
verts = [list(zip(x, y, z))]

alpha=0.5
fc = "C0"

pc = Poly3DCollection(verts, alpha = alpha, facecolors=fc, linewidths=1)

ax.add_collection3d(pc)
plt.show()

@tacaswell tacaswell modified the milestones: v2.2.0, v3.0 Feb 26, 2018
@tacaswell
Copy link
Member

I am not sure that is this the right fix as if you apply alpha multiple times they will stack (ex setting the alpha to 0.5 twice will effectively be 0.25).

The same fix should probably be applied to edge colors as well.

@DanielMatu
Copy link
Contributor Author

Hmm I'll look into that soon, and yeah this seems applicable to edgecolors as well. I just kept it at facecolors for simplicity since I'm a first-time contributer.

@jklymak jklymak changed the title fixed transparency bug #10237 fixed transparency bug Feb 27, 2018
@DanielMatu
Copy link
Contributor Author

Alpha stacking doesn't seem to be an issue from what I see. I've tested this by applying alpha (setting alpha to 0.5) 6 times (effectively reducing down to 0.01 if stacking occurs), but it remained 0.5, demonstrating that stacking doesn't happen. Let me know if I'm missing something (I'm uploading this response in a bit of a rush!).

Code for reproduction:

from mpl_toolkits.mplot3d import Axes3D
from mpl_toolkits.mplot3d.art3d import Poly3DCollection
import matplotlib.pyplot as plt

fig = plt.figure()
ax = Axes3D(fig)
x = [0, 1, 1, 0]
y = [0, 0, 1, 1]
z = [0, 1, 0, 1]
verts = [list(zip(x, y, z))]

alpha=0.5
fc = "C0"

pc = Poly3DCollection(verts, alpha = alpha, facecolors=fc, linewidths=1)
pc.set_alpha(alpha)
pc.set_alpha(alpha)
pc.set_alpha(alpha)
pc.set_alpha(alpha)
pc.set_alpha(alpha)
ax.add_collection3d(pc)
plt.show()

@jklymak
Copy link
Member

jklymak commented Mar 12, 2018

Thanks @DanielMatu This could use a test so this doesn't get missed again, if you are up for it: I think in tests/test_mplot3d. Bonus points if it doesn't require a new image comparison.

@DanielMatu
Copy link
Contributor Author

Sounds good. I'll get on that asap.

@ImportanceOfBeingErnest
Copy link
Member

What's that travis error that prevents this from passing the tests? It doesn't seem to be part of this PR?

@anntzer
Copy link
Contributor

anntzer commented Apr 29, 2018

forced a rebuild

@ImportanceOfBeingErnest
Copy link
Member

Would alpha need to be applied to edgecolor as well? (see #9559)

@rth
Copy link
Contributor

rth commented May 30, 2018

Would alpha need to be applied to edgecolor as well?

I just kept it at facecolors for simplicity since I'm a first-time contributer.

+1 for also applying this to edgecolors, as otherwise, this PR won't fully fix the parent issue.

@rth
Copy link
Contributor

rth commented May 30, 2018

What's that travis error that prevents this from passing the tests? It doesn't seem to be part of this PR?

Travis CI fails for Mac OS with /usr/bin/python: No module named pip possibly due to travis-ci/travis-ci#8829 . This doesn't happen on master, maybe syncing with it could help?

@jklymak jklymak modified the milestones: v3.0, v3.1 Jul 9, 2018
@jklymak jklymak requested a review from WeatherGod July 9, 2018 19:51
@WeatherGod
Copy link
Member

Looking at the original issue, there were a few different ways to get the right result and a few ways to get the wrong result. It might make sense to test a few different ways to make sure that this works right.

@jklymak jklymak modified the milestones: v3.0, v3.1 Jul 9, 2018
@tacaswell
Copy link
Member

ping @DanielMatu Any interest in still working on this?

@DanielMatu
Copy link
Contributor Author

DanielMatu commented Feb 5, 2019 via email

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 is a fix with at least one test. So it’s clearly an improvement and I would merge as is (after a rebase). @WeatherGod do you agree or do you insist on more tests?

@jklymak
Copy link
Member

jklymak commented Mar 3, 2019

Ummm, my apologies, somehow I broke this by force pushing....

@jklymak jklymak mentioned this pull request Mar 3, 2019
6 tasks
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.

Setting an alpha value to a Poly3DCollection
8 participants