Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

PyList_SetItem return value bug in clip_path_to_rect (_path.cpp). #1777

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Member

pelson commented Feb 22, 2013

Fixes a bug in the code which erroneously checks the result of PyList_SetItem.

I have some feature enhancing code to go with this. I was tempted to add it to this PR, but strictly speaking, that wouldn't be a bug-fix. A picture + some code is worth a 1000 words:

import numpy as np

import matplotlib.patches as mpatches
import matplotlib.path as mpath
import matplotlib.transforms as mtrans
import matplotlib.pyplot as plt
from matplotlib._path import clip_path_to_rect


def clip_path_mpl(path, bbox):
    """
    *THIS COULD BE A PATH METHOD*
    Clip the given path to the given bounding box. 

    """
    resultant_verts = clip_path_to_rect(path, bbox, True)

    if len(resultant_verts) == 0:
        result_path = mpath.Path([[0, 0]], codes=[mpath.Path.MOVETO])
    else:
        # Closed polygons going in to clip_path_to_rect are returned with
        # their final vertex missing. Add this back in if appropriate.
        is_closed = (np.all(path.vertices[0, :] == path.vertices[-1, :]) or
                     (path.codes is not None and 
                      path.codes[-1] == path.CLOSEPOLY))
        if is_closed:

            for i, verts in enumerate(resultant_verts):
                resultant_verts[i] = np.append(verts, verts[0:1, :], axis=0)
        vertices = np.concatenate(resultant_verts)
        result_path = mpath.Path(vertices=vertices)

    return result_path

ax = plt.axes()
ax.set_xlim([-20, 10])
ax.set_ylim([-150, 100])

path = mpath.Path.unit_regular_star(8)
path.vertices *= [10, 100]
path.vertices -= [5, 25]

patch = mpatches.PathPatch(path, alpha=0.5, facecolor='coral', edgecolor='none')
ax.add_patch(patch)

bbox = mtrans.Bbox([[-12, -77.5], [-2, 50]])
result_path = clip_path_mpl(path, bbox)
result_patch = mpatches.PathPatch(result_path, alpha=0.5, facecolor='green', lw=4, edgecolor='black')

ax.add_patch(result_patch)
plt.show()

clip_star

Member

WeatherGod commented Feb 22, 2013

Why haven't we noticed this before?

Owner

mdboom commented Feb 22, 2013

Wow -- that is a pretty serious typo.

Would you mind also adding the example above as a regression test?

Member

pelson commented Feb 22, 2013

Would you mind also adding the example above as a regression test?

I'd love to, but that would mean I would need to add the clip_path_mpl function from above. Ideally, I'd add that as a Path method - but we are then in the realms of adding features to v1.2.x. Flouting that rule in this special case would make me very happy (as I will be using the function in cartopy) but isn't something I wanted to do from the get-go.

Why haven't we noticed this before?

It's because it's not used. This is not the Agg clipping that we use all the time (which does Path -> Agg path -> clip -> draw), this is path clipping to create new Path instance (Path -> clip -> path). At least, I think that is the case...

Owner

mdboom commented Feb 22, 2013

Ah, I see. It seems like clip_path_mpl primarily exists to work around a bug in clip_path_to_rect, though (that closed paths are not returned closed). I think I'd prefer to fix that there. If we do that, then I think it's reasonable to bend the bugfix rule and add this simple method that handles the rebundling of the result from clip_path_to_rect in a Path object.

Member

pelson commented Feb 22, 2013

It seems like clip_path_mpl primarily exists to work around a bug in clip_path_to_rect

I guess it does. I've got a python version of the Sutherland-Hodgman polygon clipping algorithm which exhibits the same problem. I'd be happy enough to share (in fact, its here) you have an idea where the best place to put such a fix is? (@mdboom, you wrote the c++ implementation right?)

FYI I wont be able to pick this up until Monday.

Cheers, (and have a good weekend)

Owner

mdboom commented Feb 22, 2013

Yes -- I wrote the C++ version, and was thinking of putting the fix right in there. I may have a chance to do this sometime today, and if so, I'll just open a new PR referencing this one.

mdboom added a commit to mdboom/matplotlib that referenced this pull request Feb 22, 2013

Aternative to #1777. Properly closes the paths returned by clip_to_re…
…ct and adds a new clip_to_bbox method to Path that returns a Path object. Adds a test that tests clipping compound paths and multiple subsegments.
Member

pelson commented Feb 23, 2013

Done better in #1778.

@pelson pelson closed this Feb 23, 2013

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