Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed polygon3d rendering bug issue #178 #1928

Closed
wants to merge 1 commit into from
Closed

Fixed polygon3d rendering bug issue #178 #1928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions lib/matplotlib/collections.py
View file
Open in desktop
Expand Up @@ -745,6 +745,16 @@ def set_verts(self, verts, closed=True):

set_paths = set_verts

def set_verts_and_codes(self, verts, codes):
assert(len(verts) == len(codes))
self._paths = []
for xy, cds in zip(verts, codes):
assert(len(xy) == len(cds))
if len(xy):
self._paths.append(mpath.Path(xy, cds))
else:
self._paths.append(mpath.Path(xy))

@allow_rasterization
def draw(self, renderer):
if self._sizes is not None:
Expand Down
41 changes: 29 additions & 12 deletions lib/mpl_toolkits/mplot3d/art3d.py
View file
Open in desktop
Expand Up @@ -132,11 +132,13 @@ def path_to_3d_segment(path, zs=0, zdir='z'):
zs = np.ones(len(path)) * zs

seg = []
codes = []
pathsegs = path.iter_segments(simplify=False, curves=False)
for (((x, y), code), z) in zip(pathsegs, zs):
seg.append((x, y, z))
codes.append(code)
seg3d = [juggle_axes(x, y, z, zdir) for (x, y, z) in seg]
return seg3d
return seg3d, codes
Copy link
Member

Choose a reason for hiding this comment

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

This changes the API of this function. The correct way to go about this is to deprecate this one and have a new function set up to replace it. Or, have a kwarg defaulting to False to indicate whether to return the codes as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@WeatherGod: I have changed all code that calls this and the other function you have highlighted. I see these as internal (i.e. private) functions, even though they are do not have leading underscores.

Copy link
Member

Choose a reason for hiding this comment

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

They are not private. These are public methods to objects that people do manipulate in the wild. You will have to go through the proper process for this change.


def paths_to_3d_segments(paths, zs=0, zdir='z'):
'''
Expand All @@ -147,9 +149,12 @@ def paths_to_3d_segments(paths, zs=0, zdir='z'):
zs = np.ones(len(paths)) * zs

segments = []
codes_list = []
for path, pathz in zip(paths, zs):
segments.append(path_to_3d_segment(path, pathz, zdir))
return segments
segs, codes = path_to_3d_segment(path, pathz, zdir)
segments.append(segs)
codes_list.append(codes)
return segments, codes_list
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here. Can't change the output API without some sort of transition period.


class Line3DCollection(LineCollection):
'''
Expand Down Expand Up @@ -196,7 +201,7 @@ def draw(self, renderer, project=False):

def line_collection_2d_to_3d(col, zs=0, zdir='z'):
"""Convert a LineCollection to a Line3DCollection object."""
segments3d = paths_to_3d_segments(col.get_paths(), zs, zdir)
segments3d = paths_to_3d_segments(col.get_paths(), zs, zdir)[0]
col.__class__ = Line3DCollection
col.set_segments(segments3d)

Expand Down Expand Up @@ -363,6 +368,8 @@ def __init__(self, verts, *args, **kwargs):

PolyCollection.__init__(self, verts, *args, **kwargs)

self._codes3d = None
Copy link
Member

Choose a reason for hiding this comment

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

I am not quite understanding why this is needed, though. Doesn't the Polygons in the PolyCollection have their own codes?

Copy link
Member Author

Choose a reason for hiding this comment

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

@WeatherGod, you need to understand the existing code for my changes to make sense. Take a look at the Poly3DCollection class in art3d.py, which is derived from the 2D PolyCollection in collections.py. Poly3DCollection stores the 3D vertices in self._vec and self._segis. Each time Poly3DCollection.do_3d_projection is called, the 3D vertices are transformed and projected into 2D points that are stored in the usual PolyCollection._paths that are used for rendering.

The root cause of issue #178 is that a 2D PolyCollection has both 2D vertices and 2D codes. The codes are used so that single arrays of vertices and codes can be used for multiple polygons, such as when a contour needs to render a hole polygon inside another polygon. Up until now, Poly3DCollection has had no understanding of these codes and so has rendered all the vertices as though they were a single polygon, causing incorrect rendering.

My solution to this is to follow the existing (IMHO poor) implementation, and store the codes in Poly3DCollection as self._codes3d much like the vertices are stored as self._vec and self._segis. Each time do_3d_projection is called, both the stored vertices and codes are transformed/projected and used to update the PolyCollection._paths that are used for rendering.

As an aside, the Poly3DCollection constructor is never called for 3D contours, instead the function poly_collection_2d_to_3d does some very nasty object creation without calling the constructor. I believe this is to avoid the base PolyCollection constructor being called, as this will complain that the vertices are not 2D. I put the line you have asked about in the constructor because it should be initialised whenever the constructor is called, but of course that argument also applies to self._vec and self._segis but this has not been done before for some reason.

As I said at the top of the PR, my code changes are not elegant but they are consistent (i.e. no worse than) the existing code. I didn't really want to touch any of the mplot3d code, but as you said you didn't know how to solve the problem I thought I'd give it a try.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see it now. A 3d polygon with a hole would need a MOVETO, but that information is never really conveyed properly.

As for your aside, you are absolutely right. I am the third maintainer of this codebase, and there is a lot of stuff in it that has been "don't touch because it might break". The reason for the oddity in the object creation is probably one borne of "getting the job done". The idea for most of these things is to let the 2D plotting functions do their jobs, and then augment them with the third dimension information.

Copy link
Member Author

Choose a reason for hiding this comment

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

@WeatherGod: You are correct, I have not explained the fix in the code but pretty much nothing is explained in the existing mplot3d code. As I keep saying, my changes are consistent with the existing code.

The more I think about it the less I want my name associated with the mplot3d codebase, hence I am closing this PR. I have diagnosed the problem and come up with a fix, albeit possibly a poor fix, so you or someone else can use my code as the basis for an acceptable fix if you wish.

You still owe me a pint as for figuring about the bug as you promised in #1299!


_zsort_functions = {
'average': np.average,
'min': np.min,
Expand Down Expand Up @@ -419,6 +426,10 @@ def set_verts(self, verts, closed=True):
# 2D verts will be updated at draw time
PolyCollection.set_verts(self, [], closed)

def set_verts_and_codes(self, verts, codes):
self.set_verts(verts, closed=False)
self._codes3d = codes

def set_3d_properties(self):
# Force the collection to initialize the face and edgecolors
# just in case it is a scalarmappable with a colormap.
Expand Down Expand Up @@ -457,18 +468,24 @@ def do_3d_projection(self, renderer):

# if required sort by depth (furthest drawn first)
if self._zsort:
z_segments_2d = [(self._zsortfunc(zs), zip(xs, ys), fc, ec) for
(xs, ys, zs), fc, ec in zip(xyzlist, cface, cedge)]
indices = range(len(xyzlist))
z_segments_2d = [(self._zsortfunc(zs), zip(xs, ys), fc, ec, idx)
for (xs, ys, zs), fc, ec, idx in zip(xyzlist, cface, cedge,
indices)]
z_segments_2d.sort(key=lambda x: x[0], reverse=True)
else:
raise ValueError, "whoops"

segments_2d = [s for z, s, fc, ec in z_segments_2d]
PolyCollection.set_verts(self, segments_2d)
segments_2d = [s for z, s, fc, ec, idx in z_segments_2d]
if self._codes3d is not None:
codes = [self._codes3d[idx] for z, s, fc, ec, idx in z_segments_2d]
PolyCollection.set_verts_and_codes(self, segments_2d, codes)
else:
PolyCollection.set_verts(self, segments_2d)

self._facecolors2d = [fc for z, s, fc, ec in z_segments_2d]
self._facecolors2d = [fc for z, s, fc, ec, idx in z_segments_2d]
if len(self._edgecolors3d) == len(cface):
self._edgecolors2d = [ec for z, s, fc, ec in z_segments_2d]
self._edgecolors2d = [ec for z, s, fc, ec, idx in z_segments_2d]
else:
self._edgecolors2d = self._edgecolors3d

Expand Down Expand Up @@ -508,9 +525,9 @@ def draw(self, renderer):

def poly_collection_2d_to_3d(col, zs=0, zdir='z'):
"""Convert a PolyCollection to a Poly3DCollection object."""
segments_3d = paths_to_3d_segments(col.get_paths(), zs, zdir)
segments_3d, codes = paths_to_3d_segments(col.get_paths(), zs, zdir)
col.__class__ = Poly3DCollection
col.set_verts(segments_3d)
col.set_verts_and_codes(segments_3d, codes)
col.set_3d_properties()

def juggle_axes(xs, ys, zs, zdir):
Expand Down
8 changes: 4 additions & 4 deletions lib/mpl_toolkits/mplot3d/axes3d.py
View file
Open in desktop
Expand Up @@ -464,7 +464,7 @@ def autoscale_view(self, tight=None, scalex=True, scaley=True,
.. versionchanged :: 1.1.0
Function signature was changed to better match the 2D version.
*tight* is now explicitly a kwarg and placed first.

.. versionchanged :: 1.2.1
This is now fully functional.

Expand Down Expand Up @@ -668,7 +668,7 @@ def set_zlim3d(self, bottom=None, top=None, emit=True, auto=False, **kw):
Set 3D z limits.

See :meth:`matplotlib.axes.Axes.set_ylim` for full documentation

"""
if 'zmin' in kw:
bottom = kw.pop('zmin')
Expand Down Expand Up @@ -1860,8 +1860,8 @@ def _3d_extend_contour(self, cset, stride=5):
dz = (levels[1] - levels[0]) / 2

for z, linec in zip(levels, colls):
topverts = art3d.paths_to_3d_segments(linec.get_paths(), z - dz)
botverts = art3d.paths_to_3d_segments(linec.get_paths(), z + dz)
topverts = art3d.paths_to_3d_segments(linec.get_paths(), z - dz)[0]
botverts = art3d.paths_to_3d_segments(linec.get_paths(), z + dz)[0]

color = linec.get_color()[0]

Expand Down