Masking boundaries #1569

bnmajor · 2023-09-06T12:55:20Z

NEEDS:

Re-test LLNL workflow to make sure InteractiveTemplate changes have not broken anything
Re-test mask regions by rectangle/ellipse
- Toggle tabbed mode while masking
- Toggle view mode while masking
- Saving/loading state files with masks

KNOWN ISSUES:

In the raw, untabbed view mode rotation does not work (the template disappears) on any detector that is not the first. Rotation by arrow key works, translate works, and everything works in tabbed mode.
Rotation in the polar view skews the template
Create template(s) -> interact w/ another dialog (i.e. mask manager) -> try to draw another mask -> nothing happens (works again if the shape is toggled and then mask is drawn)
~~Draw template(s) in raw -> switch to tabbed view -> draw template but do not save it -> switch to polar view -> cannot immediately draw on canvas~~

Decouples the InteractiveTemplate class from the LLNL import tool so that it can be used more broadly across the application. The class can now accept a list of vertices in raw coordinates and create a polygon, rectangle, or ellipse. If the static_mode attribute is set to False the user can translate or rotate the shape. Signed-off-by: Brianna Major <[email protected]>

Once a user draws the bounding rectangle or ellipse for the region they'd like to mask they are able to translate and rotate the shape just like they can in the LLNL import tool. Right-click will complete the current patch and allow starting a new one. Signed-off-by: Brianna Major <[email protected]>

If the canvas is changed (by tab, tab mode or image mode) complete the current masks. Signed-off-by: Brianna Major <[email protected]>

Signed-off-by: Brianna Major <[email protected]>

bnmajor · 2023-09-06T13:24:08Z

@psavery @saransh13 While I wrap this branch up I'd welcome any thoughts or opinions on the best way to interact with the rectangle/ellipse masking. Right now the user:

Selects the shape
Draws the bounds
Translates/rotates the bounds
Right-clicks to complete the current mask and enable drawing the next

Do we need an additional or different option for indicating that the current mask is complete? Something that indicates that the template is in "interactive" (can be moved) mode still? I was thinking about the current mask being drawn in red and completed masks being drawn in black or something along those lines.

psavery

Looking really nice so far! I have mostly minor suggestions for the code. I'll give it a try soon too.

hexrd/ui/hexrd_config.py

hexrd/ui/interactive_template.py

hexrd/ui/mask_regions_dialog.py

Instead, update the current canvas on image mode or tab mode change with signals. This behavior now matches the HandDrawnMaskDialog. Signed-off-by: Brianna Major <[email protected]>

Signed-off-by: Brianna Major <[email protected]>

Renamed: patch(es) -> interactive_template(s), raw_axes -> axis, kwargs -> polygon_kwargs Signed-off-by: Brianna Major <[email protected]>

Signed-off-by: Brianna Major <[email protected]>

psavery · 2023-09-06T20:17:43Z

@bnmajor Did you try rotating the masks in the polar view? The behavior appears a little odd.

Ellipses in the raw view don't look as smooth as they used to. Do you know why?

If I draw a mask and then right-click to save it, then click "Undo Last Selection", I get an exception.

Also, can we make sure all of the signals get disconnected when the mask manager is hidden? It looks to me that if I use the mask manager several times in one session, the program slows down.

Also, we should get feedback from @saransh13, but it might be more intuitive to change the clicking behavior to be like this:

If the user left-clicks inside any template, then translate that template
If the user left-clicks outside of any template, then save the previous template and start a new template
Right-click won't do anything

However, I wonder if it will be difficult to allow the user to translate templates that were already saved (like allow them to translate any template they have already created). If that would be difficult, maybe we can skip that part.

Overall, it is looking very nice, though!

Signed-off-by: Brianna Major <[email protected]>

bnmajor · 2023-09-08T13:38:27Z

@bnmajor Did you try rotating the masks in the polar view? The behavior appears a little odd.

I did not test rotate in the polar view but there is indeed some strange behavior... Rotation causes some strange skewing. I will get that sorted out.

Ellipses in the raw view don't look as smooth as they used to. Do you know why?

We're now drawing the ellipses with the more generic patches.Polygon in the interactive template. We still use the Eliipses polygon for the initial drawing so that we can easily draw the shape and then grab the coordinates from it. The InteractiveTemplate uses Polygon for everything to stay as general as possible.

Also, can we make sure all of the signals get disconnected when the mask manager is hidden? It looks to me that if I use the mask manager several times in one session, the program slows down.

I've added a commit with a lot more precautionary disconnect calls that will hopefully help with this problem!

bnmajor · 2023-09-08T13:42:52Z

If I draw a mask and then right-click to save it, then click "Undo Last Selection", I get an exception.

Also, we should get feedback from @saransh13, but it might be more intuitive to change the clicking behavior to be like this:

If the user left-clicks inside any template, then translate that template

If the user left-clicks outside of any template, then save the previous template and start a new template

Right-click won't do anything

However, I wonder if it will be difficult to allow the user to translate templates that were already saved (like allow them to translate any template they have already created). If that would be difficult, maybe we can skip that part.

I think that this is all much smoother now! I've made a few updates and the behavior is now as follows:

User draws a shape in a region
Any existing template can be manipulated by selecting it
1. 1. Click on a template and it will turn red if selected
2. Translate and rotate as before
Click away from the selected template or simply begin drawing to create a new template shape
All templates can be undone now, not just interactive templates

Signed-off-by: Brianna Major <[email protected]>

pep8speaks · 2023-09-20T15:49:55Z

Hello @bnmajor! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-09-20 20:23:49 UTC

This makes the ellipses in the polar view appear much smoother. Signed-off-by: Patrick Avery <[email protected]>

This aspect ratio takes into account both the extent and the canvas aspect ratio. Signed-off-by: Patrick Avery <[email protected]>

We only need to confirm that we are in the correct detector in the raw image mode. Signed-off-by: Brianna Major <[email protected]>

I accidentally inverted this... Signed-off-by: Patrick Avery <[email protected]>

psavery

This is working well for me, thanks @bnmajor!

The one thing we might want to do in a future PR: if users are interacting with these masks on a high-resolution polar canvas, we probably want to animate the interactions so that they are much faster. But we will work on that only if someone requests it.

@saransh13, can you try this out and let us know if you have any suggested changes?

saransh13 · 2023-09-20T18:20:03Z

👍🏽 I’ll have feedback on this PR later today.

…

On Wed, Sep 20, 2023 at 11:13 AM Patrick Avery ***@***.***> wrote: ***@***.**** approved this pull request. This is working well for me, thanks @bnmajor <https://github.com/bnmajor>! The one thing we might want to do in a future PR: if users are interacting with these masks on a high-resolution polar canvas, we probably want to animate the interactions so that they are much faster. But we will work on that only if someone requests it. @saransh13 <https://github.com/saransh13>, can you try this out and let us know if you have any suggested changes? — Reply to this email directly, view it on GitHub <#1569 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADYZ2U453C235RMITY3UIZ3X3MW4FANCNFSM6AAAAAA4NJMDQI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

bnmajor · 2023-09-20T19:21:13Z

After speaking with @saransh13 this branch needs the following:

Larger step size for rotate by arrow key. This should only apply to masking, not the LLNL import tool
Bug fix: in polar view draw mask A -> move mask A -> draw mask B -> move mask B -> mask A moves as well

saransh13 · 2023-09-20T19:26:36Z

Forgot to mention, the bug was with the arrow key. I did not test the same behavior with the Mouse.

…

On Wed, Sep 20, 2023 at 12:21 PM Brianna Major ***@***.***> wrote: After speaking with @saransh13 <https://github.com/saransh13> this branch needs the following: - Larger step size for rotate by arrow key. This should only apply to masking, not the LLNL import tool - Bug fix: in polar view draw mask A -> move mask A -> draw mask B -> move mask B -> mask A moves as well — Reply to this email directly, view it on GitHub <#1569 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADYZ2U7UZZMTN7PAXAZLATLX3M63JANCNFSM6AAAAAA4NJMDQI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Signed-off-by: Brianna Major <[email protected]>

bnmajor · 2023-09-20T19:52:51Z

Larger step size for rotate by arrow key. This should only apply to masking, not the LLNL import tool

Bug fix: in polar view draw mask A -> move mask A -> draw mask B -> move mask B -> mask A moves as well

These are both fixed, although the angle of rotation was bumped an arbitrary amount. This value can be set to anything here. @saransh13 @psavery

psavery

Feel free to merge when ready. We will animate the interactive template in a separate PR, and fix any more issues we encounter in future PRs.

Signed-off-by: Brianna Major <[email protected]>

bnmajor added 5 commits September 6, 2023 08:45

Match the behavior of the hand drawn mask dialog

5033ae9

If the canvas is changed (by tab, tab mode or image mode) complete the current masks. Signed-off-by: Brianna Major <[email protected]>

Add updated instructions to masking dialog

6db5ecd

Signed-off-by: Brianna Major <[email protected]>

Check for threshold mask by name before applying

289c602

Signed-off-by: Brianna Major <[email protected]>

psavery reviewed Sep 6, 2023

View reviewed changes

bnmajor force-pushed the masking-boundaries branch from 70741d4 to 511b8f6 Compare September 6, 2023 18:14

bnmajor added 7 commits September 6, 2023 14:47

Do not expect main window as parent

028f038

Instead, update the current canvas on image mode or tab mode change with signals. This behavior now matches the HandDrawnMaskDialog. Signed-off-by: Brianna Major <[email protected]>

Remove unused attribute

29f0a01

Signed-off-by: Brianna Major <[email protected]>

Simplify check for threshold mask by name

6d0e662

Signed-off-by: Brianna Major <[email protected]>

Rename for consistency

d3d7299

Renamed: patch(es) -> interactive_template(s), raw_axes -> axis, kwargs -> polygon_kwargs Signed-off-by: Brianna Major <[email protected]>

Set focus policy on canvas initialization

5fa0a37

Signed-off-by: Brianna Major <[email protected]>

Fix incorrect computation of current axes image

5ad2f1f

Signed-off-by: Brianna Major <[email protected]>

Always disconnect before setting up new canvas connections

4ab41e0

Signed-off-by: Brianna Major <[email protected]>

bnmajor force-pushed the masking-boundaries branch from 511b8f6 to 4ab41e0 Compare September 6, 2023 18:48

bnmajor requested a review from psavery September 6, 2023 18:49

Make sure all signals are disconnected

026bc03

Signed-off-by: Brianna Major <[email protected]>

bnmajor force-pushed the masking-boundaries branch from db18f37 to b16ebeb Compare September 8, 2023 13:58

bnmajor added 5 commits September 20, 2023 11:49

Allow users to manipulate all existing templates

0a493e0

Signed-off-by: Brianna Major <[email protected]>

Simplify redundant list of templates

dc76257

Signed-off-by: Brianna Major <[email protected]>

Color active bounds

1cb06c2

Signed-off-by: Brianna Major <[email protected]>

Reset current template on undo

ec58684

Signed-off-by: Brianna Major <[email protected]>

Prevent drawing when interacting

af3796a

Signed-off-by: Brianna Major <[email protected]>

bnmajor force-pushed the masking-boundaries branch from 3176da4 to af3796a Compare September 20, 2023 15:49

Use shape.get_verts() to get vertices

c8cd796

This makes the ellipses in the polar view appear much smoother. Signed-off-by: Patrick Avery <[email protected]>

psavery and others added 3 commits September 20, 2023 12:30

Correct polar view rotation by aspect ratio

9cc66f6

This aspect ratio takes into account both the extent and the canvas aspect ratio. Signed-off-by: Patrick Avery <[email protected]>

Fix plot title check

d2446e0

We only need to confirm that we are in the correct detector in the raw image mode. Signed-off-by: Brianna Major <[email protected]>

Fix extent aspect ratio correction

43f46ca

I accidentally inverted this... Signed-off-by: Patrick Avery <[email protected]>

psavery approved these changes Sep 20, 2023

View reviewed changes

bnmajor marked this pull request as ready for review September 20, 2023 18:20

Do not interact if in static mode

3c5bf48

Signed-off-by: Brianna Major <[email protected]>

psavery approved these changes Sep 20, 2023

View reviewed changes

Allow the angle of rotation by arrow key to be changed

85194d0

Signed-off-by: Brianna Major <[email protected]>

bnmajor force-pushed the masking-boundaries branch from 0f010d3 to 85194d0 Compare September 20, 2023 20:19

PEP 8 fixes

9c2b7ee

Signed-off-by: Brianna Major <[email protected]>

bnmajor merged commit ba1880b into HEXRD:master Sep 20, 2023
9 checks passed

bnmajor deleted the masking-boundaries branch September 20, 2023 21:44

psavery mentioned this pull request Sep 21, 2023

Add ability to rotate rectangle or ellipse masking #1416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Masking boundaries #1569

Masking boundaries #1569

bnmajor commented Sep 6, 2023 •

edited

Loading

bnmajor commented Sep 6, 2023

psavery left a comment

psavery commented Sep 6, 2023

bnmajor commented Sep 8, 2023

bnmajor commented Sep 8, 2023

pep8speaks commented Sep 20, 2023 •

edited

Loading

psavery left a comment

saransh13 commented Sep 20, 2023 via email

bnmajor commented Sep 20, 2023 •

edited

Loading

saransh13 commented Sep 20, 2023 via email

bnmajor commented Sep 20, 2023 •

edited

Loading

psavery left a comment

Masking boundaries #1569

Masking boundaries #1569

Conversation

bnmajor commented Sep 6, 2023 • edited Loading

bnmajor commented Sep 6, 2023

psavery left a comment

Choose a reason for hiding this comment

psavery commented Sep 6, 2023

bnmajor commented Sep 8, 2023

bnmajor commented Sep 8, 2023

pep8speaks commented Sep 20, 2023 • edited Loading

Comment last updated at 2023-09-20 20:23:49 UTC

psavery left a comment

Choose a reason for hiding this comment

saransh13 commented Sep 20, 2023 via email

bnmajor commented Sep 20, 2023 • edited Loading

saransh13 commented Sep 20, 2023 via email

bnmajor commented Sep 20, 2023 • edited Loading

psavery left a comment

Choose a reason for hiding this comment

bnmajor commented Sep 6, 2023 •

edited

Loading

pep8speaks commented Sep 20, 2023 •

edited

Loading

bnmajor commented Sep 20, 2023 •

edited

Loading

bnmajor commented Sep 20, 2023 •

edited

Loading