Add volume undo/redo support #4771

MichaelBuessemeyer · 2020-08-18T12:37:57Z

This PR adds support for Undo / Redo of volume annotations, while still supporting skeleton undo in e.g. hybrid tracings.

URL of deployed dev instance (used for testing):

https://addvolumeundo.webknossos.xyz

Steps to test:

Open a hybrid tracing
randomly draw around and create some node/trees
Use undo/redo as you wish. Undo should redo your last skeleton action or your last brush stroke.
Then save the tracing and reload it. Everything should look the same after reloading.

Issues:

fixes Add undo for volume annotations #4001

Updated (unreleased) changelog
~~[ ] Updated (unreleased) migration guide if applicable~~
~~[ ] Updated documentation if applicable~~
~~[ ] Adapted wk-connect if datastore API changes~~
~~[ ] Needs datastore update after deployment~~
Ready for review

- not everthing is undone :/

… into add-volume-undo

MichaelBuessemeyer · 2020-08-19T10:08:16Z

@daniel-wer I just fixed the flow error and added a better type annotation for the join function from redux-saga. => This PR is ready to receive your awesome feedback 🕺

daniel-wer

Wow, this is extremely cool! You did a very good job implementing this feature and structuring the code, kudos 🚀

I added some suggestions and asked for a couple of clarifications. I'll test next and will report back afterwards.

I also think that we should carefully measure the performance and memory impact of this. A good way would probably be to actually volume trace for 20 slices (as that is the current undo stack size), and undo/redo a couple of times in between and in the end. I would be interested in the memory overhead (using the dev console memory profiler) and whether undoing/redoing feels relatively smooth.

Very excited for this feature to land! ✨

frontend/javascripts/oxalis/model/bucket_data_handling/bucket.js

frontend/javascripts/oxalis/model/sagas/effect-generators.js.flow

frontend/javascripts/oxalis/model/sagas/save_saga.js

…-volume-undo

daniel-wer · 2020-08-20T10:14:43Z

Testing went mostly really well, I was pleasantly surprised by the performance, really cool! :)

These are the issues I encountered:

When clicking with the brush, the change is not recognized and the undo stack is not populated. Only after "stroking" (mouse-dragging) with the brush, all the changes are pushed to the undo stack at once.
When undoing a volume change, the resulting change is not saved. The save button doesn't indicate a changed tracing and the tracing can simply be reloaded without a warning of unsaved changes. After reloading the undone change is in fact not undone.
With this PR it'll be possible to change the segmentation (via undo) when the merger mode is active. This is otherwise not allowed as the volume annotation buttons are disabled if merger mode is active. I don't have a clever idea how to prevent this, and I don't think it's that important to fix it in this PR, but could you create an issue for this?

MichaelBuessemeyer · 2020-08-20T12:53:43Z

With this PR it'll be possible to change the segmentation (via undo) when the merger mode is active. This is otherwise not allowed as the volume annotation buttons are disabled if merger mode is active. I don't have a clever idea how to prevent this, and I don't think it's that important to fix it in this PR, but could you create an issue for this?

I have two ideas for this:

Before undoing a volume annotation step, check whether the merger mode is activated. If that's the case, show an info toast and do not update the volume data.
When the merger mode gets activated, erase all undo volume states from the undo queue. This way it is impossible to edit the volume with undo.

Both are pretty simple solutions but have disadvantages.

The first option makes it impossible to undo any skeleton actions that were done before the merger mode was activated.
1. edit skeleton
2. edit volume
3. activate merger mode
4. step 1 cannot be undone
The second option makes it impossible to undo any volume action, after deactivating the merger mode again: e.g.:
1. edit volume
2. activate merge mode
3. deactivate merger mode
4. first step cannot be undone

Let me know what you think @daniel-wer. I just implemented the first option.

daniel-wer · 2020-08-20T12:57:51Z

Let me know what you think @daniel-wer. I just implemented the first option.

Awesome, the first option sounds very good to me 👍

- notice every brush step as an undo step - undo triggers pushqueue - prevent undoing volume in merger mode

MichaelBuessemeyer · 2020-08-20T13:16:53Z

@daniel-wer I just fixed everything (CI, your feedback, etc.)

Could you please check the newest changes?

daniel-wer

Nice, thanks for thoroughly applying the feedback, I added a couple more comments, nothing major :)

frontend/javascripts/messages.js

frontend/javascripts/oxalis/model/bucket_data_handling/bucket.js

frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.js

frontend/javascripts/oxalis/model/reducers/annotation_reducer.js

frontend/javascripts/oxalis/model/sagas/save_saga.js

daniel-wer · 2020-08-20T15:38:42Z

I tested again and can confirm that the undo of brush-clicking and the saving now works correctly 🎉
The merger mode fix does seem to have a little problem, though, at least it doesn't work as I expected. Undoing a volume change is prevented and the toast is shown, but the change seems to be permanently popped off the undo stack, because if I keep pressing undo, earlier skeleton changes from before the volume change are undone. I would've expected the stack to just stay the same. Undoing further would then only be allowed when turning off the merger mode.

MichaelBuessemeyer · 2020-08-21T09:45:41Z

I just found another bug, that (i guess) was already existing:
How to produce (not very accurate) :

open a skeleton tracing
create some nodes
undo all actions -> this will cause that there is no active node left and the node radius will be reset to 0. This triggers an action that will clear the redo stack.
trying redoing the last actions will fail

-> I fixed this by caching the node radius in the settings component, thus preventing a reset to 0 and therefore avoiding the update action.

MichaelBuessemeyer · 2020-08-21T09:46:54Z

I tested again and can confirm that the undo of brush-clicking and the saving now works correctly tada
The merger mode fix does seem to have a little problem, though, at least it doesn't work as I expected. Undoing a volume change is prevented and the toast is shown, but the change seems to be permanently popped off the undo stack because if I keep pressing undo, earlier skeleton changes from before the volume change are undone. I would've expected the stack to just stay the same. Undoing further would then only be allowed when turning off the merger mode.

I simple mistake from my side. I just fixed that.

Could you please check the newest version 🤓 📖 @daniel-wer ?

… done

MichaelBuessemeyer · 2020-08-24T11:46:01Z

@daniel-wer I just noticed I forgot to push the code. ^ Here is the fix

daniel-wer

Everything worked beautifully during my testing 🎉 Great work!

daniel-wer · 2020-08-24T14:56:07Z

@MichaelBuessemeyer Please add a changelog entry for this cool new feature :)

MichaelBuessemeyer added 7 commits August 13, 2020 16:55

fix typo

88af051

WIP: Adding volume annotation in batches to undo stack

76357d0

first version with undo

f2d4f0d

- not everthing is undone :/

added first version of volume undo, not working correctly :/

defa7aa

I think undo works now

94d3b4a

clean up

72975e8

small fix

d5b6c65

MichaelBuessemeyer added frontend new feature hybrid labels Aug 18, 2020

MichaelBuessemeyer self-assigned this Aug 18, 2020

MichaelBuessemeyer and others added 3 commits August 18, 2020 14:38

Merge branch 'master' into add-volume-undo

c0b55dd

fix tests and adjust naming of worker

da7f7fd

Merge branch 'add-volume-undo' of github.com:scalableminds/webknossos…

373442a

… into add-volume-undo

MichaelBuessemeyer requested a review from daniel-wer August 18, 2020 13:20

MichaelBuessemeyer added 4 commits August 18, 2020 15:28

add comment

5182358

add type annotation

e5ebdc8

fix flow

30a6ba8

add better flow defintion for saga effect join

9bca52f

daniel-wer reviewed Aug 19, 2020

View reviewed changes

MichaelBuessemeyer added 2 commits August 20, 2020 11:58

apply feedback

d58e285

Merge branch 'master' of github.com:scalableminds/webknossos into add…

6240fd3

…-volume-undo

remove useless type cast & fix typo

9a0023c

MichaelBuessemeyer added 3 commits August 20, 2020 15:00

various fixes

83a1467

- notice every brush step as an undo step - undo triggers pushqueue - prevent undoing volume in merger mode

pretty flow file

5c1a7e2

fix tests

eb4e099

MichaelBuessemeyer and others added 2 commits August 20, 2020 15:18

Merge branch 'master' into add-volume-undo

bf83dcb

fix linting

e64a7af

daniel-wer reviewed Aug 20, 2020

View reviewed changes

MichaelBuessemeyer added 2 commits August 21, 2020 11:08

apply feedback

c8813f3

cache node radius to avoid clearing the redo stack

3081000

MichaelBuessemeyer and others added 3 commits August 21, 2020 11:48

Merge branch 'master' into add-volume-undo

06378fb

fix merge

3d61885

undo node radius fix && only clear redo stack if a relevant action is…

8a27a7f

… done

daniel-wer approved these changes Aug 24, 2020

View reviewed changes

add changelog entry

dd89a32

MichaelBuessemeyer merged commit 49b1c05 into master Aug 25, 2020

philippotto deleted the add-volume-undo branch June 14, 2022 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add volume undo/redo support #4771

Add volume undo/redo support #4771

MichaelBuessemeyer commented Aug 18, 2020 •

edited

Loading

MichaelBuessemeyer commented Aug 19, 2020

daniel-wer left a comment

daniel-wer commented Aug 20, 2020 •

edited by MichaelBuessemeyer

Loading

MichaelBuessemeyer commented Aug 20, 2020

daniel-wer commented Aug 20, 2020

MichaelBuessemeyer commented Aug 20, 2020

daniel-wer left a comment

daniel-wer commented Aug 20, 2020

MichaelBuessemeyer commented Aug 21, 2020

MichaelBuessemeyer commented Aug 21, 2020 •

edited

Loading

MichaelBuessemeyer commented Aug 24, 2020

daniel-wer left a comment

daniel-wer commented Aug 24, 2020

Add volume undo/redo support #4771

Add volume undo/redo support #4771

Conversation

MichaelBuessemeyer commented Aug 18, 2020 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

MichaelBuessemeyer commented Aug 19, 2020

daniel-wer left a comment

Choose a reason for hiding this comment

daniel-wer commented Aug 20, 2020 • edited by MichaelBuessemeyer Loading

MichaelBuessemeyer commented Aug 20, 2020

daniel-wer commented Aug 20, 2020

MichaelBuessemeyer commented Aug 20, 2020

daniel-wer left a comment

Choose a reason for hiding this comment

daniel-wer commented Aug 20, 2020

MichaelBuessemeyer commented Aug 21, 2020

MichaelBuessemeyer commented Aug 21, 2020 • edited Loading

MichaelBuessemeyer commented Aug 24, 2020

daniel-wer left a comment

Choose a reason for hiding this comment

daniel-wer commented Aug 24, 2020

MichaelBuessemeyer commented Aug 18, 2020 •

edited

Loading

daniel-wer commented Aug 20, 2020 •

edited by MichaelBuessemeyer

Loading

MichaelBuessemeyer commented Aug 21, 2020 •

edited

Loading