Skip to content

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

Merged
merged 1 commit into from
Jun 10, 2020

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Apr 17, 2020

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

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • [n/a] New features are documented, with examples if plot related
  • [n/a] Documentation is sphinx and numpydoc compliant
  • [n/a] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • [n/a] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@brunobeltran
Copy link
Contributor

brunobeltran commented Apr 17, 2020

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).

@brunobeltran
Copy link
Contributor

P.S. Once #16832 gets merged, I'll be able to clean up #16607 to compute the "exact" Bbox for an arbitrary path given the stroke width and miter limit. Just dropping this reference here so I remember to also fix this code to set an exact clipping path once this is possible.

@QuLogic
Copy link
Member Author

QuLogic commented Apr 17, 2020

I don't believe so, because all markers have cap style of 'butt'.

@QuLogic
Copy link
Member Author

QuLogic commented Apr 17, 2020

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.

@brunobeltran
Copy link
Contributor

Yeah the only ones which are miter are e.g. X and other 90 degree angle-heavy guys. Since this code is marker-specific (and isn't called for general paths, where we might have much more acute angles), I'd say it's good to go as-is, but would still recommend rewording the comment to clarify that the only reason this works is because the built-in markers never have angles more acute than 90 degrees.

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).

@brunobeltran
Copy link
Contributor

brunobeltran commented Apr 17, 2020

Actually, I guess MarkerStyle._joinstyle is private, so 45 degrees should work for now. Although we will have to be careful to fix this in the future if we go down a path like suggested in #16773.

@QuLogic
Copy link
Member Author

QuLogic commented Apr 17, 2020

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.

@QuLogic
Copy link
Member Author

QuLogic commented Apr 17, 2020

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.

Copy link
Contributor

@brunobeltran brunobeltran left a 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.

@anntzer
Copy link
Contributor

anntzer commented Apr 18, 2020

Why did you need to change the baseline image? Here tests seem to pass without changing it.

@QuLogic
Copy link
Member Author

QuLogic commented Apr 20, 2020

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.

@anntzer
Copy link
Contributor

anntzer commented Apr 20, 2020

The tests fail with the old baselines for you?

@QuLogic
Copy link
Member Author

QuLogic commented Apr 20, 2020

They do, with ghostscript 9.27, if that's the problem.

@QuLogic QuLogic added this to the v3.3.0 milestone May 26, 2020
@anntzer
Copy link
Contributor

anntzer commented Jun 9, 2020

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.
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

Postci.

@QuLogic QuLogic merged commit 21c3acb into matplotlib:master Jun 10, 2020
@QuLogic QuLogic deleted the pdf-marker-clip branch June 10, 2020 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vertices clipped for certain markers when plotting more than two points and saving as pdf
4 participants