Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Replace quiver dpi callback with reinit-on-dpi-changed.#22807
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Instead of registering a callback to update itself whenever the dpichanges, do the same thing as every other artist which depends on dpi(e.g. FancyArrowPatch), i.e. update oneself as needed. We can stillsave duplicate calculations by only doing the init if the figure dpichanged compared to last time.Also drop _new_UV, which is never read.
Nice! Thanks for the quick fix. I had just started looking into this as well and was going to change to using "resize_event" on the canvas which is what is really desired I think, but this is nicer :) On that note, I assume you realized that "dpi_changed" isn't used anywhere else in the codebase, so I would vote for deprecating that in another PR too... Ithink |
Not looking at the details, but should these things be done in trans_dpi_acale? |
I'm not sure that that would be possible as these are arrows positioned in data space, but whose size is determined by screen space. |
We have this problem relatively often, at least with arrows - we want a glyph to have one dimension in data space, but the shape of the arrow heads in physical space. This doesn't seem too hard to do in physical space at draw time. Transfer the anchors to physical space and then draw the arrow and let its transform be physical space (for which we need a better name than trans_dpi_scale) if we do that dpi changes happen at the renderer level and the upper level code doesn't need a callback. |
Is this only used for labelsep? Can we place text with an offset transform? |
| @@ -562,18 +525,19 @@ def _init(self): | |||
| """ | |||
| # It seems that there are not enough event notifications | |||
| # available to have this work on an as-needed basis at present. | |||
| if True: #notself._initialized: | |||
| if True: #self._dpi_at_last_init !=self.axes.figure.dpi | |||
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.
Why did you change the comment? why does this code get run unconditionally?
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.
I don't know why the original check got commented out (and don't really want to investigate right now), I'm just replacing _initialized by its newer moral equivalent.
| def _init(self): | ||
| if True: # not self._initialized: | ||
| if not self.Q._initialized: | ||
| if True: # self._dpi_at_last_init != self.axes.figure.dpi |
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.
I don't understand why this condition is here if its always true?
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.
As above.
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.
This seems good. I still think quiver should not have a concept of dpi, but I think this change could go in before a significant refactoring. Definitely prefer this to a callback.
have you tested changing the dpi? I'm not even sure how we do that.
Agree that deprecating dpi_changed is perhaps a good idea, but this pr is really just aiming for the minimum fix. Ditto for using offset_transform or dpi_scale_trans instead. |
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.
I tested this on my multi-monitor mixed-dpi setup and it looks good to me. I think this is good to go for now and there are a few follow-up things that can be done in subsequent PRs.
@meeseeksdev backport to v3.5.x |
Could not push to auto-backport-of-pr-22807-on-v3.5.x due to error, aborting. |
@meeseeksdev backport to v3.5.x |
Could not push to auto-backport-of-pr-22807-on-v3.5.x due to error, aborting. |
meeseeksdev backport to v3.5.x |
…807-on-v3.5.xBackport PR#22807 on branch v3.5.x (Replace quiver dpi callback with reinit-on-dpi-changed.)
Instead of registering a callback to update itself whenever the dpi
changes, do the same thing as every other artist which depends on dpi
(e.g. FancyArrowPatch), i.e. update oneself as needed. We can still
save duplicate calculations by only doing the init if the figure dpi
changed compared to last time.
Also drop _new_UV, which is never read.
Closes#22804.
PR Summary
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).