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

Rework volume interpolation feature to be shortcut-bound and support depths > 2 #6235

Merged
merged 22 commits into from
May 25, 2022

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented May 24, 2022

From changelog:

  • Added support for segment interpolation with depths > 2. Also, the feature was changed to work on an explicit trigger (either via the button in the toolbar or via the shortcut V). When triggering the interpolation, the current segment id is interpolated between the current slice and the least-recently annotated slice.
  • Removed the feature to copy a segment from the previous/next slice with the V shortcut. Use the new volume interpolation feature instead (also bound to V and available via the toolbar).

URL of deployed dev instance (used for testing):

Steps to test:

  • create a new annotation
  • label a cell in slice X
  • move forward to slice X + 3
  • press "V" shortcut --> X + 1 and X + 2 should be filled automatically
  • repeat the same with varying constellations (e.g., z distance == 1 which should disable the button and show a toast when pressing V; distance > 8; interpolating for a slice where the segment does not exist)

Issues:


(Please delete unneeded items, merge only when none are left open)

@philippotto philippotto changed the title [WIP] Rework volume interpolation feature to be shortcut-bound and support depths > 2 Rework volume interpolation feature to be shortcut-bound and support depths > 2 May 24, 2022
@philippotto philippotto self-assigned this May 24, 2022
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Awesome PR 🎉 . Thanks for making the interpolation available for higher distances 📐 .

Codewise I only found some minor things.

Bugs I noticed. I tested on the dev instance in this tracing:

  • In resolution 8-8-2 I drew on one slice in the xy viewport, and moved two slices forward by pressing space two times. Then I annotated a little again. Now the interpolation button is disabled (which imo shouldn't be the case) and when I press v I get the Could not interpolate segment because last labeled slice should be at least 2 slices away error message. In the described scenario the two annotated slices should be far enough away from each other for the interpolation to work, right? So I think this is a bug. While reading the code I could not spot why this might be the case.

Usability nice to haves:

  • When the interpolation button is disabled, the button is only disabled. There is no tooltip telling why the button is disabled. I would like to have an explanatory tooltip.
  • I don't know whether it is by design, but the volume interpolation button is wider than the other tool buttons. Maybe it would look nicer if it would have the same length as the other tool buttons.
    image
  • There is no visual feedback on whether the interpolation is still being computed or already finished. Something like a little yellow dot for "I am still computing" and green for "I finished interpolating" would be nice imo.

philippotto and others added 4 commits May 25, 2022 13:23
@philippotto
Copy link
Member Author

@MichaelBuessemeyer Thanks for your feedback! I think, I fixed all bugs (the wide button and missing toolbar was also a bug, caused by switching from Button to ButtonComponent). Please have another look :)

There is no visual feedback on whether the interpolation is still being computed or already finished. Something like a little yellow dot for "I am still computing" and green for "I finished interpolating" would be nice imo.

The mouse cursor should turn into a waiting cursor when wk is busy. I think, this generalizes better to a yellow dot or similar. However, I don't see that cursor (maybe the workload is not large enough?). takeEveryUnlessBusy should take care of this actually... If I got another second, I'll try to investigate, but I'd hope that this doesn't block the PR.

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Thanks for fixing my address issues. I couldn't reproduce them anymore. 🚀

The mouse cursor should turn into a waiting cursor when wk is busy. I think, this generalizes better to a yellow dot or similar.

Ah, using the cursor as an indication sounds good.

However, I don't see that cursor (maybe the workload is not large enough?). takeEveryUnlessBusy should take care of this actually... If I got another second, I'll try to investigate,

Same here, for my setup (MacOS + Chrome). The cursor does not turn into a waiting-like symbol :/. It might actually be that the computation is too fast 🚀 .

but I'd hope that this doesn't block the PR.

Agreed, this is be something for a follow-up pr.

@philippotto philippotto enabled auto-merge (squash) May 25, 2022 21:38
@philippotto philippotto merged commit 6a625f2 into master May 25, 2022
@philippotto philippotto deleted the deep-interpolation branch May 25, 2022 21:39
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.

Follow-up: Enable volume Interpolation with depth > 2 and clarify usability
2 participants