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

Segments lists #5696

Merged
merged 105 commits into from
Nov 1, 2021
Merged

Segments lists #5696

merged 105 commits into from
Nov 1, 2021

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Aug 30, 2021

This PR:

  • changes the old "Meshes" tab to a "Segments" tab
  • renders "Segments" in the new tab. The list of segments is not guaranteed to be complete or up to date. The list "grows" while creating an annotation or browsing a dataset. For example, selecting an existing segment or drawing with a new segment id will both ensure that the segment is listed. Outdated segments are not handled at all, right now. In a follow-up, we can validate existing segments ad-hoc and show an "outdated" warning etc.
  • When loading a mesh, the mesh will be added as child to the segment (at least visually).
  • The mesh UI (i.e., the header in the segments tab) was reworked so that the user can also choose which quality they want to have for the meshing.

URL of deployed dev instance (used for testing):

Steps to test:

  • create a new annotation (with or without fallback layer) and brush --> a segment should appear in the segments tab
  • open a dataset in view mode and hover existing segments --> the segments should appear, too
  • open a dataset in view mode and compute meshes (precomputed or adhoc) --> the segments should appear, too
  • monkey-test everything around segmentations, meshes and mappings
  • please also have an eye on performance (for example, try to have more than 100 segments by shift+clicking segments in an annotation with fallback layer). overall wk performance should not be slowed down. the segment list does not use virtual rendering, but shouldn't cost anything, if the list is hidden. so, we can do further perf tuning as a follow-up.

Issues:


@MichaelBuessemeyer
Copy link
Contributor Author

I stumbled upon a strange behavior/bug that still needs to be fixed:

This happens when a part of a segment is overdrawn and the segment's position has to be set again.

Steps to reproduce:

  1. In my concrete case, I draw a simple dot in one part of the dataset and then another dot in another part of the dataset. This ensures that the two parts of the segment are in different buckets.
  2. Then I overdrew the second drawn dot, where currently the segment's position is at. This causes the code to find a new position for the segment. This is then exactly where the first dot was drawn but one z-slice off.
  3. To be able to see the first drawn dot, click on the segment in the list of the meshes tab. From there move one z slice backward and you can see the segment.

Expected behavior:

It would be ideal if it wasn't necessary to move back one slice.

@MichaelBuessemeyer
Copy link
Contributor Author

Keeping the segment list always in sync with the volume data is difficult.
I added a section about this problem and how it might be possible to tackle it to the notion page:
https://www.notion.so/scalableminds/Design-Doc-Segment-Lists-c8ec5e23764b4a578a452a4c7290000e

@MichaelBuessemeyer
Copy link
Contributor Author

Additional comments that do not really fit into the code:

  • There is a naming mismatch / duplication: cellId has the same meaning as segmentId. Sometimes the name cellId (in old code) is used and the name segmentId is used in the new code. In some code passages, it makes more sense to use one name instead of the other one. But it might not be intuitive that the names actually mean the same.

@daniel-wer
Copy link
Member

daniel-wer commented Oct 27, 2021

Had a great time testing this PR. The segment list feels quite good and is intuitive to use in my opinion, great job 👍

These are the issues I encountered:

  • If the Segments tab in a skeleton-only annotation is opened, the save_saga crashes, because it calls enforceVolumeTracing at some point.
  • If an ad-hoc mesh is deleted while it is being computed/loaded, it is deleted but then sometimes loads again or the entry in the segments list doesn't fully disappear (it stays there unchecked and needs to be deleted again. If one tries to check it again, a saga crashes with an error).
  • [ ] Probably unrelated to this PR, but a bug. If in a volume annotation with fallback layer, "Only overwrite empty areas" is activated and I brush in mag 4, it looks like nothing is annotated (as would be correct, since there are segments), but after zooming in, parts of the data were overwritten (see screenshot) extracted to "Only overwrite empty areas" is not respected when brushing in higher mag #5796
    partially_overwritten_data

Copy link
Contributor

@youri-k youri-k left a comment

Choose a reason for hiding this comment

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

Backend LGTM 🎉
I like the refactoring to ApplyableVolumeActions.

See my small comments, but they do not prevent this from merging 🙂

@philippotto
Copy link
Member

  • If the Segments tab in a skeleton-only annotation is opened, the save_saga crashes, because it calls enforceVolumeTracing at some point.

Should be fixed now :)

  • If an ad-hoc mesh is deleted while it is being computed/loaded, it is deleted but then sometimes loads again or the entry in the segments list doesn't fully disappear (it stays there unchecked and needs to be deleted again. If one tries to check it again, a saga crashes with an error).

After I fiddled quite a bit with "do I need to exit here in this saga?"-code in several places, I remembered how easy it is to cancel a saga 🎉 race can simply be used for this. This is one of the times, where it shows that our decision to go with redux sagas was really worth it :)

  • Probably unrelated to this PR, but a bug. If in a volume annotation with fallback layer, "Only overwrite empty areas" is activated and I brush in mag 4, it looks like nothing is annotated (as would be correct, since there are segments), but after zooming in, parts of the data were overwritten (see screenshot)

Oh, good find. I created #5796 since this issue indeed also exists on the master branch.

@philippotto
Copy link
Member

Thank you for your detailed feedback and testing! I think, I covered everything now (if the CI passes ^^). Feel free to have another look. I plan to merge/deploy this on Monday 🤞

@philippotto
Copy link
Member

@fm3 The back-end doesn't need any migration when deploying this, does it?

@fm3
Copy link
Member

fm3 commented Nov 1, 2021

@fm3 The back-end doesn't need any migration when deploying this, does it?

Correct, but note that the tracingstore module needs to be upgraded with the main instance

@philippotto
Copy link
Member

Correct, but note that the tracingstore module needs to be upgraded with the main instance

Ok, cool :)

@daniel-wer When you've checked my last commits and have given your final "Go", I'd merge this PR today :)

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

LGTM, works beautifully 👍

resolutionInfo,
removeExistingIsosurface,
),
cancel: _take(
Copy link
Member

Choose a reason for hiding this comment

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

Very elegant 🤩

@philippotto philippotto merged commit 45c2c8b into master Nov 1, 2021
@philippotto philippotto deleted the add-segments-list branch November 1, 2021 10:11
@philippotto philippotto mentioned this pull request Nov 2, 2021
6 tasks
@philippotto philippotto mentioned this pull request Nov 9, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants