-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fix volume undo for unfetched data #5608
Merged
MichaelBuessemeyer
merged 10 commits into
master
from
fix-volume-undo-for-unfetched-data
Aug 9, 2021
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
e902772
add backend fetched data to the undo stack and apply it on demand
MichaelBuessemeyer e4c92b6
Merge branch 'master' of github.com:scalableminds/webknossos into fix…
MichaelBuessemeyer a9b367e
fix: reenable drawing
MichaelBuessemeyer fdc482d
remove discussion nodes and add changelog entry
MichaelBuessemeyer 1a0394c
Update frontend/javascripts/oxalis/model/bucket_data_handling/bucket.js
MichaelBuessemeyer 614b44a
Merge branch 'master' into fix-volume-undo-for-unfetched-data
MichaelBuessemeyer 95305cf
Merge branch 'fix-volume-undo-for-unfetched-data' of github.com:scala…
MichaelBuessemeyer 7fd968c
add comment explaining deferral of computation
MichaelBuessemeyer 23134f4
Merge branch 'master' into fix-volume-undo-for-unfetched-data
MichaelBuessemeyer 0eaf519
Merge branch 'master' into fix-volume-undo-for-unfetched-data
MichaelBuessemeyer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
And if the user then undos the action:
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:
And only if the undo action is triggered do the following:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?