10000 PyList_SetItem return value bug in clip_path_to_rect (_path.cpp). by pelson · Pull Request #1777 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

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

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
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

pelson
Copy link
Member
@pelson 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

@WeatherGod
Copy link
Member

Why haven't we noticed this before?

@mdboom
Copy link
Member
mdboom commented Feb 22, 2013

Wow -- that is a pretty serious typo.

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

@pelson
Copy link
Member Author
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...

@mdboom
Copy link
Member
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.

@pelson
Copy link
Member Author
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)

@mdboom
Copy link
Member
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
@mdboom
Aternative to matplotlib#1777. Properly closes the paths returned by …
d56aaa1 
…clip_to_rect 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.
@mdboom mdboom mentioned this pull request Feb 22, 2013
Merged
@pelson
Copy link
Member Author
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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pelson @WeatherGod @mdboom
0