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

Allow to restore older volume versions #3349

Merged
merged 11 commits into from
Oct 16, 2018
Merged

Allow to restore older volume versions #3349

merged 11 commits into from
Oct 16, 2018

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Oct 11, 2018

This PR enables the volume version restore view and adds previewing of older volume versions.

URL of deployed dev instance (used for testing):

Steps to test:

  • Open a volume/hybrid tracing. Do some volume tracing, then open the version view by clicking on "Restore older version" in the dropdown next to the save button.
  • Preview older versions of the volume tracing. The segmentation data should live reload.
  • Restore an older version. Then volume trace some more and save, everything should work.
  • Re-Open the version view, you should see the "Reverted to version x" entry in the version log.

Issues:


  • Updated changelog
  • Updated documentation if applicable
  • Needs datastore update after deployment
  • Ready for review

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.

Nice, code looks very good! Unfortunately, I couldn't test it on the dev server (I get an column not found exception). remove_dev and install_dev didn't work.

@jstriebel @fm3 If you have time on your hands, maybe you can double check why the schema isn't updated in this scenario?

@@ -275,6 +275,18 @@ class DataCube {
this.bucketIterator = ++this.bucketIterator % this.MAXIMUM_BUCKET_COUNT;
}

collectAllBuckets(): void {
for (const bucket of this.buckets) {
if (bucket != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this check necessary? According to the flow types, this.buckets is an array of non-null databuckets?

Copy link
Member Author

Choose a reason for hiding this comment

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

The buckets array is created like this: this.buckets = new Array(this.MAXIMUM_BUCKET_COUNT);. When iterating over buckets using the for ... of syntax, you'll get all the empty entries as well.

@@ -168,6 +171,9 @@ function* labelWithIterator(iterator, contourTracingMode): Saga<void> {
}

function* copySegmentationLayer(action: CopySegmentationLayerAction): Saga<void> {
const allowUpdate = yield* select(state => state.tracing.restrictions.allowUpdate);
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we wouldn't need to add this check to all volume-modifying functions but instead have a central place for it, but this is probably out of scope of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I totally agree with you, let's think of a nice way to do this as part of another PR :)

Copy link
Member Author

Choose a reason for hiding this comment

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

#3356 for reference

package.json Outdated
@@ -79,7 +79,7 @@
"gl": "scalableminds/headless-gl#4.0.5"
},
"scripts": {
"start": "./sbt run -jvm-debug 5005",
"start": "./sbt run -jvm-debug 5005 -J-XX:MaxMetaspaceSize=2048m",
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional? Is this only used for dev or also for production mode?

Copy link
Member

Choose a reason for hiding this comment

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

yarn start is only used locally (and we merged the same change already elsewhere)

@philippotto
Copy link
Member

I just tested it and it works very well 👍 However, I noticed two things:

  1. I created a new volume tracing using another segmentation. after doing two brushes and returning to the first version, the first brush was still visible. maybe there is an off-by-one error?
  2. When previewing another version and closing the view, that version is still active. Is this intended?

@daniel-wer
Copy link
Member Author

I created a new volume tracing using another segmentation. after doing two brushes and returning to the first version, the first brush was still visible. maybe there is an off-by-one error?

There is a version 0, which is the initial version, but that is not sent by the server, and therefore not yet displayed. I already fixed this on my version view refinement branch :)

create-tracing

When previewing another version and closing the view, that version is still active. Is this intended?

That is not intentional, I'll have a look.

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, will rely on your testing.
this will have to be adapted for the tracingstore module when bringing #3281 up to date (assuming that this one here is merged first. Otherwise this needs to be adapted here)

@youri-k youri-k merged commit be56b0b into master Oct 16, 2018
@fm3 fm3 deleted the volume-restore branch October 17, 2018 11:36
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.

Versioned volume tracings
4 participants