Segment group batch actions #7164

dieknolle3333 · 2023-06-20T19:20:54Z

Steps to test:

Go to the segments tab in any annotation
Make sure you see some segments in the list, e.g. by clicking segments
Either create a new group and drop segments or test the following on the root group:
-- Right-click a group and set the color
-- Revert setting the color
-- Load meshes either ad-hoc or precomputed with a meshfile. If there is no meshfile, there should only be the option to load the meshes ad-hoc
-- Hide/show meshes. The text of the option should be adjusted to the current visibility
-- Download all meshes of the group. You should receive a zip with the according stl files.

Issues:

fixes Segments Groups #6954
Related to Show all meshes for dataset (or for bounding box) #5727

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

Updated changelog
Removed dev-only changes like prints and application.conf edits
Considered common edge cases

frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx

…nt group

philippotto

very cool stuff 👍 I left a couple of notes but overall it looks pretty good. other than the refactoring of the per-segment-call code about which we already talked, there is one thing I'm a bit hesitant about: areSegmentsInGroupVisible. this is state you are maintaining in your component, but it can be affected from the outside. for example, if I rightclick into the data viewport and render a mesh, the segment list won't take note of that right now. similarily, if I use the checkbox next to a mesh to hide it, the list component will also run into an outdated state.

instead, you should probably derive this information from the IsosurfaceInformation objects in this.props.isosurfaces[segment.id] (it has a isVisible prop). you might want to use getDerivedStateFromProps for that so that you can build your areSegmentsInGroupVisible state everytime the isosurfaces change.

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

frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx

dieknolle3333 · 2023-06-30T13:58:40Z

@philippotto thank you for the review!

I'll just quickly respond to the topic on areSegmentsInGroupVisible:
In my view it was okay if the visibility of some meshes changed without the field noticing, because it was meant as a map for each group, to remember whether the last interaction made all meshes in a group visible or invisible. But you are totally right of course, that the state of the meshes matters if it was manipulated from another place. One example would be groups with only one mesh.

I thought about extracting the visibility from the meshes but I wasn't sure what a good metric in a mixed-visibility group is. If I use the actual isosurface data, is it fine to derive a field called areAnySegmentsInGroupVisible? So if any mesh in the group is visible, clicking the action hides all meshes. Or is it better the other way around? (If any mesh in the group is invisible, clicking the action makes everything visible?) I think intuitively I would prefer the second option.

philippotto · 2023-06-30T14:24:33Z

I thought about extracting the visibility from the meshes but I wasn't sure what a good metric in a mixed-visibility group is. If I use the actual isosurface data, is it fine to derive a field called areAnySegmentsInGroupVisible? So if any mesh in the group is visible, clicking the action hides all meshes. Or is it better the other way around? (If any mesh in the group is invisible, clicking the action makes everything visible?) I think intuitively I would prefer the second option.

Yes, i think it's fine to deal with mixed visibilities like you suggested. I don't have a strong opinion whether hide or show are "preferred". If you find the one variant more intuitive, let's go with that :)

hotzenklotz

Great stuff. Please see my comments on naming variables, functions etc and consistency. Otherwise this is works very well.

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

frontend/javascripts/oxalis/model/actions/annotation_actions.ts

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

hotzenklotz · 2023-07-11T08:57:51Z

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

+  const { segmentMeshController } = getSceneController();
+  const zipWriter = new Zip.ZipWriter(new Zip.BlobWriter("application/zip"));
+  try {
+    const promises = cells.map((element) => {


Can we call this something more specific / less generic? What do these promise hold? Maybe zipWriterPromises? Or zippedFilesPromises`? Idk

there is a promise for each file that's added to the zipWriter. (for reference: https://gildas-lormeau.github.io/zip.js/api/index.html) so I think addFileToZipWriterPromises is long but correct.

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

frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx

hotzenklotz · 2023-07-11T11:28:47Z

frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx

        groupTree: generatedGroupTree,
        searchableTreeItemList,
        prevProps: nextProps,
      };
-    } else {
+    }
+    if (prevState.prevProps?.isosurfaces !== isosurfaces) {


I am not sure I understand the reason for this code. Can you elaborate please. How can I test this?

This code makes sure that changing the visibility of a mesh (e.g. by clicking the checkbox) gets noticed by the segments_view as the visibility is part of the isosurface.
This is needed because I toggle the "Hide/Show meshes" option based on the mesh visibility in the group.
You can test it by loading all meshes in a group. Now, the context menu should say "Hide...". If you deselect any mesh's visibility checkbox, the context menu notices and will then read "Show all meshes".
Does that make sense?

also see philipps comment for this: #7164 (review)

…s/webknossos into segment-group-batch-actions

philippotto

great stuff :) I left some suggestions for simplifying some things further 🤞

philippotto · 2023-07-17T11:56:28Z

frontend/javascripts/libs/zipjs_wrapper.ts

@@ -0,0 +1,14 @@
+class TransFormStream {}
+
+let mockedWindow = false;


whats the purpose of this variable? if it's needed for some reason, you can add a comment to it.

Omitting it doesn't seem to break anything, so I left it out.
Just for clarification. Technically this could have been used somewhere else because the scope of the variable is global, right? (https://www.w3schools.com/js/js_scope.asp)

Technically this could have been used somewhere else because the scope of the variable is global, right? (https://www.w3schools.com/js/js_scope.asp)

The linked article is a bit confusing (generally, w3schools ist not the best resource; I recommend MDN instead). However, the specific MDN article is also not perfect (it mentions module scope, but doesn't really explain when it comes into play).
To answer your question: no, it could not have been used somewhere else because the variable is module-scoped. That scoping is achieved during the compilation/bundling of all the module files. To get a truly global variable, one has to extend window (in a browser context) or global (in a node context).

philippotto · 2023-07-17T14:30:21Z

frontend/javascripts/admin/dataset/dataset_upload_view.tsx

          const entries = await reader.getEntries();
          await reader.close();
-          const wkwFile = entries.find((entry: Entry) =>
+          const wkwFile = entries.find((entry: any) =>


I just pushed a commit (I hope the CI passes), which improves the typing a bit.

frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx

…s/webknossos into segment-group-batch-actions

philippotto · 2023-07-18T07:29:40Z

frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx

-    };
-    this.setState({
-      areSegmentsInGroupVisible: newSegmentsInGroupVisible,
-    });
  };

  getSegmentsWithMissingLocation = (groupId: number): number[] => {


I'd replace Location with Position here to keep it consistent with the somePosition attribute.

philippotto

Great :) Code looks good to me and it works great, too 👍

dieknolle3333 added 2 commits June 20, 2023 18:50

start: change group color

4e86f6d

change group color

b97c9d8

dieknolle3333 marked this pull request as draft June 20, 2023 19:21

dieknolle3333 commented Jun 20, 2023

View reviewed changes

frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx Outdated Show resolved Hide resolved

dieknolle3333 added 10 commits June 20, 2023 21:59

[very broken] first implementation to hide/load all meshes of a segme…

ea18ffd

…nt group

show/hide all msehes for group

98e458f

add batch actions: download, show/hide, load meshes for segment group

071d0e1

fix code duplication and lint

d2d8861

add warning if position of segments in unknown

e6ed640

push WIP before I leave

5472e53

zip multiple meshes

0a4c7d2

undo zip.js upgrade

31c3f87

batch actions to set colour and remove and refresh meshes option

e829f1e

Merge branch 'master' into segment-group-batch-actions

0f0f402

dieknolle3333 self-assigned this Jun 27, 2023

dieknolle3333 added 3 commits June 27, 2023 18:28

lint

c879a10

remove TODOs

e0440d3

add chagelog entry

8cab227

dieknolle3333 marked this pull request as ready for review June 27, 2023 16:34

dieknolle3333 requested a review from philippotto June 27, 2023 16:34

dieknolle3333 marked this pull request as draft June 27, 2023 16:37

refactor code

ea8b1c9

philippotto requested changes Jun 30, 2023

View reviewed changes

dieknolle3333 added 5 commits June 30, 2023 19:58

WIP: start refactoring

5944d85

WIP: add helper method

8781956

WIP: address review

e809bca

WIP: add lost method call

ca20df1

WIP: trying to improve areSegmentsInGroupVisible

a4b89e2

dieknolle3333 requested a review from hotzenklotz July 10, 2023 16:47

dieknolle3333 marked this pull request as ready for review July 10, 2023 16:47

hotzenklotz approved these changes Jul 11, 2023

View reviewed changes

dieknolle3333 added 5 commits July 12, 2023 00:26

WIP: address PR review

f3156ea

WIP: address refactoring

1198f1d

WIP: change icons

9c1c06f

address review and lint

cdf453a

merge master

2803c61

dieknolle3333 requested a review from philippotto July 14, 2023 11:50

dieknolle3333 and others added 7 commits July 14, 2023 14:45

fix capitalization

76a09af

fix spacing

720e052

fx method name

bb1e286

fix icon spacing

3baa2aa

Merge branch 'master' into segment-group-batch-actions

caa361a

improve typing

a78963f

Merge branch 'segment-group-batch-actions' of github.com:scalablemind…

bebb08f

…s/webknossos into segment-group-batch-actions

philippotto requested changes Jul 17, 2023

View reviewed changes

dieknolle3333 added 3 commits July 17, 2023 17:18

WIP: fix spacing of font awesome icons and fix linting

a4dd2cd

Merge branch 'segment-group-batch-actions' of github.com:scalablemind…

e3892db

…s/webknossos into segment-group-batch-actions

address review

4d35fca

philippotto reviewed Jul 18, 2023

View reviewed changes

dieknolle3333 added 3 commits July 18, 2023 10:07

remove TODO comment and improve naming

eef4546

Merge branch 'master' into segment-group-batch-actions

414455e

merge master

41034cc

dieknolle3333 requested a review from philippotto July 18, 2023 15:02

philippotto approved these changes Jul 19, 2023

View reviewed changes

hotzenklotz mentioned this pull request Jun 27, 2023

Segments Groups #6954

Closed

8 tasks

merge master

eddc45e

dieknolle3333 merged commit 9cecaab into master Jul 20, 2023
2 checks passed

dieknolle3333 deleted the segment-group-batch-actions branch July 20, 2023 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Segment group batch actions #7164

Segment group batch actions #7164

dieknolle3333 commented Jun 20, 2023 •

edited

philippotto left a comment

dieknolle3333 commented Jun 30, 2023

philippotto commented Jun 30, 2023

hotzenklotz left a comment

hotzenklotz Jul 11, 2023

dieknolle3333 Jul 11, 2023

hotzenklotz Jul 11, 2023

dieknolle3333 Jul 11, 2023

dieknolle3333 Jul 11, 2023

philippotto left a comment

philippotto Jul 17, 2023

dieknolle3333 Jul 18, 2023

philippotto Jul 18, 2023

philippotto Jul 17, 2023

philippotto Jul 18, 2023

philippotto left a comment

		@@ -0,0 +1,14 @@
		class TransFormStream {}

		let mockedWindow = false;

Segment group batch actions #7164

Segment group batch actions #7164

Conversation

dieknolle3333 commented Jun 20, 2023 • edited

Steps to test:

Issues:

philippotto left a comment

Choose a reason for hiding this comment

dieknolle3333 commented Jun 30, 2023

philippotto commented Jun 30, 2023

hotzenklotz left a 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

philippotto left a 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

philippotto left a comment

Choose a reason for hiding this comment

dieknolle3333 commented Jun 20, 2023 •

edited