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

feat: Absolute stage position controls #383

Merged
merged 7 commits into from
Nov 22, 2024

Conversation

gselzer
Copy link
Contributor

@gselzer gselzer commented Nov 7, 2024

This PR is designed to resolve #332.

image

@gselzer gselzer added the enhancement New feature or request label Nov 7, 2024
@gselzer gselzer self-assigned this Nov 7, 2024
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

Attention: Patch coverage is 94.04762% with 5 lines in your changes missing coverage. Please review.

Project coverage is 90.87%. Comparing base (7277aa6) to head (a5707e9).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/pymmcore_widgets/control/_stage_widget.py 94.04% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #383      +/-   ##
==========================================
+ Coverage   90.70%   90.87%   +0.16%     
==========================================
  Files          85       85              
  Lines        9199     9291      +92     
==========================================
+ Hits         8344     8443      +99     
+ Misses        855      848       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@gselzer
Copy link
Contributor Author

gselzer commented Nov 7, 2024

I am especially interested in the reviewers' opinions on a036481, which fixes/improves the testing of setting absolute position. Allowing error does not make sense to me, but it is also necessary for the tests to pass (although the positional error is much smaller than the bounds I wrote)...

@tlambert03
Copy link
Member

Thanks @gselzer, the code looks good to me. My main concern, and the reason I backed down from adding this in #334 is that this introduces a very real danger to damage hardware. programmatic control of motors, particularly when the objective is focused on a sample, is one of the main causes of expensive damage. I did like @marktsuchida's suggestion in #334 (comment) that this functionality might be hidden behind an additional button and dialog that would prevent anyone doing this by accident. I also think it would be great if that dialog also had a big "HALT" button that would stop and existing stage movement. (the diSPIM plugin has one of those in micro-manager and i love it and use it all the time)

@gselzer
Copy link
Contributor Author

gselzer commented Nov 8, 2024

My main concern, and the reason I backed down from adding this in #334 is that this introduces a very real danger to damage hardware. programmatic control of motors, particularly when the objective is focused on a sample, is one of the main causes of expensive damage.

This is a great point - happy to refactor this PR so that we can resolve your concerns.

I did like @marktsuchida's suggestion in #334 (comment) that this functionality might be hidden behind an additional button and dialog that would prevent anyone doing this by accident.

This is one option. A second, simpler option is just an opt-in toggle to enable these spinboxes - otherwise they could just perform like the label that you had before. I actually thought about adding that as a part of this PR, but opted for simplicity. Happy to delegate to your and @marktsuchida's intuition here.

I also think it would be great if that dialog also had a big "HALT" button that would stop and existing stage movement.

There's already code for that although we could maybe create a smaller button with an icon instead of text, and put it alongside these spinboxes.

@tlambert03
Copy link
Member

A second, simpler option is just an opt-in toggle to enable these spinboxes - otherwise they could just perform like the label that you had before.

I'm good with that option 👍... I wonder about spacing though, the X/Y case is already pretty wide horizontally, and has 4 checkboxes below.

Screenshot 2024-11-08 at 3 39 23 PM

just an annoying aesthetic issue :) if you can come up with a nice look for it, let's go with that option

@tlambert03
Copy link
Member

hey @gselzer, just checking in here. let me know if you want any more ideas or just need time to implement. I'd like to see a screenshot of how the buttons look when added

@gselzer
Copy link
Contributor Author

gselzer commented Nov 18, 2024

hey @gselzer, just checking in here. let me know if you want any more ideas or just need time to implement. I'd like to see a screenshot of how the buttons look when added

Oh! Sorry, lost track of this - I'll take a look tomorrow once I have some time

Additional safety features are always welcome!
@gselzer
Copy link
Contributor Author

gselzer commented Nov 19, 2024

@tlambert03 with 5034319 the UI looks like this:

image

I am very open to future feedback

@tlambert03
Copy link
Member

I was expecting a button somewhere that toggles whether the XY positions are editable in the first place (with the default being not editable, the user should have to do some explicit action to enter a mode where the stage will move in response to number entry).

@gselzer
Copy link
Contributor Author

gselzer commented Nov 19, 2024

I was expecting a button somewhere that toggles whether the XY positions are editable in the first place (with the default being not editable, the user should have to do some explicit action to enter a mode where the stage will move in response to number entry).

That's fair, however I can think of a couple rebuttals:

  1. We'd then have to add another control - I feel a bit bad about all of the clutter I've already added 😅
  2. This "explicit action" could be enabling the buttons in a GUI configuration, - that setting could then be passed to this widget. Maybe I should write a method on the StageWidget that allows toggling absolute positioning after construction, though...

If you would still like some button though, I can certainly add one

@marktsuchida
Copy link

  • We'd then have to add another control - I feel a bit bad about all of the clutter I've already added 😅

That was one of the motivations for proposing that an additional dialog open. Alternatively, the text can be uneditable by default, but a small popup could open on right-clicking and selecting "Edit...". Perhaps.

@fdrgsp
Copy link
Collaborator

fdrgsp commented Nov 19, 2024

the text can be uneditable by default, but a small popup could open on right-clicking and selecting "Edit...

I like this idea!

@marktsuchida
Copy link

Of course this small popup would need to stay open (until something else is clicked?) if we want to allow people to edit it multiple times without re-clicking. Also, perhaps something like "Enter Coordinates..." would be a better name than "Edit...".

@fdrgsp
Copy link
Collaborator

fdrgsp commented Nov 19, 2024

what about something like this?

Screen.Recording.2024-11-19.at.3.32.39.PM.mov

@gselzer
Copy link
Contributor Author

gselzer commented Nov 19, 2024

@fdrgsp I do like that implementation - are you able to push that to this branch?
Couple points:

  • Should we tether the two spin boxes together on a 2-axis stage, such that both become enabled/disabled together?
  • I like @marktsuchida's suggestion of Edit... for the menu text, but then it should probably change to something else when enabled. Maybe Enable Editing/Disable Editing?
  • Bikesheddy, but thinking that maybe I'd like the STOP! button below the checkboxes - thoughts?

Happy to make any/all of these changes once your changes are pushed.

@gselzer
Copy link
Contributor Author

gselzer commented Nov 20, 2024

@fdrgsp thank you so much for contributing, in cf9942a I cleaned up your changes a bit, and added some tests. Notably, I:

  • Tethered the spin boxes so enabling one enables both.
  • Made the menu action toggle, so no need to change text.
  • More minor changes.

Lemme know if you see any issues. Looks like a test is also failing in the MDA widget - I'll take a look tomorroww

@gselzer gselzer force-pushed the absolute-stage-pos branch 2 times, most recently from d1917b1 to 0d51d6d Compare November 20, 2024 15:58
@fdrgsp
Copy link
Collaborator

fdrgsp commented Nov 20, 2024

@gselzer I like this solution but after having a quick chat with @tlambert03 I do realize that it might be a little hard for a user to know that there is the option to enable manual editing of the coordinates.

What about having a menu on top of the widget? It might be easier to "discover"...

Screen.Recording.2024-11-20.at.3.49.04.PM.mov

@gselzer
Copy link
Contributor Author

gselzer commented Nov 20, 2024

@gselzer I like this solution but after having a quick chat with @tlambert03 I do realize that it might be a little hard for a user to know that there is the option to enable manual editing of the coordinates.

You know, this reminds me of the discussion that we had in #336 - specifically "once you notice they're editable, you know it forever."

I have a very weak preference for the current behavior as:

  • It requires one fewer control.
  • The menu is physically closer to the widgets it affects (but this is likely easily resolved).
  • This behavior maybe shouldn't be discovered and used unless you're a poweruser who really knows what they're doing.

All of these reasons are pretty minor, so I'm content with being overruled here if you guys like it better this way.

@tlambert03
Copy link
Member

tlambert03 commented Nov 20, 2024

You know, this reminds me of the discussion that we had in #336 - specifically "once you notice they're editable, you know it forever."

😂 so funny... as I was having that conversation with @fdrgsp, I was thinking to myself "boy I sound like a hypocrite". So yeah, if @gselzer is happy with the right-click and/or tooltip to enable editability, I think it's great 👍 and I'm more than happy to go with it

I'm particularly receptive to "This behavior maybe shouldn't be discovered and used unless you're a poweruser who really knows what they're doing."

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

good stuff! Thanks @gselzer!

@tlambert03 tlambert03 merged commit 7fccf95 into pymmcore-plus:main Nov 22, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stage Control - consider absolute position field
4 participants