Skip to content

Improve selection of inset indicator connectors. #12334

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 29, 2018
Merged

Improve selection of inset indicator connectors. #12334

merged 1 commit into from
Sep 29, 2018

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Sep 29, 2018

PR Summary

Instead of selecting just one pair of corners, determine whether each connector is visible individually based on relative position of the bbox corners that each one connects.
It took me a couple of false starts, but ended up relatively simple in the end.

Fixes #12323.

Since it's marked experimental, I'll leave it to @jklymak to determine milestoning, and then write up the changelog accordingly, and maybe update the test too.

On master

master

This PR

fix

PR Checklist

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

@QuLogic
Improve selection of inset indicator connectors.
ac2afc6 
Instead of selecting just one pair of corners, determine whether each
connector is visible individually based on relative position of the bbox
corners that each one connects.
@jklymak jklymak added this to the v3.0.x milestone Sep 29, 2018
jklymak
jklymak approved these changes Sep 29, 2018
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Very elegant!

@jklymak
Copy link
Member

jklymak commented Sep 29, 2018

This is great. You should include the code for the above as an example somewhere too!

I assume this works as well w an oblong shape as well?

@ImportanceOfBeingErnest
Copy link
Member

Cool stuff.

If you look closely there is a little hickup at ~0°. (Image taken from screen capture of the above animation)

image

I'm not able to reproduce this locally. Is this expected?

@QuLogic
Copy link
Member Author

QuLogic commented Sep 29, 2018

Hmm, that is weird, but the way ConnectionPatchs seem to work is that they need an extra draw to make sure everything is in place. I suspect this is some artifact of that combined with the animation code to produce the first frame a bit too early. You can see this sort of effect if you just run a static plot and resize the window quickly.

@QuLogic
Copy link
Member Author

QuLogic commented Sep 29, 2018

Oblong shapes seem fine (and should be since this looks at individual corners now):
fix2

@jklymak
Copy link
Member

jklymak commented Sep 29, 2018

For the drawing bug have a look at: #11753

@ImportanceOfBeingErnest ImportanceOfBeingErnest merged commit 97cad3d into matplotlib:master Sep 29, 2018
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Sep 29, 2018
@ImportanceOfBeingErnest
Backport PR matplotlib#12334: Improve selection of inset indicator co…
304d3d5 
…nnectors.
@meeseeksmachine meeseeksmachine mentioned this pull request Sep 29, 2018
Merged
@ImportanceOfBeingErnest ImportanceOfBeingErnest mentioned this pull request Sep 29, 2018
Closed
@QuLogic QuLogic deleted the inset-indicators branch September 29, 2018 20:31
ImportanceOfBeingErnest added a commit that referenced this pull request Sep 30, 2018
@ImportanceOfBeingErnest
Merge pull request #12342 from meeseeksmachine/auto-backport-of-pr-12…
9990884 
…334-on-v3.0.x

Backport PR #12334 on branch v3.0.x (Improve selection of inset indicator connectors.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@ImportanceOfBeingErnest ImportanceOfBeingErnest ImportanceOfBeingErnest approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.0.1
Development

Successfully merging this pull request may close these issues.

indicate_inset_zoom sometimes draws incorrect connector lines
3 participants
@QuLogic @jklymak @ImportanceOfBeingErnest