Disable compute-meshfile-button for volume annotations and datasets without segmentation #5648

philippotto · 2021-08-02T09:29:40Z

URL of deployed dev instance (used for testing):

https://___.webknossos.xyz

Steps to test:

open a dataset with the following properties (with enabled jobs):
- no segmentation layer / view mode (should be disabled)
- no segmentation layer / with volume annotation (should be disabled)
- segmentation layer / view mode (should be enabled)
- segmentation layer / with volume annotation (should be disabled)
- one of the above, but without enabled jobs (should be disabled)
ensure that the "compute mesh file" button is enabled/disabled correctly (including plausible tooltip explanation)

Issues:

contributes to Disable job triggers for datasets that are not reachable for wk-worker #5625

Updated (unreleased) changelog
Updated (unreleased) migration guide if applicable
Updated documentation if applicable
Adapted wk-connect if datastore API changes
Needs datastore update after deployment
Ready for review

…ithout segmentation layer

hotzenklotz · 2021-08-02T09:52:35Z

frontend/javascripts/oxalis/view/right-border-tabs/meshes_view.js

+        title = "Computation jobs are not enabled for this webKnossos instance.";
+      } else if (this.props.hasVolume) {
+        title =
+          "Meshes cannot be computed for volume annotations. Please open this dataset in view mode to compute a mesh file";


Why should view mode help here? View mode will not convert the volume annotation into a segmentation, right?

Also, if the toast supports HTML links, this should be a clickable link.

Why should view mode help here? View mode will not convert the volume annotation into a segmentation, right?

I changed the tooltip to Meshes cannot be precomputed for volume annotations. However, you can open this dataset in view mode to precompute meshes for the dataset's segmentation layer.. I hope this makes it clearer?

Also, if the toast supports HTML links, this should be a clickable link.

It's a tooltip, so, no clickable link possible..

This assumes that there is a segmentation layer to begin with in view mode. Depending on your dataset this is not the case. I am not sure if this already considered for this conditional. If so, than I guess its fine.

Yeah, this case is already handled.

hotzenklotz · 2021-08-02T09:58:16Z

frontend/javascripts/oxalis/view/right-border-tabs/meshes_view.js

+      return (
+        <Menu>
+          <Menu.Item onClick={startComputingMeshfile} disabled={disabled}>
+            <Tooltip title={title}>Compute Mesh File</Tooltip>


I am also not a big fan of the button title. What is a mesh file? This is very technical. How about something more descriptive. "Computes Mesh for (whole) dataset" or "Pre-compute meshes for all segments" or "Compute all meshes". Or any such combination.

I find it hard to find the difference between the button "Compute Mesh" and "Compute Mesh File". Likely the other button should be called something like "Compute for Mesh for (active) Segment" or "Compute single mesh" or "compute temporay mesh" ...

@normanrz Any comments?

I don't have a strong opinion re mesh file.
I agree that meshes vs isosurfaces is quite confusing. They are essentially the same but have different names.

I changed the button labels to:

Precompute Meshes (tooltip: Precompute meshes for all segments of this dataset so that meshes for segments can be loaded quickly afterwards from a mesh file.)

Load Precomputed Mesh

Compute Mesh (ad hoc)

I hope this makes it clearer. I also hide the "Compute Mesh (ad hoc)" button automatically, if a mesh file is already available (since there is no benefit in using the ad-hoc part).

I agree that meshes vs isosurfaces is quite confusing. They are essentially the same but have different names.

I went through the code and changed the last user-facing "isosurface" bits to "mesh", but there were quite rare (mainly, in the download/reload buttons of the mesh list). I hope I caught them all.

hotzenklotz · 2021-08-02T09:59:44Z

frontend/javascripts/oxalis/view/right-border-tabs/meshes_view.js

+    };
+
+    const getComputeMeshFileButton = () => {
+      const { disabled, title } = getComputeMeshFileTooltipInfo();


Do theses till tip infos apply to the "Compute Mesh" button as well? I think they do, right? A single mesh also needs a segmentation layer and won't work with volume annotations.

Now, the ad-hoc mesh button also has a tooltip. It needed slightly different code (e.g., it doesn't depend on jobsEnabled) and wording.

@hotzenklotz @philippotto I am a bit confused here, should the ad-hoc isosurface computation really be disabled for volume annotations? There exists the /volume/:tracingId/isosurface request in the tracingstore, which is now inaccessible?

Oh, good catch. I'll open a PR to fix this.

For the record, fixed by @philippotto’s PR #5689 – thanks for the quick reaction! 🚀

hotzenklotz · 2021-08-02T10:01:39Z

On a more high-level note: Do we really want to show these button if the can not do anything, i.e. the "error" toasts? Alternatively, we could not show the buttons at all and have the "error"/help texts directly in the segmentation tab/pane. Not sure how I feel about that though...

…w ad-hoc loading button if mesh file is active

…e to use mesh

philippotto · 2021-08-02T16:04:56Z

On a more high-level note: Do we really want to show these button if the can not do anything, i.e. the "error" toasts? Alternatively, we could not show the buttons at all and have the "error"/help texts directly in the segmentation tab/pane. Not sure how I feel about that though...

This might feel better, but I think, the disabled buttons convey quite well what the user can expect if the conditions are met. If we decide to change this, we should do this in a different PR and focus on the polish. The current PR was only meant as a hotfix for scenarios where meshes are simply not supported.

philippotto · 2021-08-02T16:06:55Z

@daniel-wer feel free to review now :) I'm sorry that the PR got quite big. The last commit (26cedef) is the major contributor to this. In that commit, I extracted some code from the huge render method to make it a bit simpler. You could review the commit individually to get a better diff experience.

…able-compute-mesh-for-volume-annotation

daniel-wer · 2021-08-03T13:38:45Z

@philippotto Could you annotate your testing instructions with when the button should be enabled or disabled? This would help me to review and test this PR :)

philippotto · 2021-08-03T13:40:22Z

Sure, just updated it!

daniel-wer

Works exactly as described and the tooltip explanations are very helpful and descriptive 👍

frontend/javascripts/oxalis/model/sagas/isosurface_saga.js

Co-authored-by: Daniel <[email protected]>

disable compute-meshfile-button for volume annotations and datasets w…

31b62bd

…ithout segmentation layer

philippotto requested a review from daniel-wer August 2, 2021 09:29

philippotto self-assigned this Aug 2, 2021

update changelog

322132e

hotzenklotz reviewed Aug 2, 2021

View reviewed changes

philippotto added 4 commits August 2, 2021 16:46

improve wording for (pre)computed meshes and ad-hoc meshes; don't sho…

ff02e1d

…w ad-hoc loading button if mesh file is active

change userfacing isosurface wording to mesh and also change some cod…

cd8c11d

…e to use mesh

also add explanation tooltip when ad-hoc mesh button is disabled

6a8d991

refactor huge render method into smaller parts

26cedef

philippotto added 2 commits August 2, 2021 18:07

Merge branch 'master' of github.com:scalableminds/webknossos into dis…

029cbd3

…able-compute-mesh-for-volume-annotation

also forbid precomputing meshes if not registered

faec7fe

daniel-wer approved these changes Aug 3, 2021

View reviewed changes

frontend/javascripts/oxalis/model/sagas/isosurface_saga.js Outdated Show resolved Hide resolved

frontend/javascripts/oxalis/model/sagas/isosurface_saga.js Outdated Show resolved Hide resolved

philippotto and others added 2 commits August 3, 2021 16:50

Apply suggestions from code review

9e2cebf

Co-authored-by: Daniel <[email protected]>

Merge branch 'master' into disable-compute-mesh-for-volume-annotation

3409ce0

philippotto enabled auto-merge (squash) August 4, 2021 08:06

philippotto merged commit 3de0a3e into master Aug 4, 2021

fm3 deleted the disable-compute-mesh-for-volume-annotation branch August 25, 2021 09:47

philippotto mentioned this pull request Aug 25, 2021

Re-enable ad-hoc mesh computation for volume annotations #5689

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable compute-meshfile-button for volume annotations and datasets without segmentation #5648

Disable compute-meshfile-button for volume annotations and datasets without segmentation #5648

philippotto commented Aug 2, 2021 •

edited

Loading

hotzenklotz Aug 2, 2021

philippotto Aug 2, 2021

hotzenklotz Aug 2, 2021

philippotto Aug 2, 2021

hotzenklotz Aug 2, 2021

hotzenklotz Aug 2, 2021

normanrz Aug 2, 2021

philippotto Aug 2, 2021

hotzenklotz Aug 2, 2021

philippotto Aug 2, 2021

fm3 Aug 25, 2021

philippotto Aug 25, 2021

fm3 Aug 26, 2021

hotzenklotz commented Aug 2, 2021

philippotto commented Aug 2, 2021

philippotto commented Aug 2, 2021

daniel-wer commented Aug 3, 2021

philippotto commented Aug 3, 2021

daniel-wer left a comment

Disable compute-meshfile-button for volume annotations and datasets without segmentation #5648

Disable compute-meshfile-button for volume annotations and datasets without segmentation #5648

Conversation

philippotto commented Aug 2, 2021 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hotzenklotz commented Aug 2, 2021

philippotto commented Aug 2, 2021

philippotto commented Aug 2, 2021

daniel-wer commented Aug 3, 2021

philippotto commented Aug 3, 2021

daniel-wer left a comment

Choose a reason for hiding this comment

philippotto commented Aug 2, 2021 •

edited

Loading