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

Editable Mappings aka Supervoxel Proofreading #6195

Merged
merged 148 commits into from
Jun 22, 2022
Merged

Conversation

fm3
Copy link
Member

@fm3 fm3 commented May 9, 2022

Prototype for Editable Mappings a.k.a Supervoxel-Based Proofreading

Compare notes at https://www.notion.so/scalableminds/Design-Doc-Supervoxel-Proof-Reading-e012fb444d1242fdacaaa1c9d498f285

Assumptions for Prototype

  • no undo/redo support for now
  • no mutating volume buckets in editable-mapping annotations
  • no evolutions / backwards compatibility
  • only one datastore per tracingstore
  • no support in webknossos-connect
  • no uint64 segmentations (can’t operate directly on Long then. Also, those are currently forbidden for volume annotations anyway.)
  • a valid hdf5 agglomerate file exists for the fallback layer
  • no export/download/copy to my account

TODO Frontend

  • Hide proofread button if no agglomerate view exists
  • Make proofreading work for non-uint32 segmentations
  • Repair isosurface computation for non-editable mapping annotations with fallback layer
  • Split up proofreading saga functions into smaller parts
  • Decide whether the meshes from the oversegmentation are helpful or not (disabled by default for now, enable using window.__proofreadProximityNm = 2000;)
  • Gracefully fail if proofreading is not allowed, because the user brushed before (maybe find out in advance when activating the proofread mode?) (not possible to find out in advance yet, but the code fails gracefully)

TODO Backend

  • find largest agglomerate id from hdf5 agglomerate mapping
  • how to let the front-end know that a volume layer has an editable mapping?
  • merge master
  • clean up isosurface code (remove EditableMappingsIsosurfaceService?)
  • perf
    • Caching for infos fetched from datastore
      • unify caching code
      • add caching for largest agglomerate id
    • Store as JSON vs Protobuf?
  • check that no brushing has happened? (key-scan on make editable, then later refuse update bucket actions if editable mapping exists)

Follow-Up

Moved to follow up issue #6288

Note on FossilDB Data

This branch introduces two new column families in fossilDB. After running that and then switching to a different wk branch, you need to run rm -rf fossildb/data; git checkout fossildb/data to re-initialize the fossildb content, otherwise you’ll get RocksDBException: You have to open all column families

URL of deployed dev instance (used for testing):

Steps to test:

  • abc

Issues:


@daniel-wer
Copy link
Member

@fm3 As a heads up, I merged the current master so an evolution is required locally.

I implemented a rudimentary proofreading tool. If the tool is active, agglomerate skeletons can be loaded by clicking onto agglomerates (segments). If an edge is deleted in such an agglomerate or two agglomerates are merged, the saga will send update actions (to the incorrect end point, see questions). Afterwards, the volume tracing layer is reloaded.

Also a couple of questions:

  • Could you document the new routes and update action formats? (Especially the ones to change the mapping and to pin a mapping (make it editable))
  • Did I get it correctly that the {split,merge}Agglomerate update actions are neither of type skeleton nor volume but of a new type? If so, what's the type and what's the respective end point the actions should be sent to?
  • Currently, I designed the {split,merge}Agglomerate update actions differently than in the design doc:
    export type SplitAgglomerateUpdateAction = {
      name: "splitAgglomerate";
      value: {
        segmentPosition1: Vector3;
        segmentPosition2: Vector3;
      };
    };
    export type MergeAgglomerateUpdateAction = {
      name: "mergeAgglomerate";
      value: {
        segmentPosition1: Vector3;
        segmentPosition2: Vector3;
      };
    };
    In the design doc, the agglomerate ids are included as well. Are those needed/helpful? Also, as discussed I exchanged the segment ids for segment positions.

@daniel-wer
Copy link
Member

@philippotto Thank you so much for the helpful feedback and extensive testing!

I should have addressed most of your feedback by now, except for some of the usability issues you noticed during testing. It would be great if you could have another look at the code.

I've disabled the rendering of the fine meshes from the oversegmentation by default for now. They can be enabled by setting window.__proofreadProximityNm to a value larger than 0.

Maybe this has already been like this, but I noticed these artifacts in the mesh when looking from a certain direction:

This seems to be related to the transparent meshes. Researching online, rendering double sided transparent objects seems to be rather hard. I'm still fiddling around to find a good solution, maybe rendering the passive (very transparent) meshes only one sided would be an option. I've reset the transparency for the "normal" meshes, so this shouldn't be an issue for those.

Somehow, I managed to create various redundant agglomerate skeletons (see screenshot). I didn't notice this at first, but wondered why I couldn't delete an edge (the edge existed multiple times which is why I thought the deletion didn't happen).

I would be interested whether you can still reproduce this in the newly deployed version. Not sure what happened there or whether an older version was deployed - I made sure duplicate agglomerate skeletons are not loaded quite a while ago 🤔

When clicking on a agglomerate node in the 3D viewport, sometimes this toast comes up "Clicked on the background. Please click on a segment to load a skeleton."

I wasn't able to reproduce this :/ What I've noticed is that if agglomerate skeleton node positions are exactly at the lower (or left) edge of a segment, the segment ID that is read seems to be that of the lower (or left) segment instead of the actual segment the node is in. I'll have to look into that, but it could be related.

After I split a tree, I hit "undo" (this is probably not supported, right now?). Then, both tree components were gone. Hitting "undo" again, restored the initial tree. Redoing twice took me back to the two tree components. Not sure why there is a superfluous (?) entry in the undo stack.

Yes, Undo/Redo is not properly supported for now. The disappearing trees happened, because I used to delete and reload the agglomerate skeletons after a proofreading action to make sure the names were updated correctly. I've switched to simply renaming the skeletons in the frontend. Still, the renaming causes separate undo stack entries which is not ideal. Not sure what to do about this, to be honest. A fairly involved solution could be to actually reverse the merge/split actions so that the editable mapping actions is undone as well.

It feels a bit unusual to split and merge trees with the proofreading tool. I could imagine that users will quickly switch back to the skeleton tool for that (which wouldn't do any editing on the mapping then). Not sure how to solve this, though.. Maybe one could detect that the user edited an agglomerate skeleton in the wrong mode? I've made the mistake myself during testing multiple times..

We could try that (only if an editable mapping is active maybe to avoid false positive warnings) and wait for feedback whether this is annoying or helpful :)

@daniel-wer
Copy link
Member

Somehow, I managed to create various redundant agglomerate skeletons (see screenshot). I didn't notice this at first, but wondered why I couldn't delete an edge (the edge existed multiple times which is why I thought the deletion didn't happen).

I would be interested whether you can still reproduce this in the newly deployed version. Not sure what happened there or whether an older version was deployed - I made sure duplicate agglomerate skeletons are not loaded quite a while ago thinking

Nevermind, I was able to reproduce this. This happens if an editable mapping X is activated and an agglomerate Y that was not modified before is selected. The frontend checks whether a tree with name "agglomerate Y (X)" exists. If it doesn't exist, the agglomerate skeleton is loaded. However, the loaded agglomerate skeleton has the name "agglomerate Y (agglomerate_view_80)", despite mapping X being active (probably because agglomerate Y was not modified before by mapping X). Therefore, on every click a new skeleton is loaded, because the expected and actual agglomerate skeleton tree names differ.
@fm3 Would this be possible to fix in the backend? If not, I don't have a good idea how to handle this behavior 🤔

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

code looks great 👍 just left some cosmetics 💅 will test again now!

@philippotto
Copy link
Member

Yes, Undo/Redo is not properly supported for now. The disappearing trees happened, because I used to delete and reload the agglomerate skeletons after a proofreading action to make sure the names were updated correctly. I've switched to simply renaming the skeletons in the frontend. Still, the renaming causes separate undo stack entries which is not ideal. Not sure what to do about this, to be honest. A fairly involved solution could be to actually reverse the merge/split actions so that the editable mapping actions is undone as well.

That's probably something for the next iteration. Is there a follow-up issue already?
Also: Would it be easy to warn the user if they try to use undo/split in combination with the proofreading? I feel like this could easily produce inconsistent states otherwise 🤔

@daniel-wer
Copy link
Member

@philippotto I should have addressed all of your feedback and also moved the followup tasks to a new issue #6288

The newest version is deployed on the dev branch, would be great if you could have another look 🦅

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome!! I was able to fix a few mergers and splits while testing. The added warnings against manipulating the skeletons in the wrong tool were very helpful [1].

The code also looks very good. I think, the PR can be merged then :shipit:

By the way, during testing I once again noticed that a min-cut tool would probably be very helpful 🙈

[1] However, I already managed to use the wrong tool in the very beginning before the mapping was editable (so, no warning there). This happened because the 3 shortcut to toggle the segmentation layer also switches the tool to move. However, that's not the PR's fault :)

@@ -127,15 +128,13 @@ export const redoAction = (callback?: () => void): RedoAction => ({
export const disableSavingAction = (): DisableSavingAction => ({
type: "DISABLE_SAVING",
});
// Unfortunately, using type Dispatch produces countless Flow errors.
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@daniel-wer daniel-wer merged commit a5a1524 into master Jun 22, 2022
@daniel-wer daniel-wer deleted the editable-mappings branch June 22, 2022 11:45
hotzenklotz added a commit that referenced this pull request Jun 23, 2022
…type

* 'master' of github.com:scalableminds/webknossos:
  fix error when loading agglomerate skeleton for single-segment agglomerate (#6294)
  Editable Mappings aka Supervoxel Proofreading (#6195)
  Increase maximum interpolation depth to 100 (#6292)
  Add download modal to dataset view actions (#6283)
  Drop "Explorational" from info tab (#6290)
  Allow version history view in annotations not owned by you (#6274)
  Bucket loading meter (#6269)
  Revert "Merge "Shared Annotations" with "My annotations" (#6230)" (#6286)
  Merge "Shared Annotations" with "My annotations" (#6230)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify async cache usages, refactor Easier proofreading of volume data
4 participants