8000 `_path.get_extents` does not correctly handle bezier curves · Issue #16830 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

_path.get_extents does not correctly handle bezier curves #16830

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
brunobeltran opened this issue Mar 19, 2020 · 1 comment · Fixed by #16832
Closed

_path.get_extents does not correctly handle bezier curves #16830

brunobeltran opened this issue Mar 19, 2020 · 1 comment · Fixed by #16832
Labels
topic: path handling
Milestone
v3.3.0

Comments

@brunobeltran
Copy link
Contributor
brunobeltran commented Mar 19, 2020 •
edited
Loading

Bug report

Bug summary

Currently, .Path extents are calculated by _path.h as simply the Bbox of the control points.

For bezier curves, the convex hull of the control points can be easily shown to contain the path. So these extents are a correct upper bound. However, especially for cubic curves, this upper bound will often not be even approximately equal. (See example below).

Some thoughts:

  1. Doing this correctly is required in order to correctly implement "width", "bbox-width", and "bbox-area" for MarkerStyle.normalization as initially proposed in Sizes of different markers are not perceptually uniform #15703 and discussed in Remove more API deprecated in 3.1 #16772.
  2. Presumably this hasn't been a problem in the past because in practice, matplotlib primarily deals with lines, where the control points are exactly the extents.
  3. It might make sense to leave the current Path.get_extents code, and implement a Path.get_exact_extents to be used upon MarkerStyle.__init__ to pre-calculate/cache the Bbox of that marker.

This solution prevents incurring a (presumably significant) performance penalty for something that clearly has not been a big issue in the past, allows us to still quickly compute the Bbox of a collection of markers by adding the appropriate padding to the Bbox of the marker centers, and doesn't change existing API.

Someone who's more familiar with the Agg internals is free to translate my Python code if we decide it's worth it to implement a "fast" version of the "correct" solution for use on all Path's.

I will submit a PR to implement Path.get_exact_extents Python-side (not much work), but I wanted to open this up for discussion.

Code for reproduction

import matplotlib.pyplot as plt
from matplotlib.path import Path
from matplotlib.transforms import Bbox
from matplotlib import patches

hard_curve = Path([[0, 0],
                   [4, 0], [-3, 1], [1, 1]],
                  [Path.MOVETO,
                   Path.CURVE4, Path.CURVE4, Path.CURVE4])
reported_extents = hard_curve.get_extents()
actual_extents = Bbox([[-0.3, 0], [1.3, 1]])
fig, ax = plt.subplots()
patch = patches.PathPatch(hard_curve, facecolor='orange', lw=2)
ax.add_patch(patch)
ax.set_xlim(-1, 2)
ax.set_ylim(-0.1, 1.1)
ax.set_title(f"Path extents are {reported_extents};\nShould be ~{actual_extents}.")

Actual outcome

issue-image

# If applicable, paste the console output here
#
#

Expected outcome

See title of plot.

Matplotlib version

  • Operating system: Debian 9 Jessie
  • Matplotlib version: master
  • Matplotlib backend (print(matplotlib.get_backend())): qt5agg
  • Python version: 3.7.3
  • Jupyter version (if applicable): N/A
  • Other libraries: N/A
@brunobeltran
Copy link
Contributor Author

Ah whoops, this has in fact been reported before, I just didn't search well enough. Basically dup of #7184.

This was referenced Mar 19, 2020
Merged
Merged
Draft
@QuLogic QuLogic added this to the v3.3.0 milestone Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic: path handling
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging a pull request may close this issue.

Correctly compute path extents brunobeltran/matplotlib
2 participants
@QuLogic @brunobeltran
0