Skip to content

Conversation

@bnmajor
Copy link
Collaborator

@bnmajor bnmajor commented May 30, 2025

  • Remove the "remove mask" column in favor of the "remove selected" option
  • Remove the presentation combo boxes for each mask. Instead, list the current presentation status as text as use global presentation selection drop-down for selected masks
  • Remove the "apply changes" button since all updates are now immediate
  • Highlight masks as they are selected
  • Add options to set highlight color and opacity in the style dialog

Relies on #1849

@bnmajor bnmajor force-pushed the mask-manager-dialog branch from 5bb5eaf to d7e1d4a Compare May 30, 2025 20:17
@saransh13
Copy link
Member

Two issue I found:

  1. cannot change mask appearance for single selection.
  2. The presentation field is editable.

The polygon filling of the mask feels a little laggy currently. It'd be a great improvement if we can improve that aspect as well.

@bnmajor bnmajor force-pushed the mask-manager-dialog branch from d7e1d4a to 7e52028 Compare June 5, 2025 12:35
@bnmajor bnmajor marked this pull request as ready for review June 25, 2025 16:50
@bnmajor bnmajor force-pushed the mask-manager-dialog branch from d989601 to 6f5a9e3 Compare June 25, 2025 16:56
@bnmajor
Copy link
Collaborator Author

bnmajor commented Jul 2, 2025

Remaining issues:

  • highlighting remains if dialog is closed

@bnmajor bnmajor force-pushed the mask-manager-dialog branch from 6f5a9e3 to a2d497f Compare July 29, 2025 22:07
@bnmajor bnmajor force-pushed the mask-manager-dialog branch from a2d497f to bef41f9 Compare August 13, 2025 19:23
@bnmajor bnmajor requested a review from psavery August 13, 2025 19:23
Copy link
Member

@saransh13 saransh13 left a comment

Choose a reason for hiding this comment

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

LGTM. Will wait for Patrick to have the final approval before merge.

@psavery
Copy link
Collaborator

psavery commented Aug 14, 2025

Looks like there's a merge conflict. @bnmajor can you resolve that?

I'm going to take a look over the code now...

@bnmajor bnmajor force-pushed the mask-manager-dialog branch from bef41f9 to 4776e50 Compare August 14, 2025 14:40
continue

if not mask.visible and not mask.show_border:
if not mask.visible and not mask.show_border and not mask.highlight:
Copy link
Collaborator

@psavery psavery Aug 14, 2025

Choose a reason for hiding this comment

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

This means that highlighted masks will affect the azimuthal lineout, even if the masks are not "visible" or the border is not shown. I think that's probably not what we want. We probably want to keep it the way it was before (only affect the azimuthal lineout if the mask is visible or the border is shown).

Copy link
Collaborator

Choose a reason for hiding this comment

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

So I'd recommend just removing this change.

mode=None,
xray_source=None
xray_source=None,
highlight=False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
highlight=False
highlight=False,

I'd recommend including a , at the end of all final arguments. The reason is that, without it, a new change is necessary in the changelog to add a comma to the previous one (as is the case here for xray_source=None). This can cause confusion as to who authored that line and why it was authored.

self._mask_boundary_artists.clear()

def draw_mask_boundaries(self, axis, det=None):
def update_mask_highlights(self, axis):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need a new signal/slot mechanism that, when the user's selection changes, it only triggers highlight artist updates (and doesn't retrigger all the polar mask logic).

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would make highlight selections much faster.

@bnmajor bnmajor force-pushed the mask-manager-dialog branch from 2573d14 to 5a14e78 Compare August 27, 2025 19:33
@psavery psavery force-pushed the mask-manager-dialog branch from 5a14e78 to 2573d14 Compare August 27, 2025 20:23

if self.mode in (ViewType.polar, ViewType.stereo):
self.update_mask_highlights(self.axis)
self.draw_idle()
Copy link
Collaborator

@psavery psavery Aug 28, 2025

Choose a reason for hiding this comment

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

So, the purpose of using the blit manager (and the animation features) is to avoid calling self.draw() and self.draw_idle(), because those can be expensive calls when you need faster speeds for things like animations. Instead, self.blit_manager.update() is all you need.

And, it looks like the highlight_masks() function already calls self.blit_manager.update(), so you can safely remove both calls to self.draw_idle() above.

After you remove these, you might also no longer need the debouncer in the mask manager dialog, because the updates should be much faster. But I'll leave it up to you whether to keep the debouncer or not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, the calls to self.draw_idle() are most time-consuming when the polar view has a very high resolution (small tth and eta pixel size). So you may not notice the performance impact much unless you switch to higher resolutions.

However, our users do typically work with high resolution polar views.

@bnmajor bnmajor force-pushed the mask-manager-dialog branch from caaff32 to d950beb Compare August 30, 2025 17:13
@psavery
Copy link
Collaborator

psavery commented Sep 2, 2025

LGTM

@psavery psavery merged commit f9824a8 into master Sep 2, 2025
6 checks passed
@psavery psavery deleted the mask-manager-dialog branch September 2, 2025 14:02
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.

4 participants