X Tutup
Skip to content

do not ignore "closed" parameter in Poly3DCollection#8014

Merged
WeatherGod merged 2 commits intomatplotlib:masterfrom
dlaidig:poly3dbug
Feb 6, 2017
Merged

do not ignore "closed" parameter in Poly3DCollection#8014
WeatherGod merged 2 commits intomatplotlib:masterfrom
dlaidig:poly3dbug

Conversation

@dlaidig
Copy link
Copy Markdown
Contributor

@dlaidig dlaidig commented Feb 3, 2017

In Poly3DCollection the "closed" parameter is effectively ignored, as Poly3DCollection.do_3d_projection will call PolyCollection.set_verts without passing the parameter, leading to the polygons always being closed.

To fix this, I store the "closed" parameter in Poly3DCollection.set_verts and apply it in do_3d_projection. Passing closed=True to PolyCollection in Poly3DCollection.set_verts only seems to add overhead, so I changed it to always pass False.

Example demonstrating the bug:

import numpy as np
import matplotlib.pyplot as plt
import mpl_toolkits.mplot3d as a3
from mpl_toolkits.mplot3d.art3d import Poly3DCollection

fig = plt.figure()
ax = fig.gca(projection='3d')
poly1 = np.array([[0, 0, 1], [0, 1, 1], [0, 0, 0]], np.float)
poly2 = np.array([[0, 1, 1], [1, 1, 1], [1, 1, 0]], np.float)
collection1 = Poly3DCollection([poly1], linewidths=3, edgecolor='k', facecolor=(0.5, 0.5, 1, 0.5), closed=True)
collection2 = Poly3DCollection([poly2], linewidths=3, edgecolor='k', facecolor=(1, 0.5, 0.5, 0.5), closed=False)
ax.add_collection3d(collection1)
ax.add_collection3d(collection2)
plt.savefig('poly3d.png')

I do not have any experience with the matplotlib internals, so please bear with me in case I got something wrong.

@dlaidig
Copy link
Copy Markdown
Contributor Author

dlaidig commented Feb 3, 2017

test_mixedsubplots does not fail on my system...

@tacaswell
Copy link
Copy Markdown
Member

tacaswell commented Feb 3, 2017

That test has some non-deterministic element and is known to be flaky.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Feb 3, 2017
@tacaswell tacaswell requested a review from WeatherGod February 3, 2017 13:29
@tacaswell
Copy link
Copy Markdown
Member

tacaswell commented Feb 3, 2017

Provisional 👍 from me, leave it to @WeatherGod to decide if this needs an image test or not.

@WeatherGod
Copy link
Copy Markdown
Member

WeatherGod commented Feb 3, 2017

This seems to work. We used to have major problems with the closed/unclosed polygons that we just went ahead and forced everything to be closed in order to get the trisurf stuff working and fix a few other bugs. I am a little surprised that the fix is as simple as it is.

The only thing we need is for the posted example be made into an image test. Tests are in lib/mpl_toolkits/tests/test_mplot3d.py and images go into lib/mpl_toolkits/tests/baseline_images/test_mplot3d/.

@WeatherGod WeatherGod merged commit c4b8fe6 into matplotlib:master Feb 6, 2017
@WeatherGod
Copy link
Copy Markdown
Member

WeatherGod commented Feb 6, 2017

Thank you for your fix!

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

Reviewers

@WeatherGod WeatherGod Awaiting requested review from WeatherGod

Assignees

No one assigned

Labels

topic: mplot3d

Projects

None yet

Milestone

v2.1

Development

Successfully merging this pull request may close these issues.

4 participants

@dlaidig @tacaswell @WeatherGod @dstansby
X Tutup