Improve segment proofreading in 3D viewport

[philippotto]

• allows to activate segment via mesh in 3D viewport
• highlights supervoxels on hover
• allows merging/splitting in 3d viewport by interacting with the agglomerates / supervoxels

URL of deployed dev instance (used for testing):

• https://___.webknossos.xyz

Steps to test:

• create new annotation
• select a hdf5 mapping
• select proofreading tool and load some meshes
• try to split and merge them in the 3D viewport

TODOs:

• [x] backend:
  • [x] as an alternative to positions, also accept unmappedSegmentIds for split/merge related operations
  • [x] implement API that maps from segmentId to a segmentPosition
• [x] frontend
  • [x] integrate the above ino the context menu in the 3D viewport
  • [x] don't show the proofreading cross in the data viewports if the user activated a supervoxel in the 3D viewport
  • [x] bugs
    • [x] loading the same mesh twice breaks the hovering effect for unknown reasons
    • [X] after proofread operation, selected super voxel is not highlighted anymore (but stored as active)
    • [x] when creating a new annotation, a new active segment id is chosen. when switching to proofreading, one can try to merge with that segment id. however, it doesn't exist anywhere.
    • ~[ ] selecting one partner in the 3D viewport and the other in a data viewport (or vice versa) doesn't work~ the user is told so now. hopefully can be improved with #6569

Issues:

• contributes to #7619

---

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

• [x] Updated changelog
• [ ] Updated migration guide if applicable
• [ ] Updated documentation if applicable
• [ ] Adapted wk-libs python client if relevant API parts change
• [ ] Removed dev-only changes like prints and application.conf edits
• [ ] Considered common edge cases
• [ ] Needs datastore update after deployment

[philippotto]

@fm3 When you have some time, it would be great if you could make the splitAgglomerate and mergeAgglomerate compatible with passing the unmapped segment ids. The same basically for the /agglomerateGraphMinCut and /getNeighborsForAgglomerateNode route :pray:

[fm3]

@philippotto I’ve done that now, please give it a try! The new params are called segmentId1, segmentId2, segmentId

I assume that the frontend now always has the segment ids, right? Applying the update actions is backwards compatible, but do I need to build the same for the mincut/neighbors routes?

[philippotto]

@philippotto I’ve done that now, please give it a try! The new params are called segmentId1, segmentId2, segmentId

Awesome, thanks, I'll give it a try.

I assume that the frontend now always has the segment ids, right?

We don't have the "magic mapping", yet, but for the typical split/merge proofreading operations we already have the (unmapped) segment ids, yes.

Applying the update actions is backwards compatible, but do I need to build the same for the mincut/neighbors routes?

For the mincut/neighbors route in non-3d-viewport, we don't have the unmapped id yet, but we can do another request to get these (like we do it for split/merge). These round trips will then be eliminated with the magic mapping soon.

[fm3]

For the mincut/neighbors route in non-3d-viewport, we don't have the unmapped id yet, but we can do another request to get these (like we do it for split/merge). These round trips will then be eliminated with the magic mapping soon.

Right, I wasn’t sure about that. If it’s complicated to do these roundtrips, I can also make the routes accept both variants

[fm3]

In the TODOs for this PR the following is listed:

implement API that maps from segmentId to a segmentPosition

In my understanding wk already knows where a segment is via the segment stats, correct? If that's the case, doesnt the frontend already know this when loading a segment via the segment stats? Or is this part of the segments stats not sent to the frontend and thus the backend needs to explicitly serve this information as it is needed?

If we could rely on segment stats being available always, that would be true. However, in case of on-disk segmentation layers, and annotations based on them (as proofreading annotations always are), those are only available if there is a segment index file stored on disk with the segmentation layer, which is still rare and will probably always be optional. So instead, this code relies on the information supplied by the agglomerate file, which stores a single position per segment. If the agglomerate file is also missing, proofreading is not possible.

[MichaelBuessemeyer]

Here is my testing feedback / observations:

• [X] Imo it would be nice to select a supervoxel via left click, or shift + left click or ctrl + left click and not only via the context menu :). <- Just my first UX impression. I accidentally deleted a mesh multiple times while looking for a shortcut :sweat_smile: (via ctrl + left click)

• [x] I am able to reach an inconsistent state with the super voxel highlighting via the following steps (nothing serious):

1. Select a super voxel in the 3d viewport via the context menu.
2. Split it from all neighbours
3. The supervoxel is still highlighted as active, although when opening the context menu, it allows me to select the supervoxel again

• [X] When I select a supervoxel and then open the context menu, I can select the option "Split from active segment" which results in an error (pretty obvious). Is it possible to disable this menu item in this case?

• [X] When I select a supervoxel in the 2d viewport and then go to a data viewport and select to merge the highlighted segment, the merging is done. Is that correct / expected? Afaik you told me that when selecting in one of the two (data viewport / 3d viewport) and then merging in the other one, should result in missing information and thus an error.

What about the bug that occurred during showcasing yesterday? Did you already find and fix it or is this still an open TODO? :)

At least, I cannot reproduce this anymore :-1:

[philippotto]

Furthermore, throughout the codebase we have the name agglomerateId and mappedSegmentId for the exact same data / object / think. The same goes for segmentId and umappedSegmentId (if I understood the code correctly). Imo it would be great to have a single consistent name here. But I can also understand that in some parts of the code one name makes more sense than the other. Do you think it would still be possible to have consistent naming while keeping the code within functions in different "logical contexts" understandable?

Unfortunately, I'm not sure how to do this well. As an example, the store has a property for the active cell id (should be renamed to active segment id but this is another issue ^^). If there is a mapping, this refers to the mapped id, which could also be called agglomerate id. However, if there is no mapping, "agglomerateId" is highly misleading..

[philippotto]

I think it was in the mesh_saga.ts where Chunck refers to a mesh of an unmapped segment while the file calls the underlying segment something like unmappedSegment. Imo it would be nicer to call the mesh unmappedSegmentMesh or something similar as it makes the relationship between the unmappedSegment and the "Chunk" clearer.

Hmm, similar to the other thread above, I'm not sure how to do this well. I wouldn't want to use unmappedSegmentMesh, because (a) it's a confusing term for the frequent case where no mappings exist in the first place and (b) when a mapping exists, the back-end maps the individual mesh chunks to the requested id and then returns these chunks. So, they are mapped in a sense. I'm not very happy about it, either, but I'm having trouble to see how to it can be improved easily..

I have one more question before testing: Did I understand proofreading actions correctly that the underlying work / changes to the mapping is solely done by the backend as the frontend only sends update actions to the backend to modify the editable mapping, then awaits for the save queue and after that reloads the modified buckets & meshes to ensure an updated state? Thus, the frontends part is only collecting the required information for the proofreading action, sending it to the backend and then reloading? If that's correct, I think I understood the code correctly (on a relative high level view)

Yes, this is correct :) With the upcoming magic mapping pr, this is going to change, though. Then, the frontend will apply the mapping locally (and maybe also update the mapping locally).

[philippotto]

@MichaelBuessemeyer Thank you for your thorough testing and feedback :+1: I incorporated most of your feedback, fixed the hiccups you noticed and did some performance optimizations. Please have another look and feel free to test again :)

[philippotto]

When I select the Split from all neighboring segments action, I get a A precomputed mesh could not be loaded for this segment. More information was printed to the browser's console. warning toast. All segments and buckets are reloaded as expected but this warning toast should not occur imo.

Can you reproduce this reliably? If so, could you share the steps for reproduction? :)

[MichaelBuessemeyer]

Can you reproduce this reliably? If so, could you share the steps for reproduction? :)

Oh sorry, for me this behaviour occurred every time I used "Split from all neighboring segments" thus I thought that no explicit steps are needed.

I used the following steps:

• load https://bettermeshcontextmenu.webknossos.xyz/annotations/66310e57010000ad00313c3d#2850,4318,1770,0,1
• load segment "Segment 102775"
• in the 3d viewport somewhere at the nucleus of the segment / cell use the "Split from all neighboring segments" option from the context menu. Then the warning should pop up

[philippotto]

Thanks for the repro steps :pray: Indeed, it happens quite frequently on the dev instance for me, too.

@fm3 The error is coming from the back-end (the front-end might still be at fault for sending weird inputs) and I'm not sure whether I interpet it correctly.

The frontend uses the /meshes/chunks route and sends a payload à la:

{
  "meshFile": "meshfile_4-4-2",
  "segmentId": 7571522
}

Error:

[
  {
    "error": "Failed to load chunk list for segment {0} from mesh file “{1}”"
  },
  {
    "chain": "[Server Time 2024-05-02 08:04]  <~ zero chunks"
  }
]

The string interpolation doesn't seem to work, but I guess the backend simply cannot find chunks for id 7571522. Probably because that supervoxel doesn't exist in the editable mapping, anymore?

[philippotto]

When I select the Split from all neighboring segments action, I get a A precomputed mesh could not be loaded for this segment. More information was printed to the browser's console. warning toast. All segments and buckets are reloaded as expected but this warning toast should not occur imo.

As discussed here, this issue is unrelated to this PR and even occurs in view-only mode. I created this issue for the problem.

@MichaelBuessemeyer It would be great if you could have a final look at my latest commits. I'd love to merge the PR today unless someone vetoes :)

[philippotto]

During checking your changes I found a little bug in them:

Argh, you are right. TypeScript should have caught this, too, but didn't :cry: Maybe because a function can be a react component and the toast is able to render this. Still... I fixed it now!