Fix volume undo for unfetched data #5608
Conversation
|
For documentation purposes: The idea of this fix is, that the merge with the backend data is deferred to the lastest possible point in time -> once the actual undo action is triggered. The fix:
That's all the magic 🧙 |
MichaelBuessemeyer
changed the title
frontend/javascripts/oxalis/model/bucket_data_handling/bucket.js
Outdated
Show resolved
Hide resolved
| backendBucketData.byteLength, | ||
| ); | ||
| const compressedBackendData = await byteArrayToLz4Array(backendDataAsByteArray, true); | ||
| volumeUndoPart.backendData = compressedBackendData; |
There was a problem hiding this comment.
I'm struggling to wrap my head around all of this, but wouldn't it be possible to do the merging which is done in line 348 and line 367, instead here, directly? In my head, this would greatly simplify the applyAndGetRevertingVolumeBatch, because no special treatment would need to be implemented, there (except of maybe waiting for the promise to be resolved). But it could very well be that I'm overlooking something. Let me know if it's easier to discuss this in a call.
There was a problem hiding this comment.
In my head, this would greatly simplify the applyAndGetRevertingVolumeBatch, because no special treatment would need to be implemented, there (except maybe waiting for the promise to be resolved). But it could very well be that I'm overlooking something. Let me know if it's easier to discuss this in a call.
Indeed, it is possible to do the merging once the backend data arrived. But we decided against this to defer the more intensive computation. If the backend data is merged directly once it arrives, the following will be done for each bucket, whose backend data is being fetched, when the user drew over it:
Once the backend data is fetched:
- decompress the current "revert"-bucket data,
- merge it with the backend data,
- compress the result again
And if the user then undos the action:
- decompress the bucket data
- apply the decompressed data
The way the code works now is more complicated but should be (not measured) more efficient in normal uses cases. Because we assume that only a few brush strokes are undone by the user and therefore only a few buckets actually need to be merged with the backend data:
Once the backend data is fetched:
- compress the backend data
And only if the undo action is triggered do the following:
- decompress the backend data and the "normal revert"-bucket data
- merge them
- apply the merged data
As you can see, in the current implementation there are some steps saved in case that the volume action is not undone.
Does this explain the design decision to you and do you think this is justified?
There was a problem hiding this comment.
Thanks a lot for the thorough explanation! It does make sense now that I understand the reasoning :) Could you add a comment summarizing this rationale above line 248?
| backendBucketData.byteLength, | ||
| ); | ||
| const compressedBackendData = await byteArrayToLz4Array(backendDataAsByteArray, true); | ||
| volumeUndoPart.backendData = compressedBackendData; |
There was a problem hiding this comment.
Thanks a lot for the thorough explanation! It does make sense now that I understand the reasoning :) Could you add a comment summarizing this rationale above line 248?
| @@ -91,6 +97,14 @@ type racedActionsNeededForUndoRedo = { | |||
| redo: ?RedoAction, | |||
| }; | |||
|
|
|||
| // Better version: Let the undo stack / batch know that bucket data is not yet fetched by the backend. | |||
There was a problem hiding this comment.
This comment seems a little out of place here. Also, the "Better version:" beginning can be dropped I'd say. Maybe around line 240 would be more suitable to explain this (or above the compressBucketAndAddToUndoBatch function)?
There was a problem hiding this comment.
I simply removed the comment in a previous commit, as these were only notes from the call with philipp on how to solve this bug.
And I think you reviewed an old version here, as I can no longer find the comment and I am pretty sure I removed it, before assigning you as an reviewer 👀 🦢
There was a problem hiding this comment.
You did, but it seems like you didn't push this change until the 16.07. which is why I saw the "old" code during my review. In any case, all good :)
…bleminds/webknossos into fix-volume-undo-for-unfetched-data
This PR fixes the bug that if the user draws over buckets, that aren't fetched by the backend, an undo action of that drawing will overwrite the backend data.
The problem is tackled by adding the original backend data to the volume undo once it was successfully fetched.
Then once the actual drawing action is undone, the bucket data before the drawing action is merged together with the original backend data to not let the backend data get overwritten. Previously, the undo action would have simply taken the bucket data before the drawing action and would not have considered the backend data that was fetched by the backend.
URL of deployed dev instance (used for testing):
Steps to test:
Open a volume / hybrid tracing without an existing segmentation layer
Annotate a large area with one color / cell and save the tracing.
Reload the tracing an make sure that the tracing is reloaded in the 2nd mag, not the mag with the highest resolution.
Once the tracing is reloaded, change to another cell and draw over some of the already annotated parts in the 2nd mag.
Then zoom in and verify that you actually overwrote the data in the lower layer.
Zoom out again and undo your volume action. Now the whole area should only be covered with the previous cell drawing from before the reloading.
Zoom in and verify to the highest mag and verify that the whole area is also filed there.
Repeat the same steps on the master. In the last step, there should be holes in the annotation, as the frontend ignored the unfetched backend data of the highest mag.
Issues:
[ ] Updated (unreleased) migration guide if applicable[ ] Updated documentation if applicable[ ] Adapted wk-connect if datastore API changes[ ] Needs datastore update after deployment