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

Add segment groups #6966

Merged
merged 72 commits into from
Apr 27, 2023
Merged

Add segment groups #6966

merged 72 commits into from
Apr 27, 2023

Conversation

frcroth
Copy link
Member

@frcroth frcroth commented Apr 5, 2023

URL of deployed dev instance (used for testing):

Steps to test:

  • create a new annotation
  • switch to the segments tab
  • create some segments by brushing with some ids or simply click on existing segments
  • right click on the root node in the segments tab to create one or multiple groups
  • drag segments into the groups
  • delete groups
  • use undo/redo
  • save & reload to check that everything was saved correctly

Todo

  • Front end (@philippotto)
    • create groups
    • delete group (recursively)
    • renaming of groups
    • dnd for groups and segments
    • undo/redo for group mutations
    • bugs
      • segments and groups can be moved higher than the root which makes them disappear
      • groups are not shown when no segments exist at all
      • undo might still be buggy. I fixed some bugs, but I still managed to break it at some point. I didn't find a clear reproduction, though.
      • item is moved to wrong group sometimes
    • clean up
  • doublecheck export/import via NML (backend does this, for frontend, there is only a skeleton-related download, so no need to do adapt this in my opinion)
  • Handle merging of groups
  • Fix e2e test

Issues:


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

@philippotto
Copy link
Member

Changing the color of a segment leads to a crash

Oops, this is broken on master currently. I pushed a fix. If I merge this PR on Monday, it should be fine, I think.

@philippotto
Copy link
Member

Changing the color of a segment leads to a crash

Oops, this is broken on master currently. I pushed a fix. If I merge this PR on Monday, it should be fine, I think.

@frcroth just pointed out that the back-end review is also pending, so I extracted the fix into #7000 to not create an unnecessary pressure.

Copy link
Member

@fm3 fm3 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 :)

  • During testing I noticed that you can get -1 as group id for segments if you drag them into a group and then back into the root group. The -1 should not reach the backend/the NML. I guess this is a front end issue

@philippotto
Copy link
Member

The PR should be ready now from my side :) Not sure whether we want to wait for @daniel-wer's final review or maybe it's enough if somebody else does a last round of monkey testing?

Importing an NML with segment groups through the frontend import fails or behaves wrongly, because the segment groups are not distinguished from the tree groups. So either the segment groups are imported as tree groups or the import fails, because of duplicate IDs. See this example NML:
NML_with_segment_groups.zip
I understand that the frontend download doesn't include the segment groups, because it explicitly says "Download selected trees", but maybe it should import the segment groups and segments correctly? Otherwise, the import modal should state that only trees are imported, I think.

The import of skeleton groups should not be confused by segment groups anymore. I started implementing the import of segment groups, but it turned out to be a rabbit hole. In the end, the main problem was that it's not clear into which volume layer the segments should be imported (the id can change). Also, semantics-wise it is weird, because no actual volume data is imported. So, I decided together with @fm3, that the import should stick to skeletons and for everything else, the backend import should be used.

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, well done! I didn't notice any issues during retesting 🎉

@philippotto philippotto enabled auto-merge (squash) April 27, 2023 11:24
@philippotto philippotto merged commit 8feb9f0 into master Apr 27, 2023
@philippotto philippotto deleted the segmentgroups branch April 27, 2023 11:39
hotzenklotz added a commit that referenced this pull request Apr 28, 2023
…ove_wkconnect

* 'master' of github.com:scalableminds/webknossos:
  Update docker compose commands + dev install readme (#7002)
  Add segment groups (#6966)
  Add screenshot nightly test for wkorg (#7030)
  Workaround for WebGL crash for datasets with many segmentation layers (#6995)
  Fix download of public annotation, include access ctx in user cache key (#7025)
  Fix that changing a segment color could lead to a crash (#7000)
  Add more error chaining to annotation download (#7023)
  Guard against NaNs in shader (#7018)
  Store editable mappings in multiple fossildb columns+keys (#6903)
  Context action to move tree to group (#7005)
  Release 23.05.0 (#7014)
  Remove vault cache when reloading dataset (#7007)
  Fix viewing of public datasets (#7010)
  Update screenshots scalebar positioning (#7003)
  Update team members (#6999)
hotzenklotz added a commit that referenced this pull request May 17, 2023
…ty-list-drawings

* 'master' of github.com:scalableminds/webknossos: (25 commits)
  Fix issues with styling in dark mode on login page (#7052)
  Fix nightly by setting missing token (#7048)
  Release 23.05.1 (#7042)
  DRY types in update_actions.ts (#7036)
  Remove some spammy logging from backend (#7039)
  Use zarr string fill values (#7017)
  Fix voxel offset for Neuroglancer Precomputed datasets (#7019)
  Log when user is activated (#7027)
  Fix exception in applying UpdateTreeGroupVisibility skeleton action (#7037)
  Fix organization storage layouting (#7034)
  Update docker compose commands + dev install readme (#7002)
  Add segment groups (#6966)
  Add screenshot nightly test for wkorg (#7030)
  Workaround for WebGL crash for datasets with many segmentation layers (#6995)
  Fix download of public annotation, include access ctx in user cache key (#7025)
  Fix that changing a segment color could lead to a crash (#7000)
  Add more error chaining to annotation download (#7023)
  Guard against NaNs in shader (#7018)
  Store editable mappings in multiple fossildb columns+keys (#6903)
  Context action to move tree to group (#7005)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segments Groups
4 participants