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

Don't remove other volume layers when opening volume annotation #6186

Merged
merged 21 commits into from
May 16, 2022

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Apr 29, 2022

When creating a new annotation with a volume layer (without fallback) for a dataset which has an existing segmentation layer:

  • keep on-disk segmentation layers in the annotation so that they can be activated
  • hide on-disk segmentation layer on first view and make new volume layer visible
  • ensure that changing the layer visibility of a layer in an annotation, does not change the layer visibility for the dataset settings. otherwise, opening the dataset would likely show no segmentation layer.

URL of deployed dev instance (used for testing):

Steps to test:

  • Open a dataset with a segmentation layer (keep that dataset in an own tab)
  • Hide the segmentation layer & refresh -> layer should still be hidden
  • Unhide the segmentation layer & refresh -> layer should still be shown
  • Open a second tab to create an annotation for that dataset via the dashboard (use the advanced menu) and select that a new segmentation should be created
  • The empty segmentation layer should be visible by default. Also, the original segmentation layer should still be listed in the left sidebar (but hidden by default).
  • Reload the annotation -> visibilities shouldn't have changed (i.e., the original segmentation layer should be invisible)
  • Reload the dataset in tab 1 -> visibilities shouldn't have changed there (i.e., the original segmentation layer should still be visible).
  • Make the original segmentation layer visible in tab 2 and reload --> original segmentation layer should still be visible.

Issues:


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

@philippotto philippotto self-assigned this Apr 29, 2022
@philippotto philippotto changed the title [WIP] Don't remove other volume layers when opening volume annotation Don't remove other volume layers when opening volume annotation May 10, 2022
@philippotto philippotto requested a review from daniel-wer May 10, 2022 13:16
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.

Nice, works as expected for me 👍

Since this functionality is somewhat brittle, would it be a good candidate for a snapshot test using the e2e test setup? One could create a new annotation without a fallback layer, and would test that the visibility and number of the layers is correct.

frontend/javascripts/oxalis/model/sagas/settings_saga.ts Outdated Show resolved Hide resolved
frontend/javascripts/oxalis/model_initialization.ts Outdated Show resolved Hide resolved
@philippotto
Copy link
Member Author

Thanks for your review!

Since this functionality is somewhat brittle, would it be a good candidate for a snapshot test using the e2e test setup? One could create a new annotation without a fallback layer, and would test that the visibility and number of the layers is correct.

Do you mean the e2e tests or the screenshot tests? The e2e tests mainly test the REST API between JS and Scala which doesn't really lend itself to test the behavior introduced in this PR (since the main logic happens in wk JS core). However, I just adapted the screenshot tests to also create a hybrid annotation for a dataset (once with a fallback layer and once without). This tests the basic setup of the default visibilities quite well, I think.

Testing that the visibilities are changed correctly would be nice, too. However, I don't see an easy way to do so, since this logic happens in the sagas. Interacting with the page via puppeteer sounds too messy/brittle and calling the sagas manually without the surrounding page context is also not really lucrative in my opinion. This is why I hope that the two new screenshot tests are a good trade-off for now. Do you agree?

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 👍

Do you mean the e2e tests or the screenshot tests?

I meant the e2e tests, but I overlooked that the frontend doesn't run in that case. Thanks for pointing that out. In that case, I think the screenshot testing is the best we can do right now.

@@ -292,6 +292,10 @@ workflows:
version: 2
circleci_build:
jobs:
- nightly:
Copy link
Member

Choose a reason for hiding this comment

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

Reminder to remove this before merging

@philippotto philippotto enabled auto-merge (squash) May 16, 2022 09:10
@philippotto philippotto merged commit ab05c0f into master May 16, 2022
@philippotto philippotto deleted the keep-seg-layers branch May 16, 2022 09: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.

Don't remove other volume layers when opening volume annotation
3 participants