-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Move visibility check to allow_rasterization #12040
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
Comments
Having Using a An additional and independent decorator
by a single line
One would also need to make sure different draw methods do not use some more sophisticated and/or more individualized logic, in which case at least part of the logic still needs to exist in the Readability matters; and I feel like in this case the benefits of using a decorator are too low to sacrifice readablity. I think this would be different if a performance gain was to be expected from using a decorator. But I don't see that to be the case. |
Well, the point above is that the first case I found of not checking visibility is a bug (quiverkey), and I'd be puzzled if there were cases where we really don't want to check it -- just like not allowing rasterization for 3D artists was an oversight (#10013). |
This issue has been marked "inactive" because it has been 365 days since the last comment. If this issue is still present in recent Matplotlib releases, or the feature request is still wanted, please leave a comment and this label will be removed. If there are no updates in another 30 days, this issue will be automatically closed, but you are free to re-open or create a new issue if needed. We value issue reports, and this procedure is meant to help us resurface and prioritize issues that have not been addressed yet, not make them disappear. Thanks for your help! |
Let's just track the actual visibility bug at #25890. |
Currently, most implementations of draw() start with a check on get_visible() to immediately return if the artist is not visible. Most implementations of draw() are also decorated with allow_rasterization to, well, allow rasterization.
An exception to the former is QuiverKey; modifying e.g. the quiver_simple_key demo shows that QuiverKey does not respect the visibility flag.
While this could be fixed in place, I'd suggest instead to move the visibility check to the top of allow_rasterization, which will allow removing it from all actual implementations (of course, if there are artists that really don't want to be rasterizable (if there are any?), then they will need their own visibility check, and converserly if they don't want to obey the visibility flag (but I'd think most cases are just bugs)).
... in which case it may be worth renaming allow_rasterization to e.g. draw_wrapper or check_visibility_and_allow_rasterization (depending on the extent to which we want the name to state the functionality). Or we can just add another check_visibility decorator, but I guess it's a bit performant to have just one wrapper instead of two (no numbers to confirm that statement).
The text was updated successfully, but these errors were encountered: