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
Added code information to Poly3DCollection #4793
Conversation
self._paths = [] | ||
for xy, cds in zip(verts, codes): | ||
if len(xy): | ||
self._paths.append(mpath.Path(xy,cds)) |
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.
pep8 whitespace
attn @WeatherGod |
def set_verts_and_codes(self, verts, codes): | ||
if (len(verts) != len(codes)): | ||
raise ValueError("'codes' must be a 1D list or array "\ | ||
"with the same length of 'verts' ") |
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.
should also get rid of the unneeded backslash and the extraneous space at the end of the exception message.
by and large, this looks good to me. Should add at least an image test, probably also a 'what's new' entry to describe the new method added to PolyCollection to allow for describing collections of polygons with holes (assuming I am understanding the limitation of the old code correctly?). |
@WeatherGod thanks. I pushed a new commit addressing the pep8 issues. I'll add the docstring, the what's new and look into the image test. You have the gist of it, polygons with holes in certain scenarios render improperly as every line ends up being a "draw" when some need to be "moves" (as I understand it). Making use of the path codes has fixed 100% of my problem cases, which was almost all of my hundreds of plots. |
self._paths.append(mpath.Path(xy, cds)) | ||
else: | ||
self._paths.append(mpath.Path(xy)) | ||
|
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.
still missing the self.stale = True
.
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.
I caught that after I pushed the last commit. I have it in my local branch and I'll push it once I have the testcase figured out (trying to find a simplest case than my 40x40 data that reproduces the error)
Fixes matplotlib#4784 by adding path code information to the Poly3DCollection. This adds two new methods, path_to_3d_segment_with_codes and paths_to_3d_segments_with_codes which are meant to replace the versions without "_with_codes". A method was added to PolyCollection to allow Poly3DCollection to set vertices with path codes. Add image test for a case that causes improper polygon rendering without this fix. This code was adapted from a PR by @ianthomas23.
@WeatherGod I pushed a new commit with the additions and changes mentioned above. It failed the python 3.3 CI check, but as far as I can tell that error wasn't related to my changes. |
seg = [] | ||
codes = [] | ||
pathsegs = path.iter_segments(simplify=False, curves=False) | ||
for (((x, y), code), z) in zip(pathsegs, zs): |
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.
If you use izip_longest
here, you don't need to allocate a large array of ones in zs
first.
👍 |
The failure is a transient one that has been occurring on Travis for a while now. I restarted the test to make everything green. |
Added code information to Poly3DCollection
Fixes #4784 by adding path code information to the Poly3DCollection.
This adds two new methods, path_to_3d_segment_with_codes and
paths_to_3d_segments_with_codes which are meant to replace the
versions without "_with_codes". A method was added to PolyCollection
to allow Poly3DCollection to set vertices with path codes.
This code was adapted from a PR by @ianthomas23.