-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix clipping of markers in PDF backend. #17163
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.
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
Conversation
Can check carefully in a second, but doesn't the maximum amount of extra clipping box needed depend on the miter limit? I would believe that it happens to occur at 45 degrees, but maybe documenting it that way is misleading? Edit: default PDF miter limit appears to be 10 (https://help.adobe.com/pdfl_sdk/18/PDFL_SDK_HTMLHelp/PDFL_SDK_HTMLHelp/API_References/PDFL_API_Reference/PDFEdit_Layer/General.html), which means the longest possible miter will occur for an angle of around 11.5 degrees (https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke-miterlimit). |
I don't believe so, because all markers have cap style of 'butt'. |
Oh, default joinstyle is 'round', but some are 'miter'. I was a bit worried about that, but no markers in our tests or examples seem to have extended passed the sqrt(2) limit. |
Yeah the only ones which are miter are e.g. Nix what I said up there. We now allow custom markers and this will fail for those. I would recommend using an extra buffer of size "10/2" instead of "sqrt(2)/2" to account for the miter limit of 10 (your code worked because "sqrt(2)/2" is the miter limit that starts beveling after 90 degrees). |
Actually, I guess |
Actually, I had focused on X initially, and that one is definitely 90° so 45° makes sense, but looking at triangles again, they are slightly clipped. You have to zoom in about 8x, but since PDF is vector, it's definitely there. |
I updated the scaling to 5 as you said, to match the miter limit, and threw in some spec references. This fixes the triangles, 'v', '^', '<', '>', and diamond 'd', which are all markers with sharper corners. I don't think my test triggers it, but I think we'd have to use huge markers to really see it. I'm a bit confused as to why, but I can confirm there's a difference when using the marker reference example. |
e87606d
to
ed79844
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test seems to check what we want. The fix and comments seem readable. LGTM.
I think making the image tests larger so that it's more precise (to catch 'd', '<', etc.) is getting to be diminishing returns, I'd merge as-is.
Why did you need to change the baseline image? Here tests seem to pass without changing it. |
TBH, I'm not sure, but I think it has to do with Ghostscript rasterization. If I run diffpdf on it, it sees no difference, nor if I just look at the two in a PDF viewer. |
The tests fail with the old baselines for you? |
They do, with ghostscript 9.27, if that's the problem. |
I guess I would rather have this go on top of #17603 to avoid having to add a new baseline image. |
The bbox only contains the points of the marker, but the line will extend outside by half the line width, unless it's mitered. The PDF miter limit is 10, so pad by 5*line width (half the miter length). This fixes corners on 'v', '^', '<', '>', 'p', 'h', 'H', 'D', 'd', 'X'. Fixes matplotlib#9829.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Postci.
The bbox only contains the points of the marker, but the line will extend outside by half the line width. This was handled before, except when the line is at an angle to the edge, as then the line outside is wider than half a line width. The maximum is at 45°, or √2.
This fixes corners on 'v', '^', '<', '>', 'p', 'h', 'H', 'D', 'd', 'X'.
Fixes #9829.
PR Summary
PR Checklist