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

Support multiple segmentation layers #5683

Merged
merged 73 commits into from
Sep 8, 2021
Merged

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Aug 23, 2021

Summary

  • A dataset can have multiple segmentation layers now.
  • Volume annotations are always based on ONE segmentation layer. It's not possible to brush in two different segmentation layers
  • Only one segmentation layer can be rendered at all times (for now). Rendering of multiple seg layers itself is not a big problem, but there's a lot of code which simply accesses the "one visible segmentation" layer for which the adaption will take quite some effort.
  • Structures, such as meshes and mappings, are maintained per segmentation layer now.

Also have a look at the follow up issue: #5695

URL of deployed dev instance (used for testing):

To Do

  • support opening DS with multiple segmentation layers
  • allow dynamic switching of visibility for segmentation layers
  • proper support for mappings
  • support for volume annotation (user should decide before hand for which layer an annotation is created)
  • support for (precomputed) meshes
  • create follow up issue: [Follow up]: Multiple segmentation layers #5695
  • the volume tools should be disabled if one has only enabled a secondary segmentation layer (i.e., not a volume layer)
  • fix tests
  • adapt "Create Annotation" and "Make Hybrid" to new fallbackLayerName parameter
  • clarify whether an annotation without fallback should still contain the original segmentation layer (maybe it's safer to do for now and later we can re-consider having both layers at the same time)
  • ensure that volume layer is always visible when creating a new annotation
  • double check api modules (code examples and graceful fallback)
  • test manually

Steps to test:

I suggest a lot of monkey testing, but broadly we should cover the following:

  • Test that the status quo works:
    • view/annotate datasets with zero segmentation layers
    • view/annotate datasets with one segmentation layer
      • with fallback segmentation
      • without fallback segmentation
    • meshes
      • test precomputed mesh loading
      • test ad-hoc mesh loading
    • mappings
      • merger mode
      • JSON mappings
      • agglomerate mappings
  • Test the support of multiple segmentation layers
    • open a dataset with two segmentation layers (simply copy a layer on disk or download a volume tracing and move it into a DS folder, be careful to set a large-enough bbox there)
    • test switching between two segmentation layers
      • enabling one layer should disable the layer automatically
    • mappings
      • test that mappings still work as usual
      • enable mappings for two segmentation layers and switch between them

Issues:


@philippotto philippotto self-assigned this Aug 23, 2021
@philippotto
Copy link
Member Author

@daniel-wer Welcome back 😆 This PR got quite large, but I hope it's reviewable. There are still some todos left, but I think, I should catch a break from this and the main part should be completed. Also see the follow-up issue mentioned in the PR description, as that also sheds light on the restrictions of this first iteration. Let me know if you want to talk about this PR in person 🤙

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.

First part of the review, I'll continue tomorrow. Looking very good so far :)

frontend/javascripts/features.js Show resolved Hide resolved
frontend/javascripts/oxalis/api/api_latest.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/api/api_latest.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/api/api_latest.js Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/model.js Show resolved Hide resolved
frontend/javascripts/oxalis/model.js Show resolved Hide resolved
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.

Finished the second part of the review. Very nice work 👍
The only thing up for debate is the shader code I'd say. Happy to discuss this in person :)

Since the testing is quite involved, I'll wait until we discussed that and all other Todos are taken care of.

Comment on lines 481 to 484
// Remove other segmentation layers, since we are adding a new one.
// This is a temporary workaround. In the long term we want to support
// multiple segmentation layers.
layers = layers.filter(layer => layer.category !== "segmentation");
Copy link
Member

Choose a reason for hiding this comment

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

If the user requests that there shouldn't be a fallback and we don't kick out the other segmentation layer(s), wouldn't we effectively ignore the users decision?

@philippotto
Copy link
Member Author

Ok, my tests were mostly successful except for the merger mode which seems to be broken sometimes. Also, there is the question on which segmentation layer the merger mode should work. I'll have a look at this tomorrow.

@philippotto
Copy link
Member Author

Ok, my tests were mostly successful except for the merger mode which seems to be broken sometimes. Also, there is the question on which segmentation layer the merger mode should work. I'll have a look at this tomorrow.

Should be fixed now.

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.

Very nice! I mostly tested that there was no regression in single-segmentation-layer-datasets and everything worked as expected 👍

LGTM

@philippotto philippotto merged commit c3cbc7f into master Sep 8, 2021
@philippotto philippotto deleted the multiple-segmentations branch September 8, 2021 14:36
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.

(Cheap) Support for multiple segmentation layers
3 participants