Skip to content
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

Add arrow curvature to sources and monitors with bend radius (#272) #714

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

lucas-flexcompute
Copy link
Collaborator

Curved arrows are only possible through FancyArrowPatch, but those define the arrow head size in display coordinates and end points in data coordinates. Furthermore, the arrow head is included in the total length, as opposed to what happens with Axes.arrow, that was used before. The result is that the arrow length calculated based on the axes limits or simulation boundaries can end up too small for the arrow head (and for visualization as well).

The solution here, which might actually be preferable even without the bending, is to define the arrow size in real units (inches, following matplotlib's DPI setting) so that the arrows are always the same size. That can only be accomplished by waiting for the transformations to be set, at drawing time, so we use a draw_event callback to calculate the arrow shape.

If the drawing window is resized, the arrow length will change, though, because the callback is only called once (to avoid the danger of inifite loops).

Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

I just tested this out on the Modes_bent_angled and a couple other notebooks and it looks fantastic, thanks.

image

image

I think the solution you described with the arrow head size makes sense, even for straight arrows.

Only things are:

  • maybe the arrows could be a little bit longer? to truly show some of the curvature? what do you think?
  • there are some pylint errors

Thanks a lot!

@tylerflex tylerflex added the 1.9 label Feb 23, 2023
@tylerflex
Copy link
Collaborator

Can you also throw a line in the changelog as well? Thanks

@lucas-flexcompute
Copy link
Collaborator Author

maybe the arrows could be a little bit longer? to truly show some of the curvature? what do you think?

I tried to keep them only large enough to be easily visible, but not taking too much space (they are not usually the user focus), but I can definitely increase the default a bit.

there are some pylint errors
Can you also throw a line in the changelog as well?

I'll look into those as soon as I can. I have something urgent to take care of, but I'll get to it soon

@lucas-flexcompute
Copy link
Collaborator Author

Increased the size a bit. How's that look (medium, small and large simulations)?

20230224-161130
20230224-161146
20230224-161251

@tylerflex
Copy link
Collaborator

Looks good to me! what is the last plot? is it supposed to curve out of plane like that?
@momchil-flex any thoughts?

This is awesome, thanks @lucas-flexcompute !

@lucas-flexcompute
Copy link
Collaborator Author

what is the last plot? is it supposed to curve out of plane like that?

That's just a test for a long and thin simulation bound, and they are curved around the different axis for testing as well.

@momchil-flex momchil-flex changed the base branch from develop to pre/1.9.0 February 24, 2023 21:17
@momchil-flex
Copy link
Collaborator

Hi Lucas, unfortunately we didn't notice this earlier, but this PR is meant to go into pre/1.9.0, not develop. I changed the base and now there are some conflicts. I wonder if you can fix that in a way that's not too painful? You could try branching off pre/1.9.0 and cherry-picking your new commits, for example.

@lucas-flexcompute
Copy link
Collaborator Author

@momchil-flex No problem, there were only small conflicts.

@momchil-flex
Copy link
Collaborator

Awesome, thanks!

Curved arrows are only possible through FancyArrowPatch, but those
define the arrow head size in display coordinates and end points in data
coordinates.  Furthermore, the arrow head is included in the total
length, as opposed to what happens with Axes.arrow, that was used
before.  The result is that the arrow length calculated based on the
axes limits or simulation boundaries can end up too small for the arrow
head (and for visualization as well).

The solution here, which might actually be preferable even without the
bending, is to define the arrow size in real units (inches, following
matplotlib's DPI setting) so that the arrows are always the same size.
That can only be accomplished by waiting for the transformations to be
set, at drawing time, so we use a `draw_event` callback to calculate the
arrow shape.

If the drawing window is resized, the arrow length will change, though,
because the callback is only called once (to avoid the danger of inifite
loops).

Signed-off-by: Lucas Heitzmann Gabrielli <[email protected]>
@momchil-flex
Copy link
Collaborator

I removed two commits that had been picked up from the develop branch.

@momchil-flex momchil-flex merged commit df008ef into pre/1.9.0 Feb 24, 2023
@momchil-flex momchil-flex deleted the lucas/bent_arrows branch February 24, 2023 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants