Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions doc/api/api_changes/2017-02-26-MB_intersects_bbox.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
Path.intersects_bbox always treats the bounding box as filled
`````````````````````````````````````````````````````````````

Previously, when ``Path.intersects_bbox`` was called with ``filled`` set to
``False``, it would treat both the path and the bounding box as unfilled. This
behavior was not well documented and it is usually not the desired behavior,
since bounding boxes are used to represent more complex shapes located inside
the bounding box. This behavior has now been changed: when ``filled`` is
``False``, the path will be treated as unfilled, but the bounding box is still
treated as filled. The old behavior was arguably an implementation bug.

When ``Path.intersects_bbox`` is called with ``filled`` set to ``True``
(the default value), there is no change in behavior. For those rare cases where
``Path.intersects_bbox`` was called with ``filled`` set to ``False`` and where
the old behavior is actually desired, the suggested workaround is to call
``Path.intersects_path`` with a rectangle as the path:

from matplotlib.path import Path
from matplotlib.transforms import Bbox, BboxTransformTo
rect = Path.unit_rectangle().transformed(BboxTransformTo(bbox))
result = path.intersects_path(rect, filled=False)
13 changes: 6 additions & 7 deletions lib/matplotlib/path.py
Original file line number Diff line number Diff line change
Expand Up @@ -558,14 +558,13 @@ def intersects_bbox(self, bbox, filled=True):
:class:`~matplotlib.transforms.Bbox`.

*filled*, when True, treats the path as if it was filled.
That is, if one path completely encloses the other,
:meth:`intersects_path` will return True.
That is, if the path completely encloses the bounding box,
:meth:`intersects_bbox` will return True.

The bounding box is always considered filled.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this a change in behavior or clarification of current behavior?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's a change in behaviour when filled=False, however this mode was only used in legend.py, and in that context the old behaviour was incorrect: the bounding box (legend) is always filled even when the plot lines are not filled.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a way to not change this behavior? This is a public method on a public so we must be careful about changing API. You will be surprised what behavior users have found and rely on 😈 .

On the other hand, the docstring was clearly a copy-paste from the method above and not quite right and I think it is reasonable to interpret 'intersects bbox' as 'is in or crosses boundary of bbox'. It that case this needs to be documented as an API change in https://github.com/matplotlib/matplotlib/tree/master/doc/api/api_changes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The name 'bounding box' already implies that the box represents the boundary of some more complex object which is somewhere inside the bounding box, so I think the new behavior makes more sense.

In order to retain the old behavior, it would be necessary to add an additional option, because we would actually need three use cases:

  • Filled path intersects filled bbox (filled=True, unchanged, needed in many places).
  • Unfilled path intersects unfilled bbox (filled=False, old behavior).
  • Unfilled path intersects filled bbox (filled=False, new behavior, needed in legend).

So it looks like that would also result in an API change, unless the filled argument accepts more than two possible values.

If you agree, I will keep the behavior as I've implemented it now and update api_changes.

"""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did this used to deal with non-affine transformations?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, I checked that. This is just a somewhat confusing way of getting a rectangle that represents the bounding box.

from .transforms import BboxTransformTo
rectangle = self.unit_rectangle().transformed(
BboxTransformTo(bbox))
result = self.intersects_path(rectangle, filled)
return result
return _path.path_intersects_rectangle(self,
bbox.x0, bbox.y0, bbox.x1, bbox.y1, filled)

def interpolated(self, steps):
"""
Expand Down
60 changes: 60 additions & 0 deletions src/_path.h
Original file line number Diff line number Diff line change
Expand Up @@ -872,6 +872,66 @@ bool path_intersects_path(PathIterator1 &p1, PathIterator2 &p2)
return false;
}

// returns whether the segment from (x1,y1) to (x2,y2)
// intersects the rectangle centered at (cx,cy) with size (w,h)
// see doc/segment_intersects_rectangle.svg for a more detailed explanation
inline bool segment_intersects_rectangle(double x1, double y1,
double x2, double y2,
double cx, double cy,
double w, double h)
{
return fabs(x1 + x2 - 2.0 * cx) < fabs(x1 - x2) + w &&
fabs(y1 + y2 - 2.0 * cy) < fabs(y1 - y2) + h &&
2.0 * fabs((x1 - cx) * (y1 - y2) - (y1 - cy) * (x1 - x2)) <
w * fabs(y1 - y2) + h * fabs(x1 - x2);
}

template <class PathIterator>
bool path_intersects_rectangle(PathIterator &path,
double rect_x1, double rect_y1,
double rect_x2, double rect_y2,
bool filled)
{
typedef PathNanRemover<py::PathIterator> no_nans_t;
typedef agg::conv_curve<no_nans_t> curve_t;

if (path.total_vertices() == 0) {
return false;
}

no_nans_t no_nans(path, true, path.has_curves());
curve_t curve(no_nans);

double cx = (rect_x1 + rect_x2) * 0.5, cy = (rect_y1 + rect_y2) * 0.5;
double w = fabs(rect_x1 - rect_x2), h = fabs(rect_y1 - rect_y2);
double xmin = std::min(rect_x1, rect_x2), xmax = std::max(rect_x1, rect_x2);
double ymin = std::min(rect_x1, rect_x2), ymax = std::max(rect_x1, rect_x2);

double x1, y1, x2, y2;

curve.vertex(&x1, &y1);
if (2.0 * fabs(x1 - cx) <= w && 2.0 * fabs(y1 - cy) <= h) {
return true;
}

while (curve.vertex(&x2, &y2) != agg::path_cmd_stop) {
if (segment_intersects_rectangle(x1, y1, x2, y2, cx, cy, w, h)) {
return true;
}
x1 = x2;
y1 = y2;
}

if (filled) {
agg::trans_affine trans;
if (point_in_path(cx, cy, 0.0, path, trans)) {
return true;
}
}

return false;
}

template <class PathIterator>
void convert_path_to_polygons(PathIterator &path,
agg::trans_affine &trans,
Expand Down
34 changes: 34 additions & 0 deletions src/_path_wrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,39 @@ static PyObject *Py_path_intersects_path(PyObject *self, PyObject *args, PyObjec
}
}

const char *Py_path_intersects_rectangle__doc__ = "path_intersects_rectangle(path, rect_x1, rect_y1, rect_x2, rect_y2, filled=False)";

static PyObject *Py_path_intersects_rectangle(PyObject *self, PyObject *args, PyObject *kwds)
{
py::PathIterator path;
double rect_x1, rect_y1, rect_x2, rect_y2;
int filled = 0;
const char *names[] = { "path", "rect_x1", "rect_y1", "rect_x2", "rect_y2", "filled", NULL };
bool result;

if (!PyArg_ParseTupleAndKeywords(args,
kwds,
"O&dddd|i:path_intersects_rectangle",
(char **)names,
&convert_path,
&path,
&rect_x1,
&rect_y1,
&rect_x2,
&rect_y2,
&filled)) {
return NULL;
}

CALL_CPP("path_intersects_rectangle", (result = path_intersects_rectangle(path, rect_x1, rect_y1, rect_x2, rect_y2, filled)));

if (result) {
Py_RETURN_TRUE;
} else {
Py_RETURN_FALSE;
}
}

const char *Py_convert_path_to_polygons__doc__ =
"convert_path_to_polygons(path, trans, width=0, height=0)";

Expand Down Expand Up @@ -819,6 +852,7 @@ extern "C" {
{"affine_transform", (PyCFunction)Py_affine_transform, METH_VARARGS, Py_affine_transform__doc__},
{"count_bboxes_overlapping_bbox", (PyCFunction)Py_count_bboxes_overlapping_bbox, METH_VARARGS, Py_count_bboxes_overlapping_bbox__doc__},
{"path_intersects_path", (PyCFunction)Py_path_intersects_path, METH_VARARGS|METH_KEYWORDS, Py_path_intersects_path__doc__},
{"path_intersects_rectangle", (PyCFunction)Py_path_intersects_rectangle, METH_VARARGS|METH_KEYWORDS, Py_path_intersects_rectangle__doc__},
{"convert_path_to_polygons", (PyCFunction)Py_convert_path_to_polygons, METH_VARARGS|METH_KEYWORDS, Py_convert_path_to_polygons__doc__},
{"cleanup_path", (PyCFunction)Py_cleanup_path, METH_VARARGS, Py_cleanup_path__doc__},
{"convert_to_string", (PyCFunction)Py_convert_to_string, METH_VARARGS, Py_convert_to_string__doc__},
Expand Down
Loading