Repository navigation
[Bug]: Wedge tight bounding box incorrect #21931
Description
Bug summary
The tight bounding box of a Wedge patch appears to be incorrect. I suspect this is HiDPI issue - multiplying x0/y0/width/height by 2 seems to result in the correct bounding box for me.
Code for reproduction
import matplotlib.pyplot as plt
from matplotlib.patches import Rectangle, Wedge
fig, ax = plt.subplots()
wedge = Wedge(0.5, 0.4, 10, 30)
ax.add_patch(wedge)
# Plot bounding box
renderer = fig.canvas.get_renderer()
bbox = wedge.get_tightbbox(renderer)
rect = Rectangle((bbox.x0, bbox.y0), width=bbox.width, height=bbox.height,
linewidth=72./fig.dpi, edgecolor="red", facecolor="none",
transform=None)
fig.add_artist(rect)
plt.show()Actual outcome
Expected outcome
Red bounding box bounds the blue wedge.
Additional information
No response
Operating system
macOS
Matplotlib Version
3.6.0.dev1023+gb0f7b8aaf8
Matplotlib Backend
Qt5Agg
Python version
3.9.7
Jupyter version
No response
Installation
git checkout
Activity
dstansby commentedon Dec 11, 2021
This bisects to 877598d from #19126
dstansby commentedon Dec 11, 2021
Tagged as 3.5.2 as it looks like this is a newly introduced bug in 3.5.x
jklymak commentedon Dec 12, 2021
Right,
get_window_extentis returning virtual pixels, but IdentityTransform which is used byWedgeis working in physical pixels. I'm not sure that we have a virtual pixel transform.dstansby commentedon Dec 12, 2021
I just noticed that if I get the bounding box after calling
plt.show, it is correct, ie.So it looks like
get_tightbboxis missing out on a bit of logic to get the device pixel ratio.jklymak commentedon Dec 12, 2021
Ok so this is like many things - the bbox is only properly set after a draw_without_rendering. I should have thought of that. There is a lot of other stuff like this, so im not sure we should do anything here?
dstansby commentedon Dec 12, 2021
Even putting a
fig.canvas.draw()before getting the bounding box doesn't work in this case.jklymak commentedon Dec 12, 2021
Weird, so what extra happens with doing plt.show that doesn't happen for
draw_without_rendering?tacaswell commentedon Dec 14, 2021
I think @dstansby hit the nail on the head as we update the dpi ratio when we actually show the window (which in the case of a system with multiple screens with different dpis is the first time we actually know this information).
There is a way to set the transforms on one Artist to be relative units to the bounding box of another (but I do not know how off the top of my head). Doing it that will will help make sure that things are updated on every draw (in case you change the dpi by moving between screens or just resize the window) and gives us a chance to make sure that the artists are updated in the right order (or at least raise if they are drawing in the wrong order even if we can not fix it).
jklymak commentedon Dec 15, 2021
The underlying problem is that our "native" co-ordinate system is in pixels, and the number of pixels in the figure can change without warning.
I think we need to re-engineer so that virtual pixels as the "native" co-ordinate system; it might as well be 72 dpi for consistency with PDF and EPS (which would effectively make our virtual pixels typographic points in physical space). All extents etc get translated to these virtual pixels. Only at draw time would the requested "real" dpi get applied.
I hope its clear that dpi before draw time is completely arbitrary, and that we change the figure dpi all the time when we draw to agg, pdf, etc. I don't think it serves any purpose to know the dpi while composing the figure.
tacaswell commentedon Dec 15, 2021
I'm pretty sold on this. We already have a
dpi_scale_trans(and have back to at least 2008) which goes from inches -> pixels, naively I think we can split that in half to inches -> virtual and virtual -> real. We could then directly use the virtual -> real transform for things like bounding boxes / absolute positioning.I think just providing
inches_to_pts_transandpts_to_pixels_transas attributes on the figure would be enough to save @dstansby (get the bounding box, invert to pts, set teh transform to befig.pts_to_pixels_trans)The "easy" change is to add a second copy of all the cannonical transforms that targets a step early, but that would probably just make it worse (2x as many transforms, users would have to "opt in" to get the right units).
The "hard" change here is to re-target all of the "default" transforms to point at the virtual pixels. If we adjust the default transfroms
Identity->pts_to_pixels_transin a few places I think a big chunk of stuff will "just work", but we will almost certainly have silent breakage and maybe a justification for mpl4.0.I think we should look into providing the two additional tarnsforms for 3.6, not do the "easy" change ever, and put the thought about the "hard" changes on the stack of big things to think about writing a grant against.
jklymak commentedon Dec 15, 2021
I think
dpi_scale_transonly shows up in the main library 20 or so times. And then another 20 or so times in examples and tutorials. Thenfig.dpiprobably appears a bunch more times. So I think you are correct that both a) it would be pretty easy to get 95% correct and b) there will be some hard fails.OTOH, the current system has quite a few hard fails, and we seem to be playing whack-a-mole with when the dpi gets applied. I'd hope this could fix a fair bit of this by separating the two concepts.
Just to clarify one thing - we don't have an "inches" co-ordinate system. inches are currently derived from pixels/dpi via
dpi_scale_trans. So adding "points" would give us a "truth" in "inches", and then what pixels things are drawn at could be figured out at draw time.jklymak commentedon Dec 16, 2021
BTW, @dstansby I think the foolproof way to fix this is to not use
transform=Nonebut instead use a well-defined transform (ax.transDatawould work as well):6 remaining items