Fix PDF marker culling with translated master transform#32003
Fix PDF marker culling with translated master transform#32003Ayush-0918 wants to merge 1 commit into
Conversation
|
Thank you for opening your first PR into Matplotlib! If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks. We also ask that you please finish addressing any review comments on this PR and wait for it to be merged (or closed) before opening a new one, as it can be a valuable learning experience to go through the review process. You can also join us on discourse chat for real-time discussion. For details on testing, writing docs, and our review process, please see the developer guide. We strive to be a welcoming and open project. Please follow our Code of Conduct. |
There was a problem hiding this comment.
Pull request overview
This pull request addresses incorrect marker culling in the PDF backend’s draw_path_collection optimization when master_transform includes a translation (notably impacting hexbin where offsets may be delta-transformed).
Changes:
- Adjusts the visibility/culling check to account for the translation component of
master_transformwhen determining whether a marker is within (or intersects) the PDF canvas. - Keeps the existing rendering coordinates unchanged while altering only the culling decision path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m = master_transform.get_matrix() | ||
| tx = m[0, 2] | ||
| ty = m[1, 2] | ||
|
|
||
| xo2 = xo + tx |
| extent_x, extent_y = path_extent_map[path_id] | ||
| if not (-extent_x <= xo <= canvas_width + extent_x | ||
| and -extent_y <= yo <= canvas_height + extent_y): | ||
| if not (-extent_x <= xo2 <= canvas_width + extent_x | ||
| and -extent_y <= yo2 <= canvas_height + extent_y): | ||
| continue |
|
Thank you for your interest in contributing to Matplotlib. There is already PR #32000 for this issue, and we generally prioritise ones that are opened first. So closing this here. |
closes #31999
PR summary
This PR fixes incorrect marker culling in the PDF backend when
master_transformincludes a translation.The visibility check was using untranslated marker coordinates, which could incorrectly skip markers that become visible after the translation is applied. This could lead to missing markers in the generated PDF.
This change applies the translated coordinates only for the visibility check, while keeping the existing rendering coordinates unchanged. This preserves the current rendering behavior and fixes the culling logic.
Testing
hexbincells after path collection optimization #31999.AI Disclosure
I used ChatGPT as a debugging assistant to help reason about the rendering logic and discuss possible approaches. I implemented the final solution myself, verified the behavior locally, and reviewed the final changes before submitting this pull request.
PR checklist
hexbincells after path collection optimization #31999 is in the body of the PR description