Skip to content

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

Closed
anntzer opened this issue Sep 6, 2018 · 4 comments
Closed

Move visibility check to allow_rasterization #12040

anntzer opened this issue Sep 6, 2018 · 4 comments
Labels
status: inactive Marked by the “Stale” Github Action

Comments

@anntzer
Copy link
Contributor

anntzer commented Sep 6, 2018

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

@ImportanceOfBeingErnest
Copy link
Member

Having allow_rasterization manage whether the function decorated with it would actually do something would be confusing, simply because there is no way of judging on that behaviour from its name.

Using a check_visibility_and_allow_rasterization decorator will lead to the need of a check_visibility_and_dont_allow_rasterization as well, which creates an unnecessary double structure.

An additional and independent decorator check_visibility may be an option, but is it really good style to defer a check like this to a decorator? Also the net gain would be to replace two lines

if not self.get_visible():
    return

by a single line

@check_visible

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 draw method itself.

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.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 6, 2018

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).
Having a single decorator to apply around draw() would make this less likely to be forgotten. I agree that splitting into a second decorator mostly defeats the purpose of the proposal.

@github-actions
Copy link

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!

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label May 15, 2023
@anntzer
Copy link
Contributor Author

anntzer commented May 15, 2023

Let's just track the actual visibility bug at #25890.

@anntzer anntzer closed this as completed May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: inactive Marked by the “Stale” Github Action
Projects
None yet
Development

No branches or pull requests

2 participants